Fix URI fragment parsing without query#6393
Conversation
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
|
| Filename | Overview |
|---|---|
| dali/util/uri.cc | Correctly guards authority and path loops against '#' as a terminator, and wraps query parsing in if (*p == '?') so fragment-only URIs bypass the query block entirely. |
| dali/util/uri_test.cc | Adds 4 new test cases covering fragment with/without query and empty-path variants; misses a fragment test on a no-authority URI (e.g. urn:path#frag), and the copyright year is not updated to the current year. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Input URI string] --> B[Parse scheme\nstop at ':']
B --> C{starts with '//'?}
C -- yes --> D[Parse authority\nstop at '/' '?' '#' or NUL]
C -- no --> E
D --> E{*p == NUL?}
E -- yes --> Z[return]
E -- no --> F[Parse path\nstop at '?' '#' or NUL]
F --> G{*p == NUL?}
G -- yes --> Z
G -- no --> H{*p == '?'}
H -- yes --> I[Parse query\nstop at '#' or NUL]
I --> J{*p == NUL?}
J -- yes --> Z
J -- no --> K
H -- no --> K{*p == '#'}
K -- yes --> L[p++ skip '#'\nParse fragment to NUL]
L --> Z
Comments Outside Diff (2)
-
dali/util/uri.cc, line 1 (link)[Style] Copyright year is stale. Both modified files still carry
2024; per thedoc-copyright-current-yearrule, files substantially modified in a PR must have a copyright header whose year range ends in the current year.Rule Used: Files that are added or substantially modified mus... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
dali/util/uri_test.cc, line 1 (link)[Style] Same copyright year issue in the test file.
Rule Used: Files that are added or substantially modified mus... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "Fix URI fragment parsing without query" | Re-trigger Greptile
| TEST(URI, Parse_FragmentWithoutQuery) { | ||
| auto uri = URI::Parse("s3://bucket/path#fragment"); | ||
| EXPECT_EQ("s3", uri.scheme()); | ||
| EXPECT_EQ("bucket", uri.authority()); | ||
| EXPECT_EQ("/path", uri.path()); | ||
| EXPECT_EQ("", uri.query()); | ||
| EXPECT_EQ("/path", uri.path_and_query()); | ||
| EXPECT_EQ("fragment", uri.fragment()); | ||
| } | ||
|
|
||
| TEST(URI, Parse_EmptyQueryWithFragment) { | ||
| auto uri = URI::Parse("s3://bucket/path?#fragment"); | ||
| EXPECT_EQ("s3", uri.scheme()); | ||
| EXPECT_EQ("bucket", uri.authority()); | ||
| EXPECT_EQ("/path", uri.path()); | ||
| EXPECT_EQ("", uri.query()); | ||
| EXPECT_EQ("/path?", uri.path_and_query()); | ||
| EXPECT_EQ("fragment", uri.fragment()); | ||
| } | ||
|
|
||
| TEST(URI, Parse_EmptyPathWithFragment) { | ||
| auto uri = URI::Parse("s3://bucket#fragment"); | ||
| EXPECT_EQ("s3", uri.scheme()); | ||
| EXPECT_EQ("bucket", uri.authority()); | ||
| EXPECT_EQ("", uri.path()); | ||
| EXPECT_EQ("", uri.query()); | ||
| EXPECT_EQ("", uri.path_and_query()); | ||
| EXPECT_EQ("fragment", uri.fragment()); | ||
| } | ||
|
|
||
| TEST(URI, Parse_EmptyPathWithQueryAndFragment) { | ||
| auto uri = URI::Parse("s3://bucket?query#fragment"); | ||
| EXPECT_EQ("s3", uri.scheme()); | ||
| EXPECT_EQ("bucket", uri.authority()); | ||
| EXPECT_EQ("", uri.path()); | ||
| EXPECT_EQ("query", uri.query()); | ||
| EXPECT_EQ("?query", uri.path_and_query()); | ||
| EXPECT_EQ("fragment", uri.fragment()); | ||
| } |
There was a problem hiding this comment.
[Nit] All four new test cases exercise
s3:// URIs (with authority). The path-loop # fix also fires on authority-less URIs such as urn:path#frag or mailto:user@example.com#section, but there is no test for that shape. Adding one test like URI::Parse("urn:path#frag") would give confidence the path-only code path is covered.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
?is present, sopath#fragmentreaches the fragment parser.Validation
git diff --checkdali/util/uri.ccFull gtest was not run locally because this checkout does not have a configured build directory.