Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resource API #316

Merged
merged 1 commit into from
Jul 1, 2024
Merged

Resource API #316

merged 1 commit into from
Jul 1, 2024

Conversation

dmlloyd
Copy link
Collaborator

@dmlloyd dmlloyd commented Jun 6, 2024

Proposed resource API. Provides a uniform API for reading resources from the class path or filesystem.

Non-goals are presently including:

  • Support for writing

@geoand
Copy link

geoand commented Jun 7, 2024

Very cool!

I think a follow up with a loader API would be most useful too

@dmlloyd
Copy link
Collaborator Author

dmlloyd commented Jun 7, 2024

I'm making some changes based on wanting a resource loader API as well (adding a canonical relative path to Resource with supporting helpers). I'm going to make this particular submodule require 17+, since it is new code (there is no compatibility risk).

@radcortez
Copy link
Member

Once we get the new Resource Loader API, should we deprecate ClassPathUtils or rewrite it to use the new APIs?

@dmlloyd
Copy link
Collaborator Author

dmlloyd commented Jun 12, 2024

Good question. I'll see if rewriting them with the new APIs is feasible and I'll probably have a better answer then.

@dmlloyd dmlloyd force-pushed the rsrc branch 3 times, most recently from d854203 to c45bc9b Compare June 13, 2024 17:55
@dmlloyd
Copy link
Collaborator Author

dmlloyd commented Jun 13, 2024

OK, here's where we stand with this.

We have Resource, which represents a resource (obviously). You can open a stream from it, get its URL, get its size, get its last-modified time, and get its relative path (which is always canonical by an efficient algorithm, and thus cannot "escape" via ..). If it's a directory, and if the impl supports it, you can iterate the resources in the directory. You can also get a buffer containing the entire contents of the resource, and also get the resource contents as a single string.

There are implementations for JAR file entries, URLs, Path, and memory-backed resources. The memory-backed resource API is future-proof relative to FFM and MemorySegment. The Path-backed API supports mmap of large resources.

Then there's the ResourceLoader interface, which allows you to find a resource given a path relative to the root of the loader. There are resource loader implementations for JAR files (future proof against a possible future port of wildfly-common's "archive" implementation, which supports mmap of JAR files), Path, and URLs.

It should be possible to reimplement (or replace our usages of) ClassPathUtil with this API. However, that module requires Java 11, so we'd have to also bump up the minimum runtime version of that module to 17 (which should be OK since I don't think WildFly is using this API at all). I think a logical next step would be to consider doing so, and then after that, to consider adding a base ClassLoader implementation which can easily make use of resources and resource loaders in some efficient manner.

@dmlloyd dmlloyd marked this pull request as ready for review June 13, 2024 18:01
Copy link

@geoand geoand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

I added a couple of small quesrions

classloader/pom.xml Outdated Show resolved Hide resolved
@dmlloyd dmlloyd force-pushed the rsrc branch 3 times, most recently from b8afb37 to e02c675 Compare June 14, 2024 20:04
@radcortez
Copy link
Member

It should be possible to reimplement (or replace our usages of) ClassPathUtil with this API. However, that module requires Java 11, so we'd have to also bump up the minimum runtime version of that module to 17 (which should be OK since I don't think WildFly is using this API at all).

We use it in SmallRye Config (and maybe other SmallRye projects are using it too). We had some discussions about moving SmallRye to Java 17 a few months ago, and we decided to wait a little longer. We probably need to revisit that discussion.

}
}

private static JarFileResourceLoader makeJar(Entry... entries) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have shrinkwrap as a test dependency in other modules to help with this if you prefer. We can also use it here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to keep it simple in this case, since these tests may require detailed control over the archive format for specific reasons.

@dmlloyd
Copy link
Collaborator Author

dmlloyd commented Jun 17, 2024

It should be possible to reimplement (or replace our usages of) ClassPathUtil with this API. However, that module requires Java 11, so we'd have to also bump up the minimum runtime version of that module to 17 (which should be OK since I don't think WildFly is using this API at all).

We use it in SmallRye Config (and maybe other SmallRye projects are using it too). We had some discussions about moving SmallRye to Java 17 a few months ago, and we decided to wait a little longer. We probably need to revisit that discussion.

I can wait on ClassPathUtil; it's definitely not urgent to change that API. Another option we have is to move Quarkus over to the new API, and deprecate ClassPathUtil and leave it until nobody is using it anymore, and then just drop it in favor of the new API. But we don't have to do anything right away.

@dmlloyd dmlloyd merged commit 9164a6b into smallrye:main Jul 1, 2024
4 checks passed
@dmlloyd dmlloyd deleted the rsrc branch July 1, 2024 15:27
@github-actions github-actions bot added this to the 2.5.0 milestone Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants