Match ns prefix by longest match, not first match #142
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, when two namespaces like /chtc and /chtc/PUBLIC existed in the cache, the matchPrefix function wound up matching whichever happened to be read first. Now it matches against the longest prefix, so a request for path /chtc/PUBLIC/foo/bar will always match /chtc/PUBLIC. Closes issue PelicanPlatform#140
bbockelm
requested changes
Sep 16, 2023
Unsurprisingly, it turns out writing the unit test was a great idea because it caught an additional bug that was a bit more nuanced. This time, the bug came from the fact that some caches/origins might support a namespace prefix that's shorter than the best/ longest prefix without supporting that longest prefix. In those cases, the wrong namespace (and slices of origins/caches) might wind up being returned not by `matchesPrefix`, but by `GetCacheAdsForPath` due to the fact that `/chtc` IS the best namespace prefix for the path `/chtc/PUBLIC/foo` if a cache only supports `/chtc`. I don't love all having to deal with all of these trailing /'s, but because I'm assuming there's a reason that was allowed in topology, so I'm hoping my test cases are robust enough to catch all the nuanced edge cases.
A very good idea -- caught a more nuanced bug in the process of writing the test. |
bbockelm
approved these changes
Sep 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, when two namespaces like /chtc and /chtc/PUBLIC existed in the cache, the matchPrefix function wound up matching whichever happened to be read first. Now it matches against the longest prefix, so a request for path /chtc/PUBLIC/foo/bar will always match /chtc/PUBLIC.
Closes issue #140