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

Add a configuration to serve a local directory with a static handler #39968

Open
ia3andy opened this issue Apr 9, 2024 · 19 comments
Open

Add a configuration to serve a local directory with a static handler #39968

ia3andy opened this issue Apr 9, 2024 · 19 comments
Labels
area/vertx good first issue Good for newcomers kind/enhancement New feature or request

Comments

@ia3andy
Copy link
Contributor

ia3andy commented Apr 9, 2024

Description

Currently we are suggesting to setup a custom route manually:
https://quarkus.io/guides/http-reference#from-a-local-directory

This would be more elegant and cleaner if it was done in the Quarkus Http using a config

@geoand
Copy link
Contributor

geoand commented Apr 10, 2024

Definitely +1 for this.

P.S. It should be pretty easy to do this.

@ia3andy ia3andy added the good first issue Good for newcomers label Apr 10, 2024
@ia3andy
Copy link
Contributor Author

ia3andy commented Apr 10, 2024

@geoand do you want to add it?

@geoand
Copy link
Contributor

geoand commented Apr 10, 2024

I am pretty sure anyone could do it 😉.

That said, if no one else takes it, I can have a look at some point.

@geoand
Copy link
Contributor

geoand commented Apr 10, 2024

@ia3andy here you go: #39993

@cescoffier
Copy link
Member

I commented on the PR - do NOT add multiple static handlers.

All the static handlers rely on the same file resolver, which contains a cache. If you have concurrent access (which you can do as soon as your number of event loops is greater than 1), BOOM.

We would have to verify that the directories do not overlap and that file names are unique (otherwise, the cache won't work).

@geoand
Copy link
Contributor

geoand commented Apr 11, 2024

@cescoffier then our docs which tell people to add one are also wrong.

@cescoffier
Copy link
Member

Yes and no - it can work, but it may not either (in the sense it could leak or corrupt).

If your path do not overlap and file names are unique, it is going to work (I still need to check ranges).

@geoand
Copy link
Contributor

geoand commented Apr 11, 2024

If your path do not overlap

You mean that each handler should serve a unique root, correct?

and file names are unique

What does this mean exactly? That different instances of StaticHandler can't serve two totally different files that happen to have the same name?

@cescoffier
Copy link
Member

Yes, different roots without overlap (including symbolic links and so on).
And yes, the file names must be unique because of the cache key.

See https://github.com/eclipse-vertx/vert.x/blob/4.5.7/src/main/java/io/vertx/core/file/impl/FileResolverImpl.java#L262
and https://github.com/eclipse-vertx/vert.x/blob/4.5.7/src/main/java/io/vertx/core/file/impl/FileCache.java#L177.

Note the (on purpose) lack of synchronization (and now you understand why I am concerned)

@geoand
Copy link
Contributor

geoand commented Apr 11, 2024

Yes, I see your point.

I am going to close my PR since this needs to be handled properly in Vertx before we do anything in Quarkus to make things easier.

@cescoffier
Copy link
Member

Note that if we want to support only resources from the file system (not from the classloader), things are way easier (and a custom, optimized static handler can be created in a few minutes)

@geoand
Copy link
Contributor

geoand commented Apr 11, 2024

Right, the way I see it, this issue is only about file system resources, not classpath resources.

and a custom, optimized static handler can be created in a few minutes

That should be part of Vertx though, no?

@ia3andy
Copy link
Contributor Author

ia3andy commented Apr 11, 2024

I think we should remove this section from the documentation until this get fixed

@cescoffier
Copy link
Member

@geoand No, I think it should be in Quarkus as we can improve things at build time.

@geoand
Copy link
Contributor

geoand commented Apr 12, 2024

Fair enough

@ia3andy
Copy link
Contributor Author

ia3andy commented Apr 12, 2024

@cescoffier wouldn't that be possible in Vertx to cache with a relative path instead of just the file name?

@cescoffier
Copy link
Member

@ia3andy check the Vertx Cache implementation, my memory is a bit rusty.

@cescoffier
Copy link
Member

@ia3andy did you check on the Vert.x side?

Pretty sure that .. is forbidden to avoid leaving the served directory (security issue)

@ia3andy
Copy link
Contributor Author

ia3andy commented Oct 17, 2024

Maybe we could do this (now that we have GeneratedStaticResourceBuildItem):

// for each files in the configured static dir:
String link = PathUtils.toUnixPath(staticDir.relativize(staticFile)); // Make sure we convert \ to / and use the relative path from the directory as link
 staticResourcesProducer.produce(new GeneratedStaticResourceBuildItem(link, staticFile));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vertx good first issue Good for newcomers kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants