-
Notifications
You must be signed in to change notification settings - Fork 3k
Add a configuration to serve a local directory with a static handler #51186
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
base: main
Are you sure you want to change the base?
Conversation
dd79a29 to
2bc57dc
Compare
ia3andy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just cleaning non changed files
...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java
Show resolved
Hide resolved
extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpConfig.java
Show resolved
Hide resolved
...ions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/HttpStaticDirConfig.java
Show resolved
Hide resolved
|
I also forgot, you need to add those files as watched for livereload |
9744cb6 to
691e79a
Compare
ii have added @ia3andy |
691e79a to
4a1de61
Compare
...vertx-http/deployment/src/main/java/io/quarkus/vertx/http/deployment/VertxHttpProcessor.java
Outdated
Show resolved
Hide resolved
...vertx-http/deployment/src/main/java/io/quarkus/vertx/http/deployment/VertxHttpProcessor.java
Outdated
Show resolved
Hide resolved
...vertx-http/deployment/src/main/java/io/quarkus/vertx/http/deployment/VertxHttpProcessor.java
Outdated
Show resolved
Hide resolved
...rtx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/LocalStaticResourcesConfig.java
Outdated
Show resolved
Hide resolved
c6feebb to
55a80ff
Compare
|
Improve validation and handling of local static resources directory
|
ia3andy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
@mathias82 the code is LGTM, could you add the doc part also, I remember an old doc doing this with the StaticHandler (which was bad), but maybe it was removed. |
This comment has been minimized.
This comment has been minimized.
cdd5850 to
76d924b
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9cd78b0 to
ec6685a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ec6685a to
9fce3d1
Compare
| @@ -0,0 +1,3 @@ | |||
| quarkus.http.local-static-resources.enabled=true | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I look at it the more I think using local and resources are confusing, with META-INF/resources which are also local static resources.
path is the path on disk which also makes it clear that it is not the endpoint.
Using / as default is also better as most of the time those static directory are serving at root.
I would suggest using this as default:
quarkus.http.static-dir.enabled=false
quarkus.http.static-dir.endpoint=/
quarkus.http.static-dir.path=static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ia3andy If I understand the @FroMage's comment correctly, quarkus.http.static-dir.endpoint=/ should enable it, without having to also type quarkus.http.static-dir.enabled=true.
Having a default static (static-resources ?) value for quarkus.http.static-dir.path is good.
Perhaps quarkus.http.static-resources-dir or quarkus.http.static-resources-directory is clearer, even if more verbose ? Though I'll let you and @mathias82 agree on this one, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok that's good too, just don't add resources or local in the naming as explained that makes it confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks both. I'll avoid using local or resources in the naming to prevent confusion with META-INF/resources, and I'll stick with the clearer static directory naming as suggested.
I have updated the configuration keys accordingly and push the changes.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| /** | ||
| * The URL path under which the local directory is exposed, | ||
| * e.g. {@code /local-static}. | ||
| */ | ||
| @WithDefault("/local-static") | ||
| String endpoint(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /** | |
| * The URL path under which the local directory is exposed, | |
| * e.g. {@code /local-static}. | |
| */ | |
| @WithDefault("/local-static") | |
| String endpoint(); | |
| /** | |
| * The base endpoint under which the local directory is exposed, | |
| * <p> | |
| * e.g. {@code /static} with hello.txt will be serve at <code>/static/hello.txt</code>. | |
| */ | |
| @WithDefault("/static") | |
| String endpoint(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| To expose the contents of a `static/` directory under the `/local-static` path: | ||
|
|
||
| [source,properties] | ||
| ---- | ||
| quarkus.http.static-dir.path=/local-static | ||
| quarkus.http.static-dir.directory=static | ||
| ---- | ||
|
|
||
| With this configuration, a file located at `static/index.html` will be available at: | ||
| `http://localhost:8080/local-static/index.html`. | ||
|
|
||
| To disable serving local static resources explicitly: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| To expose the contents of a `static/` directory under the `/local-static` path: | |
| [source,properties] | |
| ---- | |
| quarkus.http.static-dir.path=/local-static | |
| quarkus.http.static-dir.directory=static | |
| ---- | |
| With this configuration, a file located at `static/index.html` will be available at: | |
| `http://localhost:8080/local-static/index.html`. | |
| To disable serving local static resources explicitly: | |
| To expose the contents of a `static/` directory under the `/static` endpoint: | |
| [source,properties] | |
| ---- | |
| quarkus.http.static-dir.endpoint=/static | |
| quarkus.http.static-dir.path=static | |
| ---- | |
| With this configuration, a file located at `static/index.html` will be available at: | |
| `http://localhost:8080/static/index.html`. | |
| To disable serving local static resources explicitly: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mathias82 any reason not to auto enable when endpoint or path is not empty as @FroMage suggested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ia3andy @FroMage There is one reason: this feature exposes a local filesystem directory over HTTP.
If we auto-enable it as soon as endpoint or path is set, it becomes easier
for large apps with layered configuration to accidentally expose a directory
just because some partial config was inherited or left behind.
By keeping enabled as an explicit opt-in switch, we avoid accidental exposure
and make the intent very clear: nothing is served unless the user consciously
enables it.
That said, if the project convention is to auto-enable on config, I can update
the implementation accordingly, I just wanted to highlight the security/ops
angle behind the current design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FroMage I let you decide on that one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mathias82 the change I suggested is still appllicable
|
|
||
| Router router = httpRouter.getValue(); | ||
| router.route(basePath + "/*") | ||
| .handler(new HttpStaticDirHandler(staticFiles)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mathias82 what is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ia3andy This registers a catch-all Vert.x route for the configured static directory.
registerHttpStaticDirRoute collects all discovered static files at build time and
passes them to registerHttpStaticDir(...), which at runtime installs:
router.route(basePath + "/*").handler(new HttpStaticDirHandler(staticFiles));
So this line effectively exposes every file under the configured static-dir.path
at <basePath>/*, and delegates the resolution to HttpStaticDirHandler.
This comment has been minimized.
This comment has been minimized.
… local directory.
672e2a8 to
1dc209d
Compare
Status for workflow
|
Summary
This PR introduces support for serving static files from a local directory,
configurable via
quarkus.http.local-static-resources.*.New configuration
quarkus.http.local-static-resources.enabled=true
quarkus.http.local-static-resources.path=/local-static/
quarkus.http.local-static-resources.directory=local-static
Tests
Added
LocalStaticResourcesTestwhich verifies: