SLING-11352 - Fix parsing of path-only mappings #84
Conversation
It wasn't possible to add a regular path-only mapping to /etc/map because the sling:match property was mis-parsed as a URI.
|
Kudos, SonarCloud Quality Gate passed! |
|
Hi @csaboka - thank you for the PR and apologies for the delay. I guess no one had the time to look into this. I hope to do so next week, the fix seems quite simple but I don't understand all the implications yet. |
|
@csaboka - I tried to figure out whether this is safe to apply. I noticed that at https://github.com/apache/sling-org-apache-sling-resourceresolver/blob/master/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntry.java#L199-L201 we explicitly remove a prefix, would this scenario cause your proposal to behave unexpectedly? In addition, it would be good if @cziegeler could also take a look. FTR, the Starter ITs pass with this change, so nothing already tested seems to be broken. |
|
Using matches() instead of find() sounds correct to me; however we have some complicated code around it and it is hard to see whether this will break something else somewhere. |
|
@rombert : I can't say I understand this code completely, either. I'm not even sure if the "if" block you highlighted can ever be entered. MapEntries.ANY_SCHEME_HOST equals "[^/]+/[^/]+", but if the "url" variable contains any square brackets, the isRegExp() check on line 182 will cause an early return. Therefore, we know that the url cannot start with ANY_SCHEME_HOST at that point. I agree with @cziegeler that this change seems to be right, but there is no easy way to tell if something is subtly depending on the current (IMO broken) behavior. |
|
I've assigned myself as a reviewer to make sure it's on my TODO. Not sure when I will get around to it, if anyone beats me to it I won't mind :-) |
|
Kudos, SonarCloud Quality Gate passed! |
|
|
1 similar comment
|
|
It wasn't possible to add a regular path-only mapping to /etc/map because the sling:match property was mis-parsed as a URI.