Skip to content

Conversation

@captainsafia
Copy link
Member

Contributes towards #58619.

  • Remove nullable keyword from schema and support null as a value in the type property
  • Remove type overrides for primitives types in ApplyPrimitiveTypesAndFormats
  • Replace .Reference with OpenApiSchemaReference definitions
  • Update type signatures to match introduction of interfaces to API

@captainsafia captainsafia requested review from a team and wtgodbe as code owners February 8, 2025 01:54
@ghost 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 Feb 8, 2025
@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Feb 8, 2025
Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

Most of this is over my head, but I left a few questions, mostly to help me understand a bit better what this code is doing.

@danmoseley danmoseley requested a review from Copilot February 12, 2025 19:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 23 changed files in this pull request and generated no comments.

Files not reviewed (15)
  • eng/Versions.props: Language not supported
  • src/OpenApi/src/Services/OpenApiGenerator.cs: Evaluated as low risk
  • src/OpenApi/src/Extensions/OpenApiDocumentExtensions.cs: Evaluated as low risk
  • src/OpenApi/sample/Transformers/AddBearerSecuritySchemeTransformer.cs: Evaluated as low risk
  • src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiSchemaService/OpenApiSchemaService.PolymorphicSchemas.cs: Evaluated as low risk
  • src/OpenApi/src/Services/OpenApiDocumentService.cs: Evaluated as low risk
  • src/OpenApi/src/Services/OpenApiConstants.cs: Evaluated as low risk
  • src/OpenApi/src/Schemas/OpenApiSchemaKeywords.cs: Evaluated as low risk
  • src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiDocumentService/OpenApiDocumentServiceTests.Operations.cs: Evaluated as low risk
  • src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiRouteHandlerBuilderExtensionTests.cs: Evaluated as low risk
  • src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiDocumentService/OpenApiDocumentServiceTests.Responses.cs: Evaluated as low risk
  • src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiSchemaService/OpenApiSchemaService.RequestBodySchemas.cs: Evaluated as low risk
  • src/OpenApi/src/Schemas/OpenApiJsonSchema.Helpers.cs: Evaluated as low risk
  • src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/CreateSchemaReferenceIdTests.cs: Evaluated as low risk
  • src/OpenApi/sample/Program.cs: Evaluated as low risk
Comments suppressed due to low confidence (4)

src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiDocumentServiceTestsBase.cs:89

  • The OpenApiSchemaService should be initialized with IOpenApiSchema instead of OpenApiSchema.
var schemaService = new OpenApiSchemaService("Test", Options.Create(new Microsoft.AspNetCore.Http.Json.JsonOptions()), openApiOptions.Object);

src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiDocumentServiceTestsBase.cs:137

  • [nitpick] The variable name jsonOptions is ambiguous. It should be renamed to defaultJsonOptions for consistency.
var jsonOptions = builder.ServiceProvider.GetService<IOptions<Microsoft.AspNetCore.Http.Json.JsonOptions>>() ?? Options.Create(defaultJsonOptions);

src/OpenApi/src/Services/Schemas/OpenApiSchemaService.cs:204

  • Ensure that the schemaId is present before proceeding. Add a check for schema.Annotations.TryGetValue(OpenApiConstants.SchemaId, out var schemaId) after the ContainsKey check.
if (schema.Annotations.ContainsKey(OpenApiConstants.RefId) &&

src/OpenApi/src/Services/Schemas/OpenApiSchemaService.cs:263

  • Ensure that the inputSchema is correctly cast to OpenApiSchema or OpenApiSchemaReference before proceeding. This would prevent any potential runtime errors.
var schema = inputSchema is OpenApiSchemaReference schemaReference

@captainsafia
Copy link
Member Author

@BrennanConroy @DeagleGross Can I get a review on this? I'd love to merge it in before CC on Friday.

@captainsafia captainsafia merged commit 58ac2f7 into main Feb 14, 2025
27 checks passed
@captainsafia captainsafia deleted the safia/openapi-preview7 branch February 14, 2025 23:10
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview2 milestone Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants