Skip to content

Guard optional path parameters before formatting #11760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Copilot wants to merge 12 commits into
base: main
Choose a base branch
from
+722 −22
Open

Guard optional path parameters before formatting #11760

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5940399
Initial plan
Copilot Aug 25, 2026
ceee147
fix(http-client-csharp): guard nullable date path formatting
Copilot Aug 25, 2026
e919a5c
fix(http-client-csharp): exclude enums from nullable value unwrap, us…
Copilot Aug 25, 2026
bd2a68c
fix(http-client-csharp): remove redundant guard unwrap and fix enum d…
Copilot Aug 25, 2026
3068324
fix(http-client-csharp): guard nullable required path params, remove …
Copilot Aug 25, 2026
4795fb4
test(http-client-csharp): cover nullable path guard branches
Copilot Aug 25, 2026
df2e7a3
Merge branch 'main' into copilot/address-missing-nullable-value-guard
jorgerangel-msft Aug 26, 2026
baead84
fix(http-client-csharp): guard and defer separator for required-nulla…
Copilot Aug 26, 2026
e8ad7b0
fix(http-client-csharp): use resolved path parameter's input name for…
Copilot Aug 26, 2026
c4c78a9
fix(http-client-csharp): keep path separator for required nullable en…
Copilot Aug 26, 2026
91c227b
chore(http-client-csharp): remove stale inline comments in path guard…
Copilot Aug 26, 2026
be4eef7
fix(http-client-csharp): only guard optional path parameters and null…
jorgerangel-msft Aug 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 34 additions & 16 deletions ...rp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/RestClientProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -907,26 +907,19 @@ private void AddUriSegments(
var paramEndIndex = pathSpan.IndexOf('}');
var paramName = pathSpan.Slice(0, paramEndIndex).ToString();

/* An optional path parameter that is null must not leave a dangling
* path separator behind. For example "/foo/{bar}/{baz}" with an absent
* optional "baz" should produce "/foo/{bar}", not "/foo/{bar}/". When the
* upcoming parameter is optional, defer the trailing '/' of the preceding
* literal so it is only written together with the parameter value inside
* the null check below.
*/
bool hasPathOrEndpointParam = inputParamMap.TryGetValue(paramName, out var pathParamForGuard)
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
&& pathParamForGuard is InputPathParameter or InputEndpointParameter;
bool willEmitNullGuard = hasPathOrEndpointParam
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
&& (pathParamForGuard!.IsRequired == false || pathParamForGuard.Type is InputNullableType);
var pathLiteral = path.ToString();
bool separatorDeferred = false;
if (pathLiteral.EndsWith('/')
&& inputParamMap.TryGetValue(paramName, out var optionalCheckParam)
&& optionalCheckParam is InputPathParameter { IsRequired: false })
&& willEmitNullGuard)
{
pathLiteral = pathLiteral.Substring(0, pathLiteral.Length - 1);
separatorDeferred = true;
}
AppendLiteralSegment(uri, pathLiteral, statements);
/* when the parameter is in operation.uri, it is client parameter
* It is not operation parameter and not in inputParamHash list.
*/
var isClientParameter = ClientProvider.ClientParameters.Any(p => string.Equals(p.Name, paramName, StringComparison.OrdinalIgnoreCase))
|| _inputClient.Parameters.Any(p => p is InputMethodParameter { ParamAlias: string alias } && string.Equals(alias, paramName, StringComparison.OrdinalIgnoreCase));
CSharpType? type;
Expand Down Expand Up @@ -959,23 +952,48 @@ private void AddUriSegments(
bool escape = !inputPathParameter?.SkipUrlEncoding ?? true;
if (type?.OutputType.IsCollection == true)
{
statements.Add(uri.AppendPathDelimited(valueExpression, GetFormatEnumValue(serializationFormat), escape).Terminate());
MethodBodyStatement collectionStatement = uri.AppendPathDelimited(valueExpression, GetFormatEnumValue(serializationFormat), escape).Terminate();
if (willEmitNullGuard)
{
bool shouldPrependWithPathSeparator = separatorDeferred || (path.Length > 0 && path[^1] != '/');
List<MethodBodyStatement> appendPathStatements = shouldPrependWithPathSeparator
? [uri.AppendPath(Literal("/"), false).Terminate(), collectionStatement]
: [collectionStatement];
collectionStatement = BuildQueryOrHeaderOrPathParameterNullCheck(
Comment thread
jorgerangel-msft marked this conversation as resolved.
type,
valueExpression,
appendPathStatements);
}
statements.Add(collectionStatement);
}
else
{
valueExpression = type?.Equals(typeof(string)) == true
var nullCheckExpression = valueExpression;
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
if (type is { IsNullable: true, IsValueType: true, IsEnum: false })
{
valueExpression = willEmitNullGuard
? valueExpression.Property(nameof(Nullable<int>.Value))
: valueExpression.NullConditional();

@JoshLove-msft JoshLove-msft Aug 27, 2026

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.

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

}
else if (type is { IsNullable: true, IsEnum: true } && willEmitNullGuard && paramMap.TryGetValue(inputParam?.Name ?? paramName, out var enumParamProvider))
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
{
ValueExpression rawEnumVariable = enumParamProvider.Field is null ? enumParamProvider : enumParamProvider.Field;
nullCheckExpression = rawEnumVariable;
valueExpression = type.ToSerial(rawEnumVariable.Property(nameof(Nullable<int>.Value)));
}
valueExpression = type?.Equals(typeof(string)) == true || type?.IsEnum == true
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
? valueExpression
: valueExpression.Invoke(nameof(ToString), toStringParams);
MethodBodyStatement statement;
if (inputParam?.IsRequired == false)
if (willEmitNullGuard)
{
bool shouldPrependWithPathSeparator = separatorDeferred || (path.Length > 0 && path[^1] != '/');
List<MethodBodyStatement> appendPathStatements = shouldPrependWithPathSeparator
? [uri.AppendPath(Literal("/"), false).Terminate(), uri.AppendPath(valueExpression, escape).Terminate()]
: [uri.AppendPath(valueExpression, escape).Terminate()];
statement = BuildQueryOrHeaderOrPathParameterNullCheck(
type,
valueExpression,
nullCheckExpression,
appendPathStatements);
}
else
Expand Down
206 changes: 206 additions & 0 deletions ...eSpec.Generator.ClientModel/test/Providers/RestClientProviders/RestClientProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1329,6 +1329,212 @@ public void TestBuildCreateRequestMethodWithPathParameters()
Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content);
}

[TestCase(true)]
[TestCase(false)]
public void NullableDatePathParameterIsGuardedBeforeFormatting(bool isRequired)
{
var dateType = new InputDateTimeType(
DateTimeKnownEncoding.Rfc7231,
"utcDateTime",
"TypeSpec.utcDateTime",
InputPrimitiveType.String);
var operation = InputFactory.Operation(
"GetThing",
parameters: [InputFactory.PathParameter("requestOn", dateType, isRequired: isRequired)],
path: "/things/{requestOn}");
var serviceMethod = InputFactory.BasicServiceMethod(
"GetThing",
operation,
parameters:
[
InputFactory.MethodParameter(
"requestOn",
dateType,
isRequired: isRequired,
location: InputRequestLocation.Path)
]);
var client = InputFactory.Client("TestClient", methods: [serviceMethod]);
var restClient = new ClientProvider(client).RestClient;

var file = new TypeProviderWriter(restClient).Write();
Assert.AreEqual(Helpers.GetExpectedFromFile(parameters: isRequired.ToString()), file.Content);
}

[Test]
public void OptionalEnumPathParameterIsNotUnwrapped()
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
{
var enumType = InputFactory.StringEnum(
"Color",
[("Red", "red"), ("Blue", "blue")],
isExtensible: true);
var operation = InputFactory.Operation(
"GetThing",
parameters: [InputFactory.PathParameter("color", enumType, isRequired: false, scope: InputParameterScope.Client)],
path: "/things/{color}");
var serviceMethod = InputFactory.BasicServiceMethod(
"GetThing",
operation);
var client = InputFactory.Client("TestClient", methods: [serviceMethod]);
var restClient = new ClientProvider(client).RestClient;

var file = new TypeProviderWriter(restClient).Write();
Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content);
}

[Test]
public void OptionalMethodEnumPathParameterIsSerializedOnce()

@JoshLove-msft JoshLove-msft Aug 27, 2026

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.

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

{
var enumType = InputFactory.StringEnum(
"Color",
[("Red", "red"), ("Blue", "blue")],
isExtensible: true);
var operation = InputFactory.Operation(
"GetThing",
parameters: [InputFactory.PathParameter("color", enumType, isRequired: false)],
path: "/things/{color}");
var serviceMethod = InputFactory.BasicServiceMethod(
"GetThing",
operation,
parameters:
[
InputFactory.MethodParameter(
"color",
enumType,
isRequired: false,
location: InputRequestLocation.Path)
]);
var client = InputFactory.Client("TestClient", methods: [serviceMethod]);
var restClient = new ClientProvider(client).RestClient;

var file = new TypeProviderWriter(restClient).Write();
Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content);
}

[Test]
public void OptionalClientEnumPathParameterWithDistinctSerializedNameIsGuarded()
{
var enumType = InputFactory.StringEnum(
"Color",
[("Red", "red"), ("Blue", "blue")],
isExtensible: true);
var operation = InputFactory.Operation(
"GetThing",
parameters: [InputFactory.PathParameter("color", enumType, isRequired: false, serializedName: "colour", scope: InputParameterScope.Client)],
path: "/things/{colour}");
var serviceMethod = InputFactory.BasicServiceMethod(
"GetThing",
operation);
var client = InputFactory.Client("TestClient", methods: [serviceMethod]);
var restClient = new ClientProvider(client).RestClient;

var file = new TypeProviderWriter(restClient).Write();
Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content);
}

[Test]
public void RequiredNullableStringPathParameterIsGuarded()
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
{
var nullableString = new InputNullableType(InputPrimitiveType.String);
var operation = InputFactory.Operation(
"GetThing",
parameters: [InputFactory.PathParameter("name", nullableString, isRequired: true)],
path: "/things/{name}");
var serviceMethod = InputFactory.BasicServiceMethod(
"GetThing",
operation,
parameters:
[
InputFactory.MethodParameter(
"name",
nullableString,
isRequired: true,
location: InputRequestLocation.Path)
]);
var client = InputFactory.Client("TestClient", methods: [serviceMethod]);
var restClient = new ClientProvider(client).RestClient;

var file = new TypeProviderWriter(restClient).Write();
Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content);
}

[Test]
public void RequiredNullableEnumPathParameterIsGuarded()
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
{
var enumType = InputFactory.StringEnum(
"Color",
[("Red", "red"), ("Blue", "blue")],
isExtensible: true);
var nullableEnum = new InputNullableType(enumType);
var operation = InputFactory.Operation(
"GetThing",
parameters: [InputFactory.PathParameter("color", nullableEnum, isRequired: true)],
path: "/things/{color}");
var serviceMethod = InputFactory.BasicServiceMethod(
"GetThing",
operation,
parameters:
[
InputFactory.MethodParameter(
"color",
nullableEnum,
isRequired: true,
location: InputRequestLocation.Path)
]);
var client = InputFactory.Client("TestClient", methods: [serviceMethod]);
var restClient = new ClientProvider(client).RestClient;

var file = new TypeProviderWriter(restClient).Write();
Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content);
}

[Test]
public void RequiredNullableCollectionPathParameterIsGuarded()
{
var nullableArray = new InputNullableType(InputFactory.Array(InputPrimitiveType.String));
var operation = InputFactory.Operation(
"GetThing",
parameters: [InputFactory.PathParameter("ids", nullableArray, isRequired: true)],
path: "/things/{ids}");
var serviceMethod = InputFactory.BasicServiceMethod(
"GetThing",
operation,
parameters:
[
InputFactory.MethodParameter(
"ids",
nullableArray,
isRequired: true,
location: InputRequestLocation.Path)
]);
var client = InputFactory.Client("TestClient", methods: [serviceMethod]);
var restClient = new ClientProvider(client).RestClient;

var file = new TypeProviderWriter(restClient).Write();
Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content);
}

[Test]
public void OptionalClientDatePathParameterIsGuardedBeforeFormatting()
Comment thread
jorgerangel-msft marked this conversation as resolved.
{
var dateType = new InputDateTimeType(
DateTimeKnownEncoding.Rfc7231,
"utcDateTime",
"TypeSpec.utcDateTime",
InputPrimitiveType.String);
var operation = InputFactory.Operation(
"GetThing",
parameters: [InputFactory.PathParameter("requestOn", dateType, isRequired: false, scope: InputParameterScope.Client)],
path: "/things/{requestOn}");
var serviceMethod = InputFactory.BasicServiceMethod(
"GetThing",
operation);
var client = InputFactory.Client("TestClient", methods: [serviceMethod]);
var restClient = new ClientProvider(client).RestClient;

var file = new TypeProviderWriter(restClient).Write();
Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content);
}

// An optional trailing path parameter must not emit a dangling separator when null.
// e.g. "/certificates/{certificateName}/{certificateVersion}" with a null version
// should produce "/certificates/{name}", not "/certificates/{name}/".
Expand Down
32 changes: 32 additions & 0 deletions ...Data/RestClientProviderTests/NullableDatePathParameterIsGuardedBeforeFormatting(False).cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// <auto-generated/>

#nullable disable

using System;
using System.ClientModel.Primitives;

namespace Sample
{
public partial class TestClient
{
private static global::System.ClientModel.Primitives.PipelineMessageClassifier _pipelineMessageClassifier200;

private static global::System.ClientModel.Primitives.PipelineMessageClassifier PipelineMessageClassifier200 => _pipelineMessageClassifier200 ??= global::System.ClientModel.Primitives.PipelineMessageClassifier.Create(stackalloc ushort[] { 200 });

internal global::System.ClientModel.Primitives.PipelineMessage CreateGetThingRequest(global::System.DateTimeOffset? requestOn, global::System.ClientModel.Primitives.RequestOptions options)
{
global::Sample.ClientUriBuilder uri = new global::Sample.ClientUriBuilder();
uri.Reset(_endpoint);
uri.AppendPath("/things", false);
if ((requestOn != null))
{
uri.AppendPath("/", false);
uri.AppendPath(requestOn.Value.ToString("R"), true);
}
global::System.ClientModel.Primitives.PipelineMessage message = Pipeline.CreateMessage(uri.ToUri(), "GET", PipelineMessageClassifier200);
global::System.ClientModel.Primitives.PipelineRequest request = message.Request;
message.Apply(options);
return message;
}
}
}
28 changes: 28 additions & 0 deletions ...tData/RestClientProviderTests/NullableDatePathParameterIsGuardedBeforeFormatting(True).cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// <auto-generated/>

#nullable disable

using System;
using System.ClientModel.Primitives;

namespace Sample
{
public partial class TestClient
{
private static global::System.ClientModel.Primitives.PipelineMessageClassifier _pipelineMessageClassifier200;

private static global::System.ClientModel.Primitives.PipelineMessageClassifier PipelineMessageClassifier200 => _pipelineMessageClassifier200 ??= global::System.ClientModel.Primitives.PipelineMessageClassifier.Create(stackalloc ushort[] { 200 });

internal global::System.ClientModel.Primitives.PipelineMessage CreateGetThingRequest(global::System.DateTimeOffset requestOn, global::System.ClientModel.Primitives.RequestOptions options)
{
global::Sample.ClientUriBuilder uri = new global::Sample.ClientUriBuilder();
uri.Reset(_endpoint);
uri.AppendPath("/things/", false);
uri.AppendPath(requestOn.ToString("R"), true);
global::System.ClientModel.Primitives.PipelineMessage message = Pipeline.CreateMessage(uri.ToUri(), "GET", PipelineMessageClassifier200);
global::System.ClientModel.Primitives.PipelineRequest request = message.Request;
message.Apply(options);
return message;
}
}
}
31 changes: 31 additions & 0 deletions ...tData/RestClientProviderTests/OptionalClientDatePathParameterIsGuardedBeforeFormatting.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// <auto-generated/>

#nullable disable

using System.ClientModel.Primitives;

namespace Sample
{
public partial class TestClient
{
private static global::System.ClientModel.Primitives.PipelineMessageClassifier _pipelineMessageClassifier200;

private static global::System.ClientModel.Primitives.PipelineMessageClassifier PipelineMessageClassifier200 => _pipelineMessageClassifier200 ??= global::System.ClientModel.Primitives.PipelineMessageClassifier.Create(stackalloc ushort[] { 200 });

internal global::System.ClientModel.Primitives.PipelineMessage CreateGetThingRequest(global::System.ClientModel.Primitives.RequestOptions options)
{
global::Sample.ClientUriBuilder uri = new global::Sample.ClientUriBuilder();
uri.Reset(_endpoint);
uri.AppendPath("/things", false);
if ((_requestOn != null))
{
uri.AppendPath("/", false);
uri.AppendPath(_requestOn.Value.ToString("R"), true);
}
global::System.ClientModel.Primitives.PipelineMessage message = Pipeline.CreateMessage(uri.ToUri(), "GET", PipelineMessageClassifier200);
global::System.ClientModel.Primitives.PipelineRequest request = message.Request;
message.Apply(options);
return message;
}
}
}
Loading
Loading