Add APIs for OpenAPI document transformers #54935

captainsafia · 2024-04-03T23:26:43Z

This PR adds support for document and operation transformers to OpenAPI feature branch. I've not included schema transformers in this delta since schema-related changes are not in.

Rick-Anderson

I'm trusting src/OpenApi/perf/TransformersBenchmark.cs is self documenting and doesn't need any comments.

src/OpenApi/sample/Transformers/AddContactTransformer.cs

src/OpenApi/sample/Transformers/OperationTransformers.cs

src/OpenApi/src/Services/OpenApiOptions.cs

martincostello · 2024-04-04T06:25:14Z

src/OpenApi/src/Transformers/DelegateOpenApiDocumentTransformer.cs

+    {
+        if (_documentTransformer != null)
+        {
+            await _documentTransformer(document, context, cancellationToken);


Could just return here as both won't be non-null?

src/OpenApi/src/Transformers/DelegateOpenApiDocumentTransformer.cs

src/OpenApi/src/Transformers/OpenApiOperationTransformerContext.cs

captainsafia · 2024-04-04T19:39:31Z

I'm trusting src/OpenApi/perf/TransformersBenchmark.cs is self documenting and doesn't need any comments.

I added a doc string to the benchmark class to describe what it's checking for.

src/OpenApi/src/Transformers/IOpenApiDocumentTransformer.cs

src/OpenApi/src/Transformers/OpenApiOperationTransformerContext.cs

src/OpenApi/src/Transformers/OpenApiDocumentTransformerContext.cs

src/OpenApi/perf/TransformersBenchmark.cs

amcasey · 2024-04-04T21:24:28Z

src/OpenApi/perf/TransformersBenchmark.cs

+        _documentService = CreateDocumentService(_builder, _options);
+    }
+
+    [Benchmark]


What were the results? Are the comparable?

Example results are below. The OperationTransformerAsDelegate is more allocate-y than the others because of the use of the enumerator over the OpenApiOperations dictionary. We could optimize this but it makes the code a little less easy to read so I avoided this for now.

Method InvocationCount Mean Error StdDev Op/s Gen0 Gen1 Gen2 Allocated

OperationTransformerAsDelegate 10 502.24 us 2.784 us 2.604 us 1,991.07 3.9063 0.9766 0.9766 436.21 KB

ActivatedDocumentTransformer 10 50.65 us 0.510 us 0.477 us 19,744.86 0.3052 0.1221 0.0610 44.72 KB

DocumentTransformerAsDelegate 10 112.61 us 0.952 us 0.891 us 8,879.85 0.3662 0.2441 0.1221 44.72 KB

OperationTransformerAsDelegate 100 5,154.06 us 99.184 us 92.777 us 194.02 31.2500 - - 4282.05 KB

ActivatedDocumentTransformer 100 485.27 us 3.190 us 2.984 us 2,060.69 2.9297 0.9766 - 367.19 KB

DocumentTransformerAsDelegate 100 1,118.66 us 4.202 us 3.725 us 893.93 1.9531 - - 367.19 KB

OperationTransformerAsDelegate 1000 49,849.30 us 358.614 us 299.459 us 20.06 272.7273 - - 42820.56 KB

ActivatedDocumentTransformer 1000 4,973.16 us 61.507 us 57.534 us 201.08 23.4375 7.8125 - 3671.91 KB

DocumentTransformerAsDelegate 1000 11,212.38 us 81.451 us 76.189 us 89.19 15.6250 - - 3671.91 KB

Don't we expect that one to be widely used? I thought the whole point of exposing the helper was so that app developers wouldn't have to roll their own. I think it might be worth a reduction in readability in this case.

I included the optimization in 40a9912. Description of it is here.

Original (with Dictionary enumerator)

Method TransformerCount Mean Error StdDev Op/s Gen0 Gen1 Allocated

OperationTransformerAsDelegate 10 2.156 us 0.0176 us 0.0147 us 463,813.7 0.0343 0.0076 4.15 KB

ActivatedDocumentTransformer 10 1.686 us 0.0216 us 0.0202 us 592,946.5 0.0324 0.0076 3.93 KB

DocumentTransformerAsDelegate 10 1.661 us 0.0177 us 0.0157 us 601,973.3 0.0305 0.0076 3.67 KB

OperationTransformerAsDelegate 100 7.310 us 0.0464 us 0.0387 us 136,796.5 0.0610 0.0153 7.66 KB

ActivatedDocumentTransformer 100 3.801 us 0.0311 us 0.0276 us 263,095.2 0.0496 0.0114 6.04 KB

DocumentTransformerAsDelegate 100 2.838 us 0.0337 us 0.0315 us 352,392.1 0.0305 0.0076 3.67 KB

OperationTransformerAsDelegate 1000 53.321 us 0.1793 us 0.1590 us 18,754.3 0.3052 0.0610 42.82 KB

ActivatedDocumentTransformer 1000 32.117 us 0.6423 us 0.6872 us 31,136.6 0.1831 - 28.13 KB

DocumentTransformerAsDelegate 1000 12.615 us 0.0257 us 0.0201 us 79,269.2 0.0153 - 3.67 KB

Improved (with OperationType-lookup)

Method TransformerCount Mean Error StdDev Op/s Gen0 Gen1 Allocated

OperationTransformerAsDelegate 10 2.561 us 0.0293 us 0.0274 us 390,501.5 0.0305 0.0076 3.7 KB

ActivatedDocumentTransformer 10 1.686 us 0.0108 us 0.0096 us 593,113.3 0.0324 0.0076 3.93 KB

DocumentTransformerAsDelegate 10 1.794 us 0.0134 us 0.0119 us 557,380.3 0.0305 0.0076 3.67 KB

OperationTransformerAsDelegate 100 8.255 us 0.1338 us 0.1252 us 121,144.2 0.0153 - 3.7 KB

ActivatedDocumentTransformer 100 4.016 us 0.0770 us 0.0824 us 248,976.9 0.0458 0.0076 6.04 KB

DocumentTransformerAsDelegate 100 2.762 us 0.0455 us 0.0426 us 362,068.0 0.0267 0.0076 3.67 KB

OperationTransformerAsDelegate 1000 60.630 us 0.1782 us 0.1667 us 16,493.6 - - 3.7 KB

ActivatedDocumentTransformer 1000 35.310 us 0.5062 us 0.4735 us 28,320.9 0.1831 - 27.13 KB

DocumentTransformerAsDelegate 1000 12.969 us 0.0904 us 0.0755 us 77,106.4 0.0153 - 3.67 KB

src/OpenApi/sample/Transformers/AddBearerSecuritySchemeTransformer.cs

src/OpenApi/src/Services/OpenApiDocumentService.cs

src/OpenApi/src/Transformers/ActivatedOpenApiDocumentTransformer.cs

amcasey · 2024-04-04T21:44:31Z

src/OpenApi/src/Transformers/ActivatedOpenApiDocumentTransformer.cs

+
+    public Task TransformAsync(OpenApiDocument document, OpenApiDocumentTransformerContext context, CancellationToken cancellationToken)
+    {
+        Debug.Assert(Transformer != null, "Transformer should have been initialized.");


Why is Initialize a separate method? Is TransformAsync expected to be called much more often?

Since we use null-coalescing to set the Transformer we could initialize it here but I liked the clarity that separating it out into separate calls provided. The fact that this transformer type needs to do some additional work at invocation to activate dependencies from the DI container felt special enough to call out in its own method.

I don't get it. Is it important that that happen after construction but before TransformAsync?

Actually, following @BrennanConroy's feedback here, the Initialize method isn't the right way to go since we want scoped/transient services to be resolved again for each request/invocation to the document so this needs to be worked. Done in 73fc68a (#54935).

Do we care about disposable transient transformers?
class MyTransformer : IDisposable, IOpenApiDocumentTransformer

Yes, see b7ad201.

src/OpenApi/src/Transformers/DelegateOpenApiDocumentTransformer.cs

src/OpenApi/test/Transformers/OpenApiOptionsTests.cs

BrennanConroy · 2024-04-04T23:02:43Z

src/OpenApi/perf/TransformersBenchmark.cs

+    [Benchmark]
+    public async Task ActivatedDocumentTransformer()
+    {
+        for (var i = 0; i < InvocationCount; i++)


This is an odd pattern, why are you doing it? The results for different invocation counts should just be the same relative to each other.

If you think an individual operation is too fast you can have a hardcoded invocation count and use [Benchmark(OperationsPerInvoke = 1000)]

aspnetcore/src/Servers/Kestrel/perf/Microbenchmarks/HttpParserBenchmark.cs

Line 42 in 0b00a2d

[Benchmark(OperationsPerInvoke = RequestParsingData.InnerLoopCount)]

What if one's quadratic? 😝

BrennanConroy · 2024-04-04T23:09:29Z

src/OpenApi/src/Extensions/OpenApiEndpointRouteBuilderExtensions.cs

@@ -43,7 +43,7 @@ public static IEndpointConventionBuilder MapOpenApi(this IEndpointRouteBuilder e
                }
                else
                {
-                    var document = await documentService.GetOpenApiDocumentAsync();
+                    var document = await documentService.GetOpenApiDocumentAsync(context.RequestAborted);
                    var documentOptions = options.Get(documentName);
                    using var output = MemoryBufferWriter.Get();
                    using var writer = Utf8BufferTextWriter.Get(output);


Not part of this PR, but on line 53 you call output.ToArray() which seems wasteful, was BodyWriter.CopyFromAsync(output) evaluated?

src/OpenApi/src/Services/OpenApiDocumentService.cs

src/OpenApi/test/Services/OpenApiDocumentServiceTestsBase.cs

src/OpenApi/test/Transformers/DocumentTransformerTests.cs

BrennanConroy · 2024-04-05T22:04:06Z

src/OpenApi/src/Services/OpenApiDocumentService.cs

+            }
+            finally
+            {
+                if (transformer is IDisposable disposable)


Aren't these transformers effectively singletons because they come from the options object? So disposing them every use would be odd?

BrennanConroy · 2024-04-05T22:09:12Z

src/OpenApi/src/Transformers/TypeBasedOpenApiDocumentTransformer.cs

+
+    public Task TransformAsync(OpenApiDocument document, OpenApiDocumentTransformerContext context, CancellationToken cancellationToken)
+    {
+        _transformer = _transformerFactory.Invoke(context.ApplicationServices, []) as IOpenApiDocumentTransformer;


This class can be hit in parallel by parallel requests, so the instance in the _transformer field isn't going to necessarily be preserved by the time DisposeAsync is called. You could just do the dispose inline and get rid of _transformer.

amcasey

LGTM. Thanks for the changes!

src/OpenApi/src/Services/OpenApiDocumentService.cs

Co-authored-by: Martin Costello <[email protected]> Co-authored-by: Rick Anderson <[email protected]> This PR adds support for OpenAPI document generation, sans schema generation to Microsoft.AspNetCore.OpenApi. Relevant changes are available in individual PRs: - Add entry-point APIs for OpenAPI support (#54789) - Support resolving OpenApiPaths entries from document (#54847) - Support generating OpenAPI operation and associated fields (#54903) - Add APIs for OpenAPI document transformers (#54935) - Add support for generating OpenAPI parameters (#55041) - Add support for generating OpenAPI responses (#55020) - Add support for generating OpenAPI request bodies (#55040)

Add APIs for OpenAPI document transformers

7e36fe3

captainsafia requested review from a team and wtgodbe as code owners April 3, 2024 23:26

ghost added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Apr 3, 2024

captainsafia requested review from tdykstra, Rick-Anderson, BrennanConroy and amcasey April 3, 2024 23:27

Rick-Anderson reviewed Apr 4, 2024

View reviewed changes

martincostello reviewed Apr 4, 2024

View reviewed changes

Address feedback

da22fdd

tdykstra reviewed Apr 4, 2024

View reviewed changes

src/OpenApi/src/Transformers/IOpenApiDocumentTransformer.cs Outdated Show resolved Hide resolved

src/OpenApi/src/Transformers/OpenApiOperationTransformerContext.cs Outdated Show resolved Hide resolved

src/OpenApi/src/Transformers/OpenApiDocumentTransformerContext.cs Show resolved Hide resolved

martincostello approved these changes Apr 4, 2024

View reviewed changes

Update doc comments

6d818a5

amcasey reviewed Apr 4, 2024

View reviewed changes

BrennanConroy reviewed Apr 4, 2024

View reviewed changes

captainsafia added 4 commits April 4, 2024 17:11

Address more feedback

6bf3186

Handle service lifetimes correctly in activated transformers

73fc68a

Handle disposable transformers

b7ad201

Benchmark on transformer count

604498c

BrennanConroy reviewed Apr 5, 2024

View reviewed changes

captainsafia added 2 commits April 5, 2024 15:48

Fix up disposable code and improve paths perf

40a9912

Remove unused imports in SharedTypes for RDG tests

a76b037

captainsafia requested a review from halter73 as a code owner April 5, 2024 23:34

amcasey approved these changes Apr 6, 2024

View reviewed changes

src/OpenApi/src/Services/OpenApiDocumentService.cs Outdated Show resolved Hide resolved

Clean up benchmarks and TryGetCachedOperationTransformerContext

578af9f

captainsafia merged commit b341f8c into feature/openapi Apr 8, 2024

captainsafia deleted the safia/openapi-transforms branch April 8, 2024 20:27

Method	InvocationCount	Mean	Error	StdDev	Op/s	Gen0	Gen1	Gen2	Allocated
OperationTransformerAsDelegate	10	502.24 us	2.784 us	2.604 us	1,991.07	3.9063	0.9766	0.9766	436.21 KB
ActivatedDocumentTransformer	10	50.65 us	0.510 us	0.477 us	19,744.86	0.3052	0.1221	0.0610	44.72 KB
DocumentTransformerAsDelegate	10	112.61 us	0.952 us	0.891 us	8,879.85	0.3662	0.2441	0.1221	44.72 KB
OperationTransformerAsDelegate	100	5,154.06 us	99.184 us	92.777 us	194.02	31.2500	-	-	4282.05 KB
ActivatedDocumentTransformer	100	485.27 us	3.190 us	2.984 us	2,060.69	2.9297	0.9766	-	367.19 KB
DocumentTransformerAsDelegate	100	1,118.66 us	4.202 us	3.725 us	893.93	1.9531	-	-	367.19 KB
OperationTransformerAsDelegate	1000	49,849.30 us	358.614 us	299.459 us	20.06	272.7273	-	-	42820.56 KB
ActivatedDocumentTransformer	1000	4,973.16 us	61.507 us	57.534 us	201.08	23.4375	7.8125	-	3671.91 KB
DocumentTransformerAsDelegate	1000	11,212.38 us	81.451 us	76.189 us	89.19	15.6250	-	-	3671.91 KB

Method	TransformerCount	Mean	Error	StdDev	Op/s	Gen0	Gen1	Allocated
OperationTransformerAsDelegate	10	2.156 us	0.0176 us	0.0147 us	463,813.7	0.0343	0.0076	4.15 KB
ActivatedDocumentTransformer	10	1.686 us	0.0216 us	0.0202 us	592,946.5	0.0324	0.0076	3.93 KB
DocumentTransformerAsDelegate	10	1.661 us	0.0177 us	0.0157 us	601,973.3	0.0305	0.0076	3.67 KB
OperationTransformerAsDelegate	100	7.310 us	0.0464 us	0.0387 us	136,796.5	0.0610	0.0153	7.66 KB
ActivatedDocumentTransformer	100	3.801 us	0.0311 us	0.0276 us	263,095.2	0.0496	0.0114	6.04 KB
DocumentTransformerAsDelegate	100	2.838 us	0.0337 us	0.0315 us	352,392.1	0.0305	0.0076	3.67 KB
OperationTransformerAsDelegate	1000	53.321 us	0.1793 us	0.1590 us	18,754.3	0.3052	0.0610	42.82 KB
ActivatedDocumentTransformer	1000	32.117 us	0.6423 us	0.6872 us	31,136.6	0.1831	-	28.13 KB
DocumentTransformerAsDelegate	1000	12.615 us	0.0257 us	0.0201 us	79,269.2	0.0153	-	3.67 KB

Method	TransformerCount	Mean	Error	StdDev	Op/s	Gen0	Gen1	Allocated
OperationTransformerAsDelegate	10	2.561 us	0.0293 us	0.0274 us	390,501.5	0.0305	0.0076	3.7 KB
ActivatedDocumentTransformer	10	1.686 us	0.0108 us	0.0096 us	593,113.3	0.0324	0.0076	3.93 KB
DocumentTransformerAsDelegate	10	1.794 us	0.0134 us	0.0119 us	557,380.3	0.0305	0.0076	3.67 KB
OperationTransformerAsDelegate	100	8.255 us	0.1338 us	0.1252 us	121,144.2	0.0153	-	3.7 KB
ActivatedDocumentTransformer	100	4.016 us	0.0770 us	0.0824 us	248,976.9	0.0458	0.0076	6.04 KB
DocumentTransformerAsDelegate	100	2.762 us	0.0455 us	0.0426 us	362,068.0	0.0267	0.0076	3.67 KB
OperationTransformerAsDelegate	1000	60.630 us	0.1782 us	0.1667 us	16,493.6	-	-	3.7 KB
ActivatedDocumentTransformer	1000	35.310 us	0.5062 us	0.4735 us	28,320.9	0.1831	-	27.13 KB
DocumentTransformerAsDelegate	1000	12.969 us	0.0904 us	0.0755 us	77,106.4	0.0153	-	3.67 KB

Add APIs for OpenAPI document transformers #54935

Add APIs for OpenAPI document transformers #54935

Uh oh!

Conversation

captainsafia commented Apr 3, 2024

Uh oh!

Rick-Anderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

captainsafia commented Apr 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!