Optimise path decoding in RoutingUtils

[ia3andy]

• Closes: Performance regression while resolving path #47384

[ia3andy]

@franz1981 I'll try to submit a follow up PR fixing it even when there is % in the path..

[franz1981]

In theory URLDecoder can do the same here (slow path) - but what looks like a waste to me is that Vertx already spent cycles doing something here which can be replaced by a single method that does it at once.
And in theory the better home for this util method IMO is in vertex itself.
I am surprised they don't need any of this, to avoid security issues @cescoffier

[ia3andy]

@franz1981 all the vertx tooling for this is not exposed as public API

[franz1981]

Let's change it @ia3andy ❤️
@vietj can we summon you buddy?

[franz1981]

Julien is on PTO, let's wait him to come back

[ia3andy]

I think it's a bit more complicated than checking for '%' as we also need the remove dots '../' and replace '' with '/'. Let's hold a bit

[ia3andy]

This is what's in the internal API that we would need:
io.vertx.core.internal.net.RFC3986

[gsmet]

Same here, I would expect Vert.x to do this for us, I'm surprised we have to handle this manually.

[ia3andy]

I don't think we can avoid the dots removal :-/ @franz1981 when this PR is green could you run the perf check again? It will probably be less effective than before the breaking commit but still better than using URI all the time..

[franz1981]

In any case I am on PTO, right now, so cannot perform tests. My suggestion is to use the JMH module we have in quarkus and write a bench..and I can help review it 🙏

[ia3andy]

Ok, we are good!

There is no need to remove the dots are it's already done by vertx in the ctx.normalizedPath(). It takes care of decoding unreserved characters including '.' (https://datatracker.ietf.org/doc/html/rfc3986#section-2.3) and doing the dot removal with a check on slow path.

Now in getNormalizedAndDecodedPath, we re-check for '%' to also decode the special characters such as é.

This should fix the issue.

[ia3andy]

@vietj @tsegismont https://github.com/vert-x3/vertx-web/blob/master/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/StaticHandlerImpl.java#L174

It seems doing the remove dots there is not needed here as it's already done as explained in my previous comment.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 537c43c.

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✔️	JVM Integration Tests - JDK 17			Logs	Raw logs	🔍
✔️	JVM Integration Tests - JDK 17 Windows			Logs	Raw logs	🔍
✖	JVM Integration Tests - JDK 21	Build	Failures	Logs	Raw logs	🔍

Full information is available in the Build summary check run.
You can consult the Develocity build scans.

Failures

⚙️ JVM Integration Tests - JDK 21 #

- Failing: integration-tests/observability-lgtm 

📦 integration-tests/observability-lgtm

✖ io.quarkus.observability.test.LgtmReloadTest.testReload - History - More details - Source on GitHub

java.lang.IllegalStateException: 
Bad response: 404 >> 404 page not found

	at io.quarkus.observability.test.utils.GrafanaClient.handle(GrafanaClient.java:51)
	at io.quarkus.observability.test.utils.GrafanaClient.user(GrafanaClient.java:81)
	at org.awaitility.core.AbstractHamcrestCondition.lambda$new$0(AbstractHamcrestCondition.java:48)
	at org.awaitility.core.ConditionAwaiter$ConditionPoller.call(ConditionAwaiter.java:248)
	at org.awaitility.core.ConditionAwaiter$ConditionPoller.call(ConditionAwaiter.java:235)

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging/deployment

✖ io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

• Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError

java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)

[ia3andy]

@franz1981 this should be safe to merge, can you approve?

[franz1981]

Lgtm

[gsmet]

I'm not saying we shouldn't merge this as a quick fix but I'm still wondering why we have to handle things ourselves.

My understanding is that the issue is only with GeneratedStaticResourceBuildItem so I'm wondering if we should somehow alter the registered endpoint for these and escape it when we register it, rather than doing things at runtime when the request arrives.

My understanding is that we should be able to deal with this once and for all when we register the resource and then have a standard handling as if the files were on disk.

I suppose I might be missing something so happy to have an explanation :).

[gsmet]

FWIW, I have been looking at both 7899c9d and the original issue and I'm not clear on what the original patch was exactly supposed to fix.

Sorry for asking dumb questions but I would appreciate if we could take a step back and clarify things for posterity.

[ia3andy]

@gsmet the test in the original fix should explain the purpose.

[ia3andy]

Here was the original issue: #45775

[ia3andy]

Also the issue is not for GeneratedStaticResourceBuildItem but for all static resources.

[ia3andy]

My understanding is that the issue is only with GeneratedStaticResourceBuildItem so I'm wondering if we should somehow alter the registered endpoint for these and escape it when we register it, rather than doing things at runtime when the request arrives.

This is how the vertx static handler works. @cescoffier or @tsegismont might have answer to this question.

[ia3andy]

Here is where we need this: https://github.com/quarkusio/quarkus/blob/main/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/StaticResourcesRecorder.java#L98-L107

Also we are using this method from RoutingUtils from other extensions which rely on path to get resources (Qute Web for example)

[franz1981]

I got the feeling that similarly to how we scan the endpoints to decide how to configure the right handlers for the types required (see

quarkus/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/startup/RuntimeResourceDeployment.java

Line 144 in 0061772

public RuntimeResource buildResourceMethod(ResourceClass clazz,

)
we should be able there to check if the endpoint path matches the one which should contains static resources and save any check to happen since they won't deliver anything, but I don't remember by heart

[ia3andy]

While I agree we could investigate ways to have vertx deal with this logic in the first place, until then we should apply this fix and backport it as it solves a perf issue which affect all requests (including rest).

The fix is pretty safe:
• It uses URIDecoder#decodeURIComponent, just like the static handler implementation.
• There’s a test for special characters and dots to validate the behaviour.
• If there’s no %, the behaviour is the same as before the initial patch.

[tsegismont]

@vietj @tsegismont https://github.com/vert-x3/vertx-web/blob/master/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/StaticHandlerImpl.java#L174

It seems doing the remove dots there is not needed here as it's already done as explained in my previous comment.

Thanks @ia3andy for the heads-up

[tsegismont]

@vietj @tsegismont https://github.com/vert-x3/vertx-web/blob/master/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/StaticHandlerImpl.java#L174
It seems doing the remove dots there is not needed here as it's already done as explained in my previous comment.

Thanks @ia3andy for the heads-up

@ia3andy in fact, it is required because rc.normalizedPath() will not remove dot segments in case of an encoded backslah (see https://github.com/vert-x3/vertx-web/blob/0e4a13c5dcfe1edd0512f279b272554c47ef4c4b/vertx-web/src/test/java/io/vertx/ext/web/tests/handler/StaticHandlerTest.java#L955)

[ia3andy]

Having to remove dots twice feels quite wrong. Maybe we should consider adding an exception to the RFC decode step to also handle \, since they’re eventually converted to / anyway?

[ia3andy]

Or maybe we should remove support for windows path, because URI paths \ are not meant to be used as path delimiter (as they are not unreserved).

[tsegismont]

I believe it happens in two steps because a route handler (other than a static file server) can be interested in having a normalized path without fully decoding uri.