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 root path for Quinoa Web UI #691

Merged
merged 7 commits into from
Jun 18, 2024
Merged

Add root path for Quinoa Web UI #691

merged 7 commits into from
Jun 18, 2024

Conversation

liquidnya
Copy link
Contributor

Describe your changes

This change adds the following new property: quarkus.quinoa.ui-root-path
This is the path where the Web UI will be hosted and is always relative to quarkus.http.root-path.
The default value is /, which is a non-breaking change.

For example setting the property quarkus.quinoa.ui-root-path=/ui would serve the Web UI at the path /ui.

The paths in the property quarkus.quinoa.ignored-path-prefixes are relative to quarkus.quinoa.ui-root-path which is a non-braking change because the default of quarkus.quinoa.ui-root-path is /.
I added normalization to the ignored paths, so paths like foo///bar would now be added as /foo/bar and this should be good since ignored paths are checked against the normalized request path from vertx.
When the property is not set then the automatic values are adjusted.
For example when the following properties are set:

quarkus.quinoa.ui-root-path=/app
quarkus.rest.path=/app/api
quarkus.http.non-application-root-path=/q

Then /api/ will be added to the ignored paths automatically.
Note how /q/ is not added to the ignored paths, since it is not a sub-path of /app and the path /app/api/ was adjusted to /api/ since the ignored paths are relative to the ui-root-path.

Bug-Fix:
The changes also contain the following bug fix which was an issue even before these changes:
The quarkus.http.non-application-root-path is now added correctly to the ignored paths.
This is because quarkus.http.non-application-root-path is only relative to quarkus.http.root-path if the non-application path does not start with a slash (/), otherwise it will be an absolute path that is not relative to quarkus.http.root-path.
Therefore setting the following properties

quarkus.http.non-application-root-path=/foo
quarkus.http.root-path=/bar

no longer add the path /foo/ to the ignored paths.
However if the following properties are set

quarkus.http.non-application-root-path=/app/q
quarkus.http.root-path=/app

then /q/ would be added to the ignored paths (and not /app/q/).
Note that quarkus.resteasy.path and quarkus.rest.path are always relative to quarkus.http.root-path even if they start with a slash (/).

Fixes Issue

Fixes #302

Check List (Check all the applicable boxes)

I assume the documentation is automatically updated by the .adoc files or is there any manual step for updating the documentation for changes in config documentation?

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.

This adds the quarkus.quinoa.ui-root-path property which is the path for hosting the Web UI.
The quarkus.quinoa.ignored-path-prefixes property is always relative to quarkus.quinoa.ui-root-path.
The quarkus.http.non-application-root-path is not added to the default ignores if it is not relative to quarkus.http.root-path.
@melloware melloware requested a review from ia3andy June 14, 2024 16:13
@melloware melloware added the enhancement New feature or request label Jun 14, 2024
@liquidnya
Copy link
Contributor Author

liquidnya commented Jun 14, 2024

There is one issue with consistencies between the dev-mode and a production build which also existed before my changes.
When quarkus.http.root-path (or quarkus.quinoa.ui-root-path) is set to a path that is not the root for example set to /ui,
then the path /ui will not serve index.html in production, but it will be served in dev-mode, because the request will be forwarded as /ui to nodejs and nodejs serves index.html at this path!
The path /ui/ will always serve index.html.

This issue is happening because of how Quarkus server files in META-INF/resources versus how vertx or nodejs handles paths.
When adding a route to vertx for example /ui/ then both /ui and /ui/ match that route, but Quarkus will not serve index.html on the route /ui.
Using vite the dev server is hosting index.html on the path /ui when I use this config:

export default defineConfig({
    plugins: [expressPlugin()],
    build: {
        rollupOptions: {
            input: {
                quinoa: resolve(__dirname, 'index.html'),
            }
        }
    },
    base: "/ui",
    server: {
        origin: "/ui"
    }
})

I wonder if this is something that could be changed in Quarkus itself or if there is a reason why Quarkus does not serve index.html on a path like /ui.

If this is not something that should be changed in Quarkus then maybe Quinoa could redirect /ui to /ui/ either with a HTTP redirect or by redirecting this route internally. (This redirect should also be setup for only setting quarkus.http.root-path.)

@liquidnya
Copy link
Contributor Author

If this is not something that should be changed in Quarkus then maybe Quinoa could redirect /ui to /ui/ either with a HTTP redirect or by redirecting this route internally. (This redirect should also be setup for only setting quarkus.http.root-path.)

Another solution would be to explicitly not host /ui when running in dev-mode.

@liquidnya
Copy link
Contributor Author

I also have a back-port of this feature for 2.3.x: https://github.com/liquidnya/quarkus-quinoa/tree/2.3.x
I have this back-port, because initially I started working on this feature before the 2.3.x branch was created.
The back-port also does not have the issue with index.html not being hosted on /ui.
It works there, because the resources are still hosted by Quinoa and not by Quarkus META-INF/resources.

Should I open a PR for this branch?

@melloware

This comment was marked as outdated.

@melloware
Copy link
Contributor

I resolved your merge conflicts

@melloware
Copy link
Contributor

@liquidnya definitely backport to the 2.3.x branch please as well!

@melloware
Copy link
Contributor

@all-contributors add @liquidnya for code, idea

Copy link
Contributor

@melloware

I've put up a pull request to add @liquidnya! 🎉

for (BuiltResourcesBuildItem.BuiltResource resource : uiResources.get().resources()) {
// note how uiRootPath always starts and ends in a slash
// and resource.name() always starts in a slash
Copy link
Collaborator

Choose a reason for hiding this comment

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

from the comments it means there are two slashes no? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I change the comment to say resource.name() always starts in a slash, therefore resource.name().substring(1) does not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the comment to and resource.name() always starts in a slash, therfore resource.name().substring(1) never starts in a slash in commit 083e3f4

@ia3andy
Copy link
Collaborator

ia3andy commented Jun 16, 2024

This looks very good, I'll give it a go tomorrow before approving

@liquidnya
Copy link
Contributor Author

I will make one more change to the PR:
For SPA I will reroute /ui-root-path to /ui-root-path/, such that at least for SPA the missing trailing / will still host index.html.

@ia3andy
Copy link
Collaborator

ia3andy commented Jun 17, 2024

@FroMage @geoand @cescoffier any reason why we don't use the index.html when there is no trailing / ? I guess it's a vert.x choice.

IMO, if the /foo/index.html exists it would make sense to use it for /foo and /foo/?

@liquidnya
Copy link
Contributor Author

I will make one more change to the PR: For SPA I will reroute /ui-root-path to /ui-root-path/, such that at least for SPA the missing trailing / will still host index.html.

This did not work, because the lit example uses ./simple-greeting.js in the html page and when /foo/bar/ui is hosting the html, then the file ./simple-greeting.js can not be found, because it is resolved as /foo/bar/simple-greeting.js (without the ui).
I left the handler as-is, but instead I changed the debug output from Quinoa is available at: /foo/bar/ui to Quinoa is available at: /foo/bar/ui/.

@liquidnya
Copy link
Contributor Author

@FroMage @geoand @cescoffier any reason why we don't use the index.html when there is no trailing / ? I guess it's a vert.x choice.

IMO, if the /foo/index.html exists it would make sense to use it for /foo and /foo/?

I agree!
Vertx actually hosts routes without the trailing /, but the resources in META-INF/resources are not hosted with the trailing /.
I assume that this is a Quarkus choice, but I do not know the code that hosts these files.
So this problem only occurs when using GeneratedStaticResourceBuildItem.

@ia3andy
Copy link
Collaborator

ia3andy commented Jun 17, 2024

Ok, then @cescoffier @geoand @FroMage you can ignore it's just a small bug in the new StaticHandler for dev mode then

upstream issue created: quarkusio/quarkus#41245

@liquidnya
Copy link
Contributor Author

@ia3andy I pushed more changes.

I changed the debug log to print Quinoa is available at: /foo/bar/ui/ instead of Quinoa is available at: /foo/bar/ui.

And I fixed ignored paths:
https://github.com/quarkiverse/quarkus-quinoa/pull/691/files#diff-163687ebbea25059afaaef52f80c04df1f8ad7ecc95cc2a5d81237fe7da7a93aR67-R75
Previously the ignored paths only checked if the request path matches the prefix, but now I added additional logic for the following cases:

  • When the prefix starts with /foo/bar, then the path /foo/bar-not-ignored is no longer ignored.
  • When the user configures /foo/bar/, then the path /foo/bar is ignored (this is because the path is normalized and trailing / are now removed from the list of ignored paths).

I added tests to QuinoaUiRootPathTest.

@liquidnya
Copy link
Contributor Author

@liquidnya definitely backport to the 2.3.x branch please as well!

@melloware I created the PR #696 for the back-port to 2.3.x.

@melloware melloware added this to the 2.4.0 milestone Jun 17, 2024
@melloware melloware merged commit 4eb6dc7 into quarkiverse:main Jun 18, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to serve Quinoa application on a specific path
3 participants