fix: indicate whether the min/max value were provided in the original schema

[brendarearden]

Motivation and Context

A default value is being provided for minimum and maximum when not found in the original schema. While this is intended behavior, there are times when we want to remove these values from the schema when transforming into different types.

To help with this, we added an x-stoplight object to track which key/values are set explicitly in the original schema.

Description

Created an array of property names to track which values are in the schema prior to making changes.

How Has This Been Tested?

Ran existing test to validate that they failed, and then updated to pass. I used yalc to pull into platform-internal to see that the new key value pair is available for use. No new tests were added since this case was covered with existing tests.

Types of changes

• [x] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• [ ] This PR's code follows as closely as possible the coding style/guidelines of this project.
• [ ] I have added error reporting and followed the error reporting guidelines.
• [ ] I have added event tracking and followed the event tracking guidelines.
• [ ] I have updated any relevant documentation accordingly to reflect this PR's changes.
• [ ] I have added automated tests (unit/integration/e2e/other) to cover my changes.
• [ ] All new and existing tests pass locally (excluding flaky CI tests).

[daniel-white]

Would this be considered a breaking change?

[brendarearden]

My argument for it not being a breaking change is that minimum and maximum aren't required values so keeping them from being set when they aren't supposed to shouldn't be breaking. However, its does change how it was function before, so I will mark it as breaking!

[brendarearden]

@P0lip I don't really like the idea of removing the default value based on value matching during oas3 export - so after talking with @daniel-white we thought it might be valuable to indicate if the minimum or maximum were originally in the schema so that consumers of http-spec can make more informed decisions on how to handle min/max. Thoughts on this approach?

This way in oas3 export, I can remove them from the schema if hasMaximumDeclared or hasMinimumDeclared is false

[EdVinyard]

@ryotrellim, you may want to keep an eye on this conversation, since the original request/requirement came from our conversation about integer min/max values in bundled OpenAPI documents.

[P0lip]

Are we trying to fix the following two issues in this PR?

int64 values always include (incorrect) min and max values. After talking with @ryotrellim , these should be omitted if they're the defaults.

int32 values always include min and max values. When they are the default values, they should be omitted.

If so, why don't we just omit the values during the export if the provided ones are the defaults?

Kind of what we do here https://github.com/stoplightio/json-schema-viewer/blob/5e72340ab22e234e0c77edb10fa30aeec267cc45/src/components/shared/Validations.tsx#L73L106. It'd also work for byte, float, double and not just int32/int64

[brendarearden]

@P0lip Yes, those are the two things we are attempting to fix and we can definitely do the same thing we did for json-schema in the oas export.

The main drawbacks I see form continuing to remove the values if they match the default values is:

1. We don't know if they are user provided or if they were defaults added by us. (this is a bit of an edge case but not impossible)
2. It requires us to duplicate the default value calculations everywhere that we need to check them.

With this new pattern, we would be able to check a bool to know if we should omit the minimum and maximum values from the export. this change wouldn't cover the byte scenario as is.

[P0lip]

We don't know if they are user provided or if they were defaults added by us. (this is a bit of an edge case but not impossible)

It shouldn't matter if a user provided it or not. We generally don't track such stuff anywhere and the model is not expected to be a mirror of the initially authored content. What matters the most is whether the schema is semantically the same.

It requires us to duplicate the default value calculations everywhere that we need to check them.

You could export util from http-spec if needed and use it there.

With this new pattern, we would be able to check a bool to know if we should omit the minimum and maximum values from the export.

I understand the use case, but as mentioned - that's not quite the principle http-spec / SL Doc follows. We normalize quite a bit data here and there - insert some default values, remove some, etc. and we don't care whether something was user authored or not.

Either way, if we were really to insert some data, we should probably do

x-stoplight:
  hasMinimumDeclared: number
  hasMaximumDeclared: number

but again, I don't think this is the right way to do that. We should just tweak that in exporter if needed, or just keep those values unchanged (we could perhaps try working around the JS number limitations and author proper int64, but it's a known bug in Studio / our ecosystem. Parts of our world support big ints, but not everything)

[daniel-white]

and we don't care whether something was user authored or not.

as part of our project - we do care what the original author wrote

[brendarearden]

@P0lip since we need this information to make better export decisions, I have added an x-stoplight object to track which values are set explicitly in the original schema. This way its easy to ignore for downstream libraries that do not need this info to ignore. Let me know what you think!

[stoplight-bot]

:tada: This PR is included in version 5.5.3 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: