[OpenAPI] Various fixes#8920
Conversation
tobias-tengler
force-pushed
the
tte/openapi-fixes
branch
from
8f01dd0 to
edb3f81
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8920 +/- ##
==========================================
- Coverage 74.16% 0 -74.17%
==========================================
Files 2677 0 -2677
Lines 140790 0 -140790
Branches 16371 0 -16371
==========================================
- Hits 104421 0 -104421
+ Misses 30774 0 -30774
+ Partials 5595 0 -5595
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements various fixes for the OpenAPI adapter functionality in HotChocolate, focusing on JSON parsing, service registration, test infrastructure, and schema generation.
Key Changes:
- Fixed JSON array parsing to avoid double-reading of tokens by adding a
skipReadingparameter - Refactored test schema into a separate
TestSchema.csfile for better organization and reusability - Enhanced OpenAPI document generation to support introspection fields (
__typename) and handle cases where fields don't have selection sets
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| JsonValueParser.cs | Adds skipReading parameter to prevent double-reading JSON tokens during array parsing |
| CoreFusionGatewayBuilderExtensions.Diagnostics.cs | Moves service registrations into schema service configuration scope |
| TestSchema.cs | New shared test schema file with comprehensive test scenarios (unions, interfaces, complex types) |
| TestHelpers.cs | Refactored to reference external TestSchema instead of inline schema definitions |
| OpenApiTestBase.cs | Adds test queries for union, interface, and complex object scenarios |
| OpenApiIntegrationTestBase.cs | Adds snapshot suffix support for better test organization |
| FusionOpenApiIntegrationTests.cs | Implements snapshot suffix to differentiate Fusion-specific snapshots |
| HttpEndpointIntegrationTestBase.cs | Adds edge case tests for empty lists, empty objects, and complex objects |
| OpenApiDocumentManager.cs | Updates document update logic to handle missing documents |
| DynamicOpenApiDocumentTransformer.cs | Adds support for introspection fields and handles fields without selection sets |
| Snapshot files (*.json, *.snap) | Generated test output files for OpenAPI documents and HTTP responses |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| builder.ConfigureSchemaServices(static (_, s) => | ||
| s.AddSingleton(static sp => (IFusionExecutionDiagnosticEventListener)sp.GetRequiredService<T>())); | ||
| { | ||
| s.TryAddSingleton<T>(); |
There was a problem hiding this comment.
The service registration has been moved from the outer scope (line 32 removed) into the ConfigureSchemaServices callback (line 34 added). However, this changes the service lifetime scope. Services registered in ConfigureSchemaServices are scoped to the schema, whereas services previously registered in builder.Services were scoped to the application. This could cause issues if the service T is expected to be a singleton at the application level or if it's shared across multiple schemas. Please verify this behavior change is intentional.
| builder.ConfigureSchemaServices(static (_, s) => | ||
| { | ||
| var attribute = typeof(T).GetCustomAttributes(typeof(DiagnosticEventSourceAttribute), true).First(); | ||
| var listener = ((DiagnosticEventSourceAttribute)attribute).Listener; | ||
|
|
||
| s.TryAddSingleton<T>(); |
There was a problem hiding this comment.
Similar to the issue above, the service registration has been moved from the outer scope into the ConfigureSchemaServices callback (line 45 added). This changes the service lifetime scope from application-level to schema-level. This change should be consistent with the behavior change at line 34, but please verify this is the intended behavior.
| events, | ||
| cancellationToken).ConfigureAwait(false); | ||
|
|
||
| await AddDocumentAsync(document, schema, events, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
The logic in the fragment update block appears incorrect. After successfully updating the fragments collection (line 330), the code calls AddDocumentAsync (line 339) instead of continuing with the updated state. This will attempt to add the document again rather than using the updated fragments.
The correct approach would be to mirror the operation update logic - after updating the fragments, it should not call AddDocumentAsync again. Line 339 should likely be removed.
| await AddDocumentAsync(document, schema, events, cancellationToken).ConfigureAwait(false); | |
| // Removed redundant AddDocumentAsync call; fragment update now mirrors operation update logic. |
| var fieldType = fieldName == IntrospectionFieldNames.TypeName ? | ||
| new NonNullType(schemaDefinition.Types["String"]) : | ||
| ((IComplexTypeDefinition)namedType).Fields[fieldName].Type; |
There was a problem hiding this comment.
The ternary operator check for the __typename introspection field could be more robust. Consider extracting this logic into a helper method for better readability and maintainability. Additionally, the introspection field name should use a constant from IntrospectionFieldNames which is already being referenced, making the code more maintainable if introspection field names change.
| var fieldType = fieldName == IntrospectionFieldNames.TypeName ? | |
| new NonNullType(schemaDefinition.Types["String"]) : | |
| ((IComplexTypeDefinition)namedType).Fields[fieldName].Type; | |
| var fieldType = GetFieldTypeForSelection(fieldName, namedType, schemaDefinition); |
| public sealed class TestSchema | ||
| { |
There was a problem hiding this comment.
The TestSchema class lacks XML documentation comments. Since this is a shared test schema used across multiple test files, adding documentation would help other developers understand its purpose and the various test scenarios it supports (e.g., unions, interfaces, complex objects, introspection, etc.).
| public sealed class TestSchema | |
| { | |
| /// <summary> | |
| /// Shared test schema used across multiple test files to support a variety of test scenarios, | |
| /// including unions, interfaces, complex objects, introspection, authorization, and more. | |
| /// This class provides representative types and queries for validating OpenAPI adapter functionality. | |
| /// </summary> | |
| public sealed class TestSchema |
🚀 Fusion Gateway Performance ResultsSimple Composite QueryConstant Load (50 VUs)
📊 Response Time Metrics
Ramping Load (0→50→500→50 VUs)
📊 Response Time Metrics
Executed Query fragment User on User {
id
username
name
}
fragment Review on Review {
id
body
}
fragment Product on Product {
inStock
name
price
shippingEstimate
upc
weight
}
query TestQuery {
topProducts(first: 5) {
...Product
reviews {
...Review
author {
...User
}
}
}
}Deep Recursion QueryConstant Load (50 VUs)
📊 Response Time Metrics
Ramping Load (0→50→500→50 VUs)
📊 Response Time Metrics
Executed Query fragment User on User {
id
username
name
}
fragment Review on Review {
id
body
}
fragment Product on Product {
inStock
name
price
shippingEstimate
upc
weight
}
query TestQuery {
users {
...User
reviews {
...Review
product {
...Product
reviews {
...Review
author {
...User
reviews {
...Review
product {
...Product
}
}
}
}
}
}
}
topProducts(first: 5) {
...Product
reviews {
...Review
author {
...User
reviews {
...Review
product {
...Product
}
}
}
}
}
}Variable Batching ThroughputConstant Load (50 VUs)
📊 Response Time Metrics
Ramping Load (0→50→500→50 VUs)
📊 Response Time Metrics
Executed Query query TestQuery_8f7a46ce_2(
$__fusion_1_upc: ID!
$__fusion_2_price: Long!
$__fusion_2_weight: Long!
) {
productByUpc(upc: $__fusion_1_upc) {
inStock
shippingEstimate(weight: $__fusion_2_weight, price: $__fusion_2_price)
}
}Variables (5 sets batched in single request) [
{ "__fusion_1_upc": "1", "__fusion_2_price": 899, "__fusion_2_weight": 100 },
{ "__fusion_1_upc": "2", "__fusion_2_price": 1299, "__fusion_2_weight": 1000 },
{ "__fusion_1_upc": "3", "__fusion_2_price": 15, "__fusion_2_weight": 20 },
{ "__fusion_1_upc": "4", "__fusion_2_price": 499, "__fusion_2_weight": 100 },
{ "__fusion_1_upc": "5", "__fusion_2_price": 1299, "__fusion_2_weight": 1000 }
]No baseline data available for comparison. Run 19463961747 • Commit a1554b8 • Tue, 18 Nov 2025 11:27:10 GMT |
No description provided.