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.

Sign up for GitHub

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

Jump to bottom

Ensure that index.html works in any directory in native mode #39878

Merged
merged 1 commit into from
Apr 8, 2024
Merged

Ensure that index.html works in any directory in native mode #39878

merged 1 commit into from
Apr 8, 2024

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Apr 4, 2024 •
edited
Loading

@geoand geoand mentioned this pull request Apr 4, 2024
Closed
@geoand geoand marked this pull request as ready for review April 4, 2024 12:09
@geoand geoand requested a review from zakkak April 4, 2024 12:09
zakkak
zakkak reviewed Apr 4, 2024
View reviewed changes
...http/deployment/src/main/java/io/quarkus/vertx/http/deployment/StaticResourcesProcessor.java Outdated
@@ -75,7 +75,15 @@ public void nativeImageResource(Optional<StaticResourcesBuildItem> staticResourc
// TODO: do we perhaps want to register the whole directory?
continue;
}
String metaInfResourcesPath = StaticResourcesRecorder.META_INF_RESOURCES + entry.getPath();
String path = entry.getPath();
int index = path.lastIndexOf("/index.html");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is index.html the only default page name supported in JVM mode? What about index.htm for example?

Thinking out loud: Why not registering all directory paths leading up to a file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is index.html the only default page name supported in JVM mode? What about index.htm for example

Good point! It's actually configurable, so I'll use the config property

Thinking out loud: Why not registering all directory paths leading up to a file?

Because no one really needs them AFAIK.

Copy link
Contributor Author

@geoand geoand Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good point! It's actually configurable, so I'll use the config property

Actually the property is runtime... So there is not much we can do other than move the property to build time (which we would be a breaking change).

Copy link
Contributor

Choose a reason for hiding this comment

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

Or reconsider the

Because no one really needs them AFAIK.

part :)

If I get it right that would allow the users to still set the property at runtime and have it work for any file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That still would not be enough

Copy link
Member

Choose a reason for hiding this comment

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

Well, my understanding is that you were trying to cherry pick the directories with an index resource, which would be hard given you don't know the name of the index resource for sure at this point.
So my question is: I don't think it would cost much to register all directories in META-INF/resources and simplify the problem.
And I don't foresee any bad side effects.

Or did I miss something?

Copy link
Contributor Author

@geoand geoand Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, now I see what you mean.

I support we could do that, it probably won't cost much of anything when it comes to native image size.
A follow up question to that approach would be whether we want to do this recursively or not.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think you need to do that recursively given an index.html can be nested.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW that was what I meant as well. +1

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 be fixed now

integration-tests/vertx-http/src/test/java/io/quarkus/it/vertx/StaticResourcesTest.java Show resolved Hide resolved
@geoand geoand requested a review from zakkak April 4, 2024 15:26
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
zakkak
zakkak approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

LGTM

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Apr 8, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 24b3fa7.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@geoand geoand merged commit c275507 into quarkusio:main Apr 8, 2024
50 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Apr 8, 2024
@gsmet gsmet modified the milestones: 3.10 - main, 3.9.3 Apr 9, 2024
@gsmet gsmet modified the milestones: 3.9.3, 3.8.5 May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet left review comments

@zakkak zakkak zakkak approved these changes

Assignees
No one assigned
Labels
area/vertx kind/bugfix triage/flaky-test
Projects
None yet
Milestone
3.8.5
Development

Successfully merging this pull request may close these issues.

Routing for index.html fails with 404 for directories
3 participants
@geoand @gsmet @zakkak