Guard optional path parameters before formatting

[Copilot]

Nullable path parameters were formatted before checking for null, generating invalid calls such as requestOn.ToString("R") on DateTimeOffset?.

Request generation

• Retain the raw parameter or field for the guard; format the underlying value only inside the guarded block.

if (requestOn != null)
{
    uri.AppendPath("/", false);
    uri.AppendPath(requestOn.Value.ToString("R"), true);
}

• Compute the guard decision once, before the separator-deferral logic, and reuse it for both the deferral and the guard emission. Previously these were two separate predicates — one on the input model, one on the resolved CSharpType — which could disagree and drop the / separator entirely (e.g. /thingsred).
• Widen the guard to InputEndpointParameter in addition to InputPathParameter, so optional parameters in a server URL template are handled too.
• Emit a guard for optional collection path parameters. Their separator was already deferred but never restored, producing /thingsa,b.
• Null checks are now always written against the raw parameter/field rather than the serialized form. GetParamInfo returns the expression it already resolved, which removes a duplicate lookup whose failure path could emit code that does not compile, and avoids vacuous checks like _color.ToString() != null.
• Replace the over-broad type?.IsEnum == true suppression of ToString() with an isSerialized flag set where serialization is actually applied. This also fixes the double _color?.ToString().ToString() emitted for enum path parameters.

Behavior decision: required parameters are never guarded

Guarding is deliberately limited to optional path/endpoint parameters, even when the parameter's type is nullable.

Omitting a required segment would silently target a different resource — /things instead of /things/{name} — surfacing as a confusing 404 or, worse, a successful call against the collection endpoint. Today ClientUriBuilder.AppendPath calls Uri.EscapeDataString unconditionally and throws ArgumentNullException on null, and that loud, immediately diagnosable failure is preferable. A required-but-nullable parameter therefore keeps its existing unguarded shape.

Regression coverage

New and updated RestClientProviderTests snapshots:

Test	Pins
NullableDatePathParameterIsGuardedBeforeFormatting(True/False)	Required and optional RFC 7231 date-time
OptionalClientEnumPathParameterIsGuardedThenUnwrapped	Guard then .Value unwrap; no double ToString()
OptionalMethodEnumPathParameterIsSerializedOnce	Method-scoped enum serialized once
OptionalClientEnumPathParameterWithDistinctSerializedNameIsGuarded	Serialized name differing from parameter name
OptionalClientDatePathParameterIsGuardedBeforeFormatting	Client-scoped date-time
OptionalCollectionPathParameterIsGuarded	Optional collection separator restored
OptionalEndpointParameterInServerTemplateIsGuarded	InputEndpointParameter in a {endpoint}/{region} template
RequiredNullableStringPathParameterIsNotGuarded	Required-nullable policy above
RequiredNullableEnumPathParameterIsNotGuarded	Required-nullable enum, separator intact
RequiredNullableClientEnumPathParameterIsNotGuarded	Client-scoped variant of the same
RequiredNullableCollectionPathParameterIsNotGuarded	Required-nullable collection

Output for non-nullable parameters is unchanged, and the generated Spector/Local test projects are unaffected.

• Fixes Address Missing Nullable Value Guard in CreateRequest Method for DateTime Params #11759

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@typespec/http-client-csharp@11760

commit: be4eef7

[Copilot]

Pull request overview

Fixes nullable date-time path serialization so formatting occurs only after a null guard.

Changes:

• Retains the nullable expression for guard generation.
• Unwraps nullable value types before formatting.
• Adds required and optional RFC 7231 regression coverage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
RestClientProvider.cs	Adjusts nullable path formatting and guards.
RestClientProviderTests.cs	Adds regression tests.
NullableDatePathParameterIsGuardedBeforeFormatting(True).cs	Verifies required parameter output.
NullableDatePathParameterIsGuardedBeforeFormatting(False).cs	Verifies guarded optional parameter output.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/RestClientProvider.cs:993

• paramName is the URI placeholder (the parameter's serialized name), but ParameterProviderMap only registers input/original/alias/generated names. For a nullable enum whose serialized name differs from its input name, this lookup either misses—serializing the enum twice—or can resolve a colliding unrelated parameter and generate .Value against the wrong variable. Resolve the provider with pathParamForGuard.Name, matching GetParamInfo's lookup at line 1100.

                    else if (type is { IsNullable: true, IsEnum: true } && willEmitNullGuard && paramMap.TryGetValue(paramName, out var enumParamProvider))

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

[JoshLove-msft]

Review

I built this branch and diffed the generated output against main across 17 path-parameter shapes to see exactly what changes. The core fix is real and it fixes more than the description claims — but it also introduces one URL-shape regression that the 1602-test ClientModel suite does not catch.

Verified fixes (all confirmed by generating against main and this branch)

case	main	this PR
optional method date-time	if ((requestOn.ToString("R") != null))	if ((requestOn != null)) + requestOn.Value.ToString("R")
optional client date-time	_requestOn.ToString("R") — no guard at all, and does not compile on DateTimeOffset?	guarded, .Value inside
required client extensible enum	_color.ToString().ToString()	_color.ToString()
optional client extensible enum	_color?.ToString().ToString()	guarded, _color.Value.ToString()
optional collection path param	AppendPath("/things") then AppendPathDelimited(ids, ...) → /thingsa,b	guarded, separator restored

The optional-client-date-time case and the optional-collection case are both genuine bugs beyond the linked issue. Worth mentioning in the description, since they change output for existing specs.

One regression

Introducing a second, differently-spelled nullability predicate means the separator-deferral at line 922 and the guard emission at line 962 can now disagree. When they do, the trailing / is stripped and never re-emitted. A required-but-nullable enum path parameter hits this:

main:  uri.AppendPath("/things/", false);  uri.AppendPath(color, true);   =>  /things/red
PR:    uri.AppendPath("/things",  false);  uri.AppendPath(color, true);   =>  /thingsred

Details and repro inline on line 922. This is the same shape as your own RequiredNullableStringPathParameterIsGuarded test, just with an enum instead of a string, so it is squarely inside the scope this PR takes on.

One design question

Required-but-nullable parameters now silently omit the path segment when null, rather than throwing. That is a deliberate-looking choice baked into the new snapshots, but it turns a loud failure into a request against a different resource. Inline on the test.

Nothing here is blocking beyond the separator bug. Full ClientModel suite (1602) passes on this branch, which is itself the point — none of the above is covered.

--generated by Copilot

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

[JoshLove-msft]

Re-review (at 91c227b)

Re-ran the same 25-shape generated-output comparison against main and against the previous head e8ad7b0.

Confirmed fixed

The separator regression is gone. willEmitNullGuard is now computed once from the input-parameter metadata and reused for both separator deferral and guard emission, which is exactly the right shape. Verified — required-nullable enum path parameter, method-scoped:

e8ad7b0:  uri.AppendPath("/things", false);  uri.AppendPath(color, true);   =>  /thingsred
91c227b:  uri.AppendPath("/things", false);  if ((color != null)) { "/" ; color }   =>  /things/red

RequiredNullableEnumPathParameterIsGuarded locks it in, and the duplicate inputParamMap lookup is gone. Full ClientModel suite: 1603 passed.

New problem introduced by 91c227b

That commit deleted two comments that were not stale and not added by this PR — they are on main today. The request was to drop the bot's own // A guard is needed whenever... comment, which had already gone in c4c78a92. Details inline; I'd restore both.

Remaining divergence (same root cause, new symptom)

Unifying the predicate fixed the separator, but willEmitNullGuard is now derived purely from InputNullableType while the null-check expression and the .Value unwrap below are still derived from the CSharpType. Where those disagree you now get a guard that tests the already-formatted value:

// required nullable extensible enum, client-scoped
if ((_color.ToString() != null))
{
    uri.AppendPath("/", false);
    uri.AppendPath(_color.ToString(), true);
}

That is structurally the same defect as #11759 (if ((requestOn.ToString("R") != null))) — the null check applied after formatting rather than before. It is only harmless right now because of a separate pre-existing bug. Detail inline on line 971.

Still open from the previous round

6 of my 9 comments have no reply yet: the paramMap.TryGetValue silent fallthrough (line 978), the breadth of type?.IsEnum == true (line 984), missing coverage for the optional-collection fix, the OptionalEnumPathParameterIsNotUnwrapped naming, the required-nullable "silently omit the segment" design question, and endpoint-parameter coverage. Brief pointers re-attached at the current line numbers so they don't get lost.

--generated by Copilot

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[JoshLove-msft]

Reviewed at be4eef706. I built and ran the full Microsoft.TypeSpec.Generator.ClientModel.Tests suite (1606 passing) and verified each point below against generated output rather than the diff alone.

The core direction is right — computing willEmitNullGuard once and sharing it with the separator deferral is the correct fix for the /thingsred divergence, returning the raw expression from GetParamInfo removes both the duplicate lookup and the vacuous _color.ToString() != null, and OptionalCollectionPathParameterIsGuarded correctly pins the ChangeTrackingList guard.

Two things I would want resolved before merge:

1. The required-but-nullable value-type branch — the one that fixes the reported bug — has zero test coverage. I verified this by mutation: deleting it does not fail a single test.
2. The required-nullable client-scoped enum snapshot pins a silent empty path segment, which contradicts the "fail loudly" rationale the PR description uses to justify leaving required parameters unguarded.

Details inline.

--generated by Copilot

[JoshLove-msft]

This branch has no test coverage — including for the exact bug this PR fixes.

I mutation-tested it: replacing .NullConditional() here with a plain valueExpression leaves all 1606 tests in Microsoft.TypeSpec.Generator.ClientModel.Tests passing.

With that mutation applied, a required-but-nullable date path parameter generates:

internal PipelineMessage CreateGetThingRequest(global::System.DateTimeOffset? requestOn, RequestOptions options)
{
    uri.AppendPath("/things/", false);
    uri.AppendPath(requestOn.ToString("R"), true);   // CS1501: DateTimeOffset? has no ToString(string)
}

That is precisely issue #11759. Unmutated, the code correctly emits requestOn?.ToString("R") — so the line is load-bearing, it is just unprotected.

The gap is in NullableDatePathParameterIsGuardedBeforeFormatting: it passes dateType directly rather than new InputNullableType(dateType), so the isRequired: true case produces a non-nullable DateTimeOffset and never reaches this branch. Every other new RequiredNullable* test uses a type that sidesteps it too — string, collection, or an enum that flattens to string.

Suggest adding a RequiredNullableDatePathParameterIsNotGuarded test built from new InputNullableType(dateType) with isRequired: true, pinning uri.AppendPath(requestOn?.ToString("R"), true).

--generated by Copilot

[JoshLove-msft]

This snapshot pins a silent failure, which contradicts the behavior decision in the PR description.

_color is declared Color? here, and Nullable<T>.ToString() returns "" when HasValue is false — it does not throw. So a null value silently produces /things/ and the request targets the collection endpoint, which is exactly the "confusing 404 or, worse, a successful call against the collection endpoint" the PR description argues against.

The rationale for leaving required parameters unguarded rests on AppendPath failing loudly:

Today ClientUriBuilder.AppendPath calls Uri.EscapeDataString unconditionally and throws ArgumentNullException on null, and that loud, immediately diagnosable failure is preferable.

That holds for the method-scoped variant (RequiredNullableEnumPathParameterIsNotGuarded), which flattens to string color and does reach EscapeDataString(null). It does not hold for this client-scoped one, because ToString() swallows the null before AppendPath ever sees it.

If the intent is to fail loudly, _color.Value.ToString() would throw InvalidOperationException instead. Either way the divergence between the two required-nullable enum shapes is worth calling out explicitly rather than baking into a snapshot.

--generated by Copilot

[JoshLove-msft]

The widening to InputEndpointParameter is incomplete for the client-parameter case.

willEmitNullGuard is sourced solely from inputParamMap, which is built from operation.Parameters — but the comment restored a few lines below says the opposite about client parameters:

/* when the parameter is in operation.uri, it is client parameter
 * It is not operation parameter and not in inputParamHash list.
 */

When that comment's premise holds, TryGetValue misses, willEmitNullGuard is silently false, and both the guard and the separator deferral are skipped. I confirmed this with an optional endpoint parameter present in client.Parameters but absent from operation.Parameters, template {endpoint}/{region}:

uri.Reset(_endpoint);
uri.AppendPath("/", false);
uri.AppendPath(_region, true);   // unguarded; ArgumentNullException on null, plus a dangling "/"
uri.AppendPath("/things", false);

All the new client-scoped tests (OptionalClientDatePathParameterIsGuardedBeforeFormatting, OptionalClientEnumPathParameterIsGuardedThenUnwrapped, OptionalEndpointParameterInServerTemplateIsGuarded) pass the parameter in operation.Parameters, so this shape is not covered.

Worth deciding whether the comment's premise is still accurate. If it is, consider falling back to the resolved ParameterProvider/field optionality when inputParamMap misses; if it is not, the comment is now misleading and should go.

--generated by Copilot

[JoshLove-msft]

These two tests do not exercise enum serialization at all.

Both OptionalMethodEnumPathParameterIsSerializedOnce and RequiredNullableEnumPathParameterIsNotGuarded generate a string parameter, because a method-scoped enum is flattened to its serialized primitive in the protocol method:

internal PipelineMessage CreateGetThingRequest(string color, RequestOptions options)
...
uri.AppendPath(color, true);

That output is byte-identical to what a plain string path parameter produces, so neither test can detect a regression in the isSerialized / ToSerial logic they are named for — RequiredNullableEnumPathParameterIsNotGuarded is currently a duplicate of RequiredNullableStringPathParameterIsNotGuarded with a different name.

The real coverage for the double-ToString() fix comes from the client-scoped variants, which do resolve to a nullable enum field. Either drop these two or add a comment noting they are pinning the flattened-to-string shape rather than enum serialization.

--generated by Copilot

[JoshLove-msft]

Minor: seven parameters, five of them out, and two of the three call sites now discard with out _, out _. A readonly record struct ParamInfo(CSharpType? Type, SerializationFormat? Format, ValueExpression? Value, ValueExpression? RawValue, bool IsSerialized) return would read better and make the out _ sites unnecessary.

Also, the new doc comment documents 2 of the 7 parameters and has no <summary>, which is a slightly odd shape for XML docs. Consider folding the rawValueExpression note into a <summary> instead.

--generated by Copilot