Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor readers to reduce surface area #1975

Merged
merged 55 commits into from
Dec 20, 2024
Merged

Refactor readers to reduce surface area #1975

merged 55 commits into from
Dec 20, 2024

Conversation

MaggieKimani1
Copy link
Contributor

@MaggieKimani1 MaggieKimani1 commented Nov 28, 2024

Fixes #1954
fixes #1951
Fixes #1964
Fixes #1917
fixes #1918
closes #1958
closes #1929

@MaggieKimani1 MaggieKimani1 marked this pull request as ready for review December 2, 2024 11:19

Choose a reason for hiding this comment

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

Copilot reviewed 15 out of 30 changed files in this pull request and generated no suggestions.

Files not reviewed (15)
  • test/Microsoft.OpenApi.Readers.Tests/V3Tests/OpenApiCallbackTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V3Tests/OpenApiDocumentTests.cs: Evaluated as low risk
  • src/Microsoft.OpenApi/Interfaces/IOpenApiReader.cs: Evaluated as low risk
  • src/Microsoft.OpenApi/Models/OpenApiDocument.cs: Evaluated as low risk
  • src/Microsoft.OpenApi/Reader/OpenApiJsonReader.cs: Evaluated as low risk
  • src/Microsoft.OpenApi/Reader/OpenApiModelFactory.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V3Tests/OpenApiEncodingTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V3Tests/OpenApiDiscriminatorTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V3Tests/OpenApiExampleTests.cs: Evaluated as low risk
  • src/Microsoft.OpenApi.Readers/OpenApiYamlReader.cs: Evaluated as low risk
  • src/Microsoft.OpenApi.Hidi/OpenApiService.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V31Tests/OpenApiSchemaTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V31Tests/OpenApiDocumentTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Hidi.Tests/Services/OpenApiFilterServiceTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/OpenApiReaderTests/OpenApiDiagnosticTests.cs: Evaluated as low risk
src/Microsoft.OpenApi/Reader/OpenApiModelFactory.cs Dismissed Show dismissed Hide dismissed
src/Microsoft.OpenApi/Reader/OpenApiModelFactory.cs Dismissed Show dismissed Hide dismissed
@baywet baywet requested a review from Copilot December 20, 2024 13:52
Copy link

@Copilot 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 15 out of 30 changed files in this pull request and generated 2 comments.

Files not reviewed (15)
  • test/Microsoft.OpenApi.Readers.Tests/V3Tests/OpenApiDocumentTests.cs: Evaluated as low risk
  • src/Microsoft.OpenApi.Hidi/OpenApiService.cs: Evaluated as low risk
  • src/Microsoft.OpenApi/Interfaces/IOpenApiReader.cs: Evaluated as low risk
  • src/Microsoft.OpenApi/Models/OpenApiDocument.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V3Tests/OpenApiDiscriminatorTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V3Tests/OpenApiEncodingTests.cs: Evaluated as low risk
  • src/Microsoft.OpenApi.Readers/OpenApiYamlReader.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V3Tests/OpenApiCallbackTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V3Tests/OpenApiExampleTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V31Tests/OpenApiSchemaTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V31Tests/OpenApiDocumentTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/V2Tests/OpenApiDocumentTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Hidi.Tests/Services/OpenApiFilterServiceTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/OpenApiReaderTests/OpenApiDiagnosticTests.cs: Evaluated as low risk
  • test/Microsoft.OpenApi.Readers.Tests/OpenApiReaderTests/UnsupportedSpecVersionTests.cs: Evaluated as low risk
Comments suppressed due to low confidence (3)

src/Microsoft.OpenApi/Reader/OpenApiModelFactory.cs:332

  • The method PrepareStreamForReadingAsync might read the entire stream into memory, which could be inefficient for very large streams. Consider adding a check to handle large streams more efficiently.
private static async Task<(Stream, string)> PrepareStreamForReadingAsync(Stream input, string format, CancellationToken token = default)

src/Microsoft.OpenApi/Reader/OpenApiJsonReader.cs:79

  • The method ReadAsync(JsonNode, OpenApiReaderSettings, string, CancellationToken) has been removed. Replace it with Read(JsonNode, OpenApiReaderSettings, string).
return await ReadAsync(jsonNode, settings, cancellationToken: cancellationToken);

src/Microsoft.OpenApi/Reader/OpenApiJsonReader.cs:145

  • Ensure that the external references loading functionality is still covered elsewhere in the codebase, as the LoadExternalRefsAsync method has been removed.
return Read(jsonNode, settings);

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
78.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes!

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

(clicked the wrong button 🤦)

@baywet baywet merged commit 80164af into dev Dec 20, 2024
9 of 12 checks passed
@baywet baywet deleted the mk/fix-json-reader branch December 20, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants