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
Open

Guard optional path parameters before formatting #11760

Show file tree
Hide file tree
Changes from all 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
88 changes: 66 additions & 22 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 @@ -500,7 +500,7 @@ private IEnumerable<MethodBodyStatement> AppendHeaderParameters(HttpRequestApi r
CSharpType? type;
SerializationFormat? serializationFormat;
ValueExpression? valueExpression;
GetParamInfo(paramMap, operation, inputHeaderParameter, out type, out serializationFormat, out valueExpression);
GetParamInfo(paramMap, operation, inputHeaderParameter, out type, out serializationFormat, out valueExpression, out _, out _);
if (valueExpression == null)
{
continue;
Expand Down Expand Up @@ -576,7 +576,7 @@ private List<MethodBodyStatement> AppendQueryParameters(ScopedApi uri, InputOper
InputOperation operation,
bool isNextLinkRequest = false)
{
GetParamInfo(paramMap, operation, inputQueryParameter, out var paramType, out var serializationFormat, out var valueExpression);
GetParamInfo(paramMap, operation, inputQueryParameter, out var paramType, out var serializationFormat, out var valueExpression, out _, out _);
if (valueExpression == null)
{
return null;
Expand Down Expand Up @@ -907,18 +907,13 @@ 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 willEmitNullGuard = inputParamMap.TryGetValue(paramName, out var optionalCheckParam)
&& optionalCheckParam is InputPathParameter or InputEndpointParameter
&& !optionalCheckParam.IsRequired;

@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.

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

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;
Expand All @@ -932,17 +927,19 @@ private void AddUriSegments(
CSharpType? type;
SerializationFormat? serializationFormat;
ValueExpression? valueExpression;
ValueExpression? rawValueExpression;
bool isSerialized;
InputParameter? inputParam = null;
if (isClientParameter)
{
GetParamInfo(paramMap[paramName], out type, out serializationFormat, out valueExpression);
GetParamInfo(paramMap[paramName], out type, out serializationFormat, out valueExpression, out rawValueExpression, out isSerialized);
}
else
{
inputParam = inputParamMap[paramName];
if (inputParam is InputPathParameter || inputParam is InputEndpointParameter)
if (inputParam is InputPathParameter or InputEndpointParameter)
{
GetParamInfo(paramMap, operation, inputParam, out type, out serializationFormat, out valueExpression);
GetParamInfo(paramMap, operation, inputParam, out type, out serializationFormat, out valueExpression, out rawValueExpression, out isSerialized);
if (valueExpression == null)
{
break;
Expand All @@ -957,25 +954,57 @@ private void AddUriSegments(
ValueExpression[] toStringParams = format is null ? [] : [Literal(format)];
InputPathParameter? inputPathParameter = inputParam as InputPathParameter;
bool escape = !inputPathParameter?.SkipUrlEncoding ?? true;
/* The null check must always test the raw parameter or field, never the
* serialized form: `x.ToString("R") != null` is vacuously true and
* double-evaluates the serialization.
*/
ValueExpression nullCheckExpression = rawValueExpression ?? valueExpression;
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,
nullCheckExpression,
appendPathStatements);
}
statements.Add(collectionStatement);
}
else
{
valueExpression = type?.Equals(typeof(string)) == true
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)
{
/* GetParamInfo serialized the nullable enum through a null-conditional
* access. The guard makes that redundant, so re-serialize from the
* unwrapped value instead.
*/
valueExpression = type.ToSerial(nullCheckExpression.Property(nameof(Nullable<int>.Value)));
isSerialized = true;
}
valueExpression = isSerialized || type?.Equals(typeof(string)) == true
? 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 Expand Up @@ -1027,12 +1056,18 @@ private static void AppendLiteralSegment(ScopedApi uri, string literal, List<Met
}
}

private void GetParamInfo(ParameterProviderMap paramMap, InputOperation operation, InputParameter inputParam, out CSharpType? type, out SerializationFormat? serializationFormat, out ValueExpression? valueExpression)
/// <param name="rawValueExpression">
/// The unformatted parameter or field access backing <paramref name="valueExpression"/>. Null checks must be
/// written against this rather than <paramref name="valueExpression"/>, which may already be serialized.
/// </param>
/// <param name="isSerialized">Whether <paramref name="valueExpression"/> has already had serialization applied.</param>
private void GetParamInfo(ParameterProviderMap paramMap, InputOperation operation, InputParameter inputParam, out CSharpType? type, out SerializationFormat? serializationFormat, out ValueExpression? valueExpression, out ValueExpression? rawValueExpression, out bool isSerialized)

@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.

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

{
type = IsContentTypeParameter(inputParam, includeInputHeaderParameter: false)
? null
: ScmCodeModelGenerator.Instance.TypeFactory.CreateCSharpType(inputParam.Type);
serializationFormat = null;
isSerialized = false;

if (inputParam.IsApiVersion && ClientProvider.IsMultiServiceClient)
{
Expand All @@ -1042,52 +1077,61 @@ private void GetParamInfo(ParameterProviderMap paramMap, InputOperation operatio
type = apiVersionField.Type;
serializationFormat = apiVersionField.WireInfo?.SerializationFormat;
valueExpression = apiVersionField;
rawValueExpression = apiVersionField;
return;
}
}

if (inputParam.Scope == InputParameterScope.Constant && !(operation.IsMultipartFormData && inputParam is InputHeaderParameter headerParameter && headerParameter.IsContentType))
{
valueExpression = Literal((inputParam.Type as InputLiteralType)?.Value);
rawValueExpression = valueExpression;
serializationFormat = ScmCodeModelGenerator.Instance.TypeFactory.GetSerializationFormat(inputParam.Type);
}
else if (TryGetAcceptHeaderWithMultipleContentTypes(inputParam, operation, out var contentTypes))
{
string joinedContentTypes = string.Join(", ", contentTypes);
valueExpression = Literal(joinedContentTypes);
rawValueExpression = valueExpression;
serializationFormat = ScmCodeModelGenerator.Instance.TypeFactory.GetSerializationFormat(inputParam.Type);
}
else if (TryGetSpecialHeaderParam(inputParam, out var parameterProvider))
{
valueExpression = parameterProvider.DefaultValue!;
rawValueExpression = valueExpression;
serializationFormat = ScmCodeModelGenerator.Instance.TypeFactory.GetSerializationFormat(inputParam.Type);
}
else
{
if (paramMap.TryGetValue(inputParam.Name, out var paramProvider))
{
GetParamInfo(paramProvider, out type, out serializationFormat, out valueExpression);
GetParamInfo(paramProvider, out type, out serializationFormat, out valueExpression, out var raw, out isSerialized);
rawValueExpression = raw;
}
else
{
type = null;
valueExpression = null;
rawValueExpression = null;
}
}
}

private static void GetParamInfo(ParameterProvider paramProvider, out CSharpType? type, out SerializationFormat? serializationFormat, out ValueExpression valueExpression)
private static void GetParamInfo(ParameterProvider paramProvider, out CSharpType? type, out SerializationFormat? serializationFormat, out ValueExpression valueExpression, out ValueExpression rawValueExpression, out bool isSerialized)
{
type = paramProvider.Field is null ? paramProvider.Type : paramProvider.Field.Type;
rawValueExpression = paramProvider.Field is null ? paramProvider : paramProvider.Field;
if (type.IsEnum)
{
valueExpression = type.ToSerial(paramProvider);
serializationFormat = SerializationFormat.Default;
isSerialized = true;
}
else
{
valueExpression = paramProvider.Field is null ? paramProvider : paramProvider.Field;
valueExpression = rawValueExpression;
serializationFormat = paramProvider.WireInfo.SerializationFormat;
isSerialized = false;
}
}

Expand Down
Loading
Loading