Skip to content

Fix URI fragment parsing without query#6393

Open
fallintoplace wants to merge 1 commit into
NVIDIA:mainfrom
fallintoplace:fix-uri-fragment-parsing
Open

Fix URI fragment parsing without query#6393
fallintoplace wants to merge 1 commit into
NVIDIA:mainfrom
fallintoplace:fix-uri-fragment-parsing

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • Stop URI authority and path parsing at query/fragment delimiters.
  • Only parse a query when ? is present, so path#fragment reaches the fragment parser.
  • Add URI parser coverage for fragment cases with and without a path/query.

Validation

  • git diff --check
  • Focused standalone C++17 parser compile/run against dali/util/uri.cc

Full gtest was not run locally because this checkout does not have a configured build directory.

Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
@greptile-apps

greptile-apps Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a long-standing URI parser bug where a fragment appearing without a preceding ? query was never parsed correctly — the path loop consumed #fragment as part of the path, and the query block ran unconditionally even when the delimiter was # instead of ?.

  • uri.cc: Authority and path loops now stop at # in addition to their previous terminators; the query block is wrapped in if (*p == '?'), so path#fragment correctly skips query parsing and reaches the fragment parser.
  • uri_test.cc: Four new test cases cover fragment-with-path, empty-query-with-fragment, empty-path-with-fragment, and empty-path-with-query-and-fragment.

Confidence Score: 4/5

The core fix is correct: authority, path, and query loops now stop at the right delimiters and the query block is properly guarded. No existing test is broken by the change.

The logic is sound and traces cleanly through all new test cases. The only gaps are a stale copyright year in both files and the absence of a test for the no-authority-plus-fragment shape (e.g. urn:path#frag), which the code handles correctly but goes untested.

No files require special attention; the two style-level notes (copyright year, missing no-authority fragment test) are the only open items.

Important Files Changed

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
Loading

Comments Outside Diff (2)

  1. dali/util/uri.cc, line 1 (link)

    P2 [Style] Copyright year is stale. Both modified files still carry 2024; per the doc-copyright-current-year rule, 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!

  2. dali/util/uri_test.cc, line 1 (link)

    P2 [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

Comment thread dali/util/uri_test.cc
Comment on lines +97 to +135
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());
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 [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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants