Skip to content

feat: add generated request building support for path parameters in the URL.#2174

Draft
calebkiage wants to merge 7 commits into
reactiveui:mainfrom
calebkiage:u/calebkiage/aot-for-path-parameters
Draft

feat: add generated request building support for path parameters in the URL.#2174
calebkiage wants to merge 7 commits into
reactiveui:mainfrom
calebkiage:u/calebkiage/aot-for-path-parameters

Conversation

@calebkiage

Copy link
Copy Markdown

What kind of change does this PR introduce?

Adds generated request building support for path parameters provided in the URL.

What is the new behavior?

If a URL has path parameters defined in the URL and the method signature has parameters matching the path parameter, then the source generator will create a method that uses source generation.

The method signature below will now use a generated method body.

interface IProfileClient {
    [Get("/users/{userId}"]
    Task<User> GetUser(string userId);
}

What is the current behavior?

The old behavior used reflection for any methods that have parameters in the path.

What might this PR break?

None

Checklist

  • I have read the Contribute guide
  • Tests have been added or updated (for bug fixes / features)
  • Docs have been added or updated (for bug fixes / features)
  • Changes target the main branch
  • PR title follows Conventional Commits

Additional information

@glennawatson glennawatson left a comment

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.

Thanks for the draft. Know it's WIP - a few notes that should help when you pick it back up.

Runtime bug in GeneratedRequestRunner.BuildRequestPath:

var isPathParam = index > 0 && toReplace[index - 1] == '/';

toReplace[index - 1] indexes the literal "{key}" instead of path; should be path[index - 1]. As written, /users/{user} throws IndexOutOfRangeException and /{id} leaves the placeholder unreplaced. One-char fix.

Test gap: the new tests only check generated text + CompilesWithoutErrors; nothing executes a path-param request. The one emit-load-invoke test uses [Get("/users")] (no param). An emit-load-invoke case asserting the final RequestUri (single, multiple, non-string, null/empty) would catch the bug above - most valuable addition here.

Smaller, for later:

  • Empty value drops the whole segment (/users/{user} -> /users); reflection substitutes ?? string.Empty -> /users/. Worth aligning so inline and reflective URIs match.
  • replacementBlockRegex adds a per-match alloc into the parse hot path stage 1 de-LINQ'd, and the camelCase static name likely trips the analyzers; a manual scan avoids both and drops the duplicate brace logic.
  • Confirm params ReadOnlySpan<(string, string?)> (C# 13) compiles on the net4x TFMs - no CI ran on the branch.

Use scanning over regex matching in Parser.Request.cs.
Support query parameters that are part of the path string.
# Conflicts:
#	src/Refit/PublicAPI/net10.0/PublicAPI.Unshipped.txt
#	src/Refit/PublicAPI/net11.0/PublicAPI.Unshipped.txt
#	src/Refit/PublicAPI/net462/PublicAPI.Unshipped.txt
#	src/Refit/PublicAPI/net470/PublicAPI.Unshipped.txt
#	src/Refit/PublicAPI/net471/PublicAPI.Unshipped.txt
#	src/Refit/PublicAPI/net472/PublicAPI.Unshipped.txt
#	src/Refit/PublicAPI/net48/PublicAPI.Unshipped.txt
#	src/Refit/PublicAPI/net481/PublicAPI.Unshipped.txt
#	src/Refit/PublicAPI/net8.0/PublicAPI.Unshipped.txt
#	src/Refit/PublicAPI/net9.0/PublicAPI.Unshipped.txt
@calebkiage

calebkiage commented Jun 28, 2026

Copy link
Copy Markdown
Author

@glennawatson, good catch on the bug. Was working on this late.

Test gap: the new tests only check generated text + CompilesWithoutErrors; nothing executes a path-param request.

It looks like there are already tests that check for parameter substitution. They were failing and I've fixed most of them.

Empty value drops the whole segment (/users/{user} -> /users); reflection substitutes ?? string.Empty -> /users/. Worth aligning so inline and reflective URIs match.

So if a path like /users/{id}/orders gets a null value then the new URL will be /users//orders?


How should generated code behave when IUrlParameterFormatter has been set? Currently, the generated code doesn't apply any formatting to the values.

@glennawatson

Copy link
Copy Markdown
Contributor

So if a path like /users/{id}/orders gets a null value then the new URL will be /users//orders?

Yep, agreed -- and I think that's the behavior we actually want. It matches what reflection already does (Format(...) ?? string.Empty) and it's RFC 6570 simple expansion (an empty var expands to ""), so inline and reflective URIs stay identical. I'd rather keep the // than have the generator special-case it and drift from reflection. If we ever want a null path arg to fail fast, that's a separate change we'd make in both paths.

How should generated code behave when IUrlParameterFormatter has been set? Currently, the generated code doesn't apply any formatting to the values.

This is the part I care more about -- it's a real parity gap vs reflection. Let's get the generated code calling into it: route each path value through settings.UrlParameterFormatter.Format(value, attributeProvider, type) instead of .ToString(). The only snag is the ICustomAttributeProvider arg -- we don't want to reflect ParameterInfo at runtime under AOT. Could we pass the statically-known typeof(TParam) plus a generator-emitted (or empty) ICustomAttributeProvider, and emit a real per-parameter provider later if a formatter needs the attributes? That honors custom/type formatters in the common case and keeps us off reflection.

@calebkiage calebkiage force-pushed the u/calebkiage/aot-for-path-parameters branch from a2d898c to 11acde7 Compare June 29, 2026 14:19
@calebkiage

calebkiage commented Jun 29, 2026

Copy link
Copy Markdown
Author

I have the first iteration of UrlParameterFormatter support. Can you take a look?

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.

2 participants