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

Support parsing schema values exceeding decimal ranges by replacing decimal.Parse with double.Parse #1594

Draft
wants to merge 8 commits into
base: vnext
Choose a base branch
from

Conversation

MaggieKimani1
Copy link
Contributor

@MaggieKimani1 MaggieKimani1 commented Mar 13, 2024

Fixes #1106,
fixes #1265

Copy link

sonarcloud bot commented Mar 13, 2024

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

@@ -470,16 +470,16 @@ public void WriteJsonSchemaWithoutReference(IOpenApiWriter writer, JsonSchema sc
writer.WriteProperty(OpenApiConstants.Title, schema.GetTitle());

// multipleOf
writer.WriteProperty(OpenApiConstants.MultipleOf, schema.GetMultipleOf());
writer.WriteProperty(OpenApiConstants.MultipleOf, schema.GetOpenApiMultipleOf());
Copy link
Member

Choose a reason for hiding this comment

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

Are we forced to prefix with OpenAPI to differentiate from the jsonschema library methods?
Is there any reason why we can't use that library's methods?

Copy link
Contributor Author

@MaggieKimani1 MaggieKimani1 Mar 18, 2024

Choose a reason for hiding this comment

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

Yes prefixing with OpenApi helps to distinguish our custom implementation from the JsonSchema.NET library's.
Also we're overriding the library's methods because the keywords such as MultipleOf, Maximum and Minimum are defined as decimal types and we need to replace that and use double for us to support parsing schema values exceeding decimal's ranges.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to PR upstream instead of maintaining our own code in this case?

@MaggieKimani1 MaggieKimani1 self-assigned this Mar 18, 2024
@MaggieKimani1 MaggieKimani1 modified the milestone: NET:2.0 Mar 18, 2024
Base automatically changed from release/2.0.0 to vnext November 5, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants