Skip to content
Open
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license.

using System.Globalization;
Expand All @@ -21,12 +21,28 @@ public static class OpenApiSerializableExtensions
/// <param name="stream">The output stream.</param>
/// <param name="specVersion">The Open API specification version.</param>
/// <param name="cancellationToken">The cancellation token.</param>
#pragma warning disable RS0027 // The settings overload below has the same parameter count but different types; no ambiguity exists.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those suppressions are binary safe, but they are not source safe for a specific edge case.

await doc.SerializeAsJsonAsync(version, default);
await doc.SerializeAsync(version, OpenApiConstants.Json, default);
await doc.SerializeAsJsonAsync(stream, version, default);

Would now fail to compile because of the ambiguity. We'll need to review the approach here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noodled on the problem for the last 15 minutes or so. Also came across this documentation

Given the current state of things, I don't think we have a ton of options:

  • Add the settings as the LAST parameter, that's ugly but should work. And we can clean it up in the next major version.
  • Use a different method name. Ugly as well, but should work. We could even mark the existing methods as obsolete for now. And then we could clean up the API surface in the next major version.
  • Conditionally add those overloads to net8+ only, and add the OverloadResolutionPriorityAttribute on the older methods. And clean all of this up in the next major version.
  • (not merging this pull request at all, unless we get overwhelming feedback this is needed, and leave the implementation to the next major version).

Thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For option 3, Youssef Fahmy (@Youssef1313) correctly pointed out the attribute is only available starting with net9.0, so we'd need to add a new target to the project (most likely net10.0) and make the conditional compilation use that. This increases the cost of that solution (not only in terms of maintenance, but for the consumers when pulling down the package, etc...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with option 1 — placed CancellationToken before settings so callers must pass it explicitly. This removes all ambiguity. The new signatures would be:

SerializeAsJsonAsync(Stream stream, OpenApiSpecVersion specVersion, CancellationToken cancellationToken, OpenApiJsonWriterSettings settings)

SerializeAsJsonAsync(OpenApiSpecVersion specVersion, CancellationToken cancellationToken, OpenApiJsonWriterSettings settings)

SerializeAsync(OpenApiSpecVersion specVersion, string format, CancellationToken cancellationToken, OpenApiWriterSettings? settings)

Does this work for you?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's probably the least worst option. Thank you for making the changes!

Can you also add a // TODO: comment to fix that on the next major release please?

I'll also ask a colleague's input to make sure "it's ok" from an API design perspective.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It violates https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1068, not sure if you're okay with such violation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, we could create the overloads as experimental until the next major version so people who take a dependency on it know it'll change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Youssef Fahmy (@Youssef1313) Thank you for the additional information.

So our ONLY clean option would be to use a different method name at this point?

public static Task SerializeAsJsonAsync<T>(this T element, Stream stream, OpenApiSpecVersion specVersion, CancellationToken cancellationToken = default)
#pragma warning restore RS0027
Comment thread
baywet marked this conversation as resolved.
Outdated
where T : IOpenApiSerializable
{
return element.SerializeAsync(stream, specVersion, OpenApiConstants.Json, cancellationToken);
}

/// <summary>
/// Serialize the <see cref="IOpenApiSerializable"/> to the Open API document (JSON) using the given stream, specification version and settings.
/// </summary>
/// <typeparam name="T">the <see cref="IOpenApiSerializable"/></typeparam>
/// <param name="element">The Open API element.</param>
/// <param name="stream">The output stream.</param>
/// <param name="specVersion">The Open API specification version.</param>
/// <param name="settings">Settings controlling JSON output, including <see cref="OpenApiJsonWriterSettings.Terse"/> for compact formatting.</param>
public static Task SerializeAsJsonAsync<T>(this T element, Stream stream, OpenApiSpecVersion specVersion, OpenApiJsonWriterSettings settings)
Comment thread
Mahdigln marked this conversation as resolved.
Outdated
where T : IOpenApiSerializable
{
return element.SerializeAsync(stream, specVersion, OpenApiConstants.Json, settings, CancellationToken.None);
}

/// <summary>
/// Serializes the <see cref="IOpenApiSerializable"/> to the Open API document (YAML) using the given stream and specification version.
/// </summary>
Expand Down Expand Up @@ -104,7 +120,9 @@ public static Task SerializeAsync<T>(
/// <param name="writer">The output writer.</param>
/// <param name="specVersion">Version of the specification the output should conform to</param>
/// <param name="cancellationToken">The cancellation token.</param>
#pragma warning disable RS0027 // The settings-bearing SerializeAsync overloads below have the same parameter count but different types; no ambiguity exists.
public static Task SerializeAsync<T>(this T element, IOpenApiWriter writer, OpenApiSpecVersion specVersion, CancellationToken cancellationToken = default)
#pragma warning restore RS0027
where T : IOpenApiSerializable
Comment thread
baywet marked this conversation as resolved.
Outdated
{
Utils.CheckArgumentNull(element);
Expand Down Expand Up @@ -142,15 +160,33 @@ public static Task SerializeAsync<T>(this T element, IOpenApiWriter writer, Open
/// <param name="element">The Open API element.</param>
/// <param name="specVersion">The Open API specification version.</param>
/// <param name="cancellationToken">The cancellation token.</param>
#pragma warning disable RS0027 // The settings overload below has the same parameter count but different types; no ambiguity exists.
public static Task<string> SerializeAsJsonAsync<T>(
this T element,
OpenApiSpecVersion specVersion,
CancellationToken cancellationToken = default)
#pragma warning restore RS0027
where T : IOpenApiSerializable
{
return element.SerializeAsync(specVersion, OpenApiConstants.Json, cancellationToken);
}

/// <summary>
/// Serializes the <see cref="IOpenApiSerializable"/> to the Open API document as a string in JSON format using the given settings.
/// </summary>
/// <typeparam name="T">the <see cref="IOpenApiSerializable"/></typeparam>
/// <param name="element">The Open API element.</param>
/// <param name="specVersion">The Open API specification version.</param>
/// <param name="settings">Settings controlling JSON output, including <see cref="OpenApiJsonWriterSettings.Terse"/> for compact formatting.</param>
public static Task<string> SerializeAsJsonAsync<T>(
this T element,
OpenApiSpecVersion specVersion,
OpenApiJsonWriterSettings settings)
Comment thread
Mahdigln marked this conversation as resolved.
Outdated
where T : IOpenApiSerializable
{
return element.SerializeAsync(specVersion, OpenApiConstants.Json, settings);
}

/// <summary>
/// Serializes the <see cref="IOpenApiSerializable"/> to the Open API document as a string in YAML format.
/// </summary>
Expand All @@ -175,11 +211,13 @@ public static Task<string> SerializeAsYamlAsync<T>(
/// <param name="specVersion">The Open API specification version.</param>
/// <param name="format">Open API document format.</param>
/// <param name="cancellationToken">The cancellation token.</param>
#pragma warning disable RS0027 // The settings overload below has the same parameter count but different types; no ambiguity exists.
public static async Task<string> SerializeAsync<T>(
this T element,
OpenApiSpecVersion specVersion,
string format,
CancellationToken cancellationToken = default)
#pragma warning restore RS0027
where T : IOpenApiSerializable
{
Utils.CheckArgumentNull(element);
Expand All @@ -195,5 +233,30 @@ public static async Task<string> SerializeAsync<T>(
return await streamReader.ReadToEndAsync().ConfigureAwait(false);
#endif
}

/// <summary>
/// Serializes the <see cref="IOpenApiSerializable"/> to the Open API document as a string in the given format using the given settings.
/// </summary>
/// <typeparam name="T">the <see cref="IOpenApiSerializable"/></typeparam>
/// <param name="element">The Open API element.</param>
/// <param name="specVersion">The Open API specification version.</param>
/// <param name="format">Open API document format.</param>
/// <param name="settings">Provide configuration settings for controlling writing output.</param>
public static async Task<string> SerializeAsync<T>(
this T element,
OpenApiSpecVersion specVersion,
string format,
OpenApiWriterSettings? settings)
Comment thread
Mahdigln marked this conversation as resolved.
Outdated
where T : IOpenApiSerializable
{
Utils.CheckArgumentNull(element);

using var stream = new MemoryStream();
await element.SerializeAsync(stream, specVersion, format, settings, CancellationToken.None).ConfigureAwait(false);
stream.Position = 0;

using var streamReader = new StreamReader(stream);
return await streamReader.ReadToEndAsync().ConfigureAwait(false);
}
}
}
3 changes: 3 additions & 0 deletions src/Microsoft.OpenApi/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
#nullable enable
const Microsoft.OpenApi.OpenApiConstants.JsonSchemaExamplesExtension = "x-jsonschema-examples" -> string!
const Microsoft.OpenApi.OpenApiConstants.OaiLicenseIdentifier = "x-oai-license-identifier" -> string!
static Microsoft.OpenApi.OpenApiSerializableExtensions.SerializeAsJsonAsync<T>(this T element, Microsoft.OpenApi.OpenApiSpecVersion specVersion, Microsoft.OpenApi.OpenApiJsonWriterSettings! settings) -> System.Threading.Tasks.Task<string!>!
static Microsoft.OpenApi.OpenApiSerializableExtensions.SerializeAsJsonAsync<T>(this T element, System.IO.Stream! stream, Microsoft.OpenApi.OpenApiSpecVersion specVersion, Microsoft.OpenApi.OpenApiJsonWriterSettings! settings) -> System.Threading.Tasks.Task!
static Microsoft.OpenApi.OpenApiSerializableExtensions.SerializeAsync<T>(this T element, Microsoft.OpenApi.OpenApiSpecVersion specVersion, string! format, Microsoft.OpenApi.OpenApiWriterSettings? settings) -> System.Threading.Tasks.Task<string!>!
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,60 @@ public async Task UsesTheTerseOutputInformationFromSettingsNoSettings()

Assert.Equal("{\n \"name\": \"param1\",\n \"in\": \"query\",\n \"description\": \"A sample parameter\",\n \"schema\": {\n \"type\": \"string\"\n }\n}", output);
}

[Fact]
public async Task SerializeAsJsonAsync_WithTerseSettings_WritesToStream()
{
var parameter = new OpenApiParameter
{
Name = "param1",
In = ParameterLocation.Query,
Schema = new OpenApiSchema { Type = JsonSchemaType.String }
};

var settings = new OpenApiJsonWriterSettings { Terse = true };

using var stream = new MemoryStream();
await parameter.SerializeAsJsonAsync(stream, OpenApiSpecVersion.OpenApi3_1, settings);

stream.Position = 0;
using var reader = new StreamReader(stream);
var output = await reader.ReadToEndAsync();

Assert.Equal("{\"name\":\"param1\",\"in\":\"query\",\"schema\":{\"type\":\"string\"}}", output);
}

[Fact]
public async Task SerializeAsJsonAsync_WithTerseSettings_ReturnsCompactString()
{
var parameter = new OpenApiParameter
{
Name = "param1",
In = ParameterLocation.Query,
Schema = new OpenApiSchema { Type = JsonSchemaType.String }
};

var settings = new OpenApiJsonWriterSettings { Terse = true };

var output = await parameter.SerializeAsJsonAsync(OpenApiSpecVersion.OpenApi3_1, settings);

Assert.Equal("{\"name\":\"param1\",\"in\":\"query\",\"schema\":{\"type\":\"string\"}}", output);
}

[Fact]
public async Task SerializeAsync_WithSettings_ReturnsFormattedString()
{
var parameter = new OpenApiParameter
{
Name = "param1",
In = ParameterLocation.Query,
Schema = new OpenApiSchema { Type = JsonSchemaType.String }
};

var settings = new OpenApiJsonWriterSettings { Terse = false };

var output = await parameter.SerializeAsync(OpenApiSpecVersion.OpenApi3_1, OpenApiConstants.Json, settings);

Assert.Equal("{\n \"name\": \"param1\",\n \"in\": \"query\",\n \"schema\": {\n \"type\": \"string\"\n }\n}", output);
}
}