Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,9 @@ private void AddUriSegments(
*/
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));
bool willEmitNullGuard = inputParamMap.TryGetValue(paramName, out var pathParamForGuard)
&& pathParamForGuard.IsRequired == false
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
&& (pathParamForGuard is InputPathParameter || pathParamForGuard is InputEndpointParameter);
CSharpType? type;
SerializationFormat? serializationFormat;
ValueExpression? valueExpression;
Expand Down Expand Up @@ -942,19 +945,40 @@ private void AddUriSegments(
}
else
{
valueExpression = type?.Equals(typeof(string)) == true
// The null check must always be performed against the raw, still-nullable value:
// for enums, GetParamInfo has already serialized valueExpression (e.g. via a
// null-conditional ToString call), so capture the check subject before any further
// unwrapping below could make it unconditionally dereference a null value.
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
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();

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(paramName, out var enumParamProvider))
Comment thread
jorgerangel-msft marked this conversation as resolved.
Outdated
{
// Already inside a null guard: check the raw nullable field directly (instead of
// repeating the null-conditional serialized expression) and unwrap it via .Value
// before serializing, avoiding both a redundant guard invocation and a duplicate
// ToString call.
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
Original file line number Diff line number Diff line change
Expand Up @@ -1164,6 +1164,58 @@ 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);
}

// 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
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;
}
}
}
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;
}
}
}
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 ((_color != null))
{
uri.AppendPath("/", false);
uri.AppendPath(_color.Value.ToString(), 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