thim81 / openapi-format

Format an OpenAPI document by ordering, formatting and filtering fields.
MIT License
79 stars 14 forks source link

Fixing large number transformation #41

Closed DanielContrerasAladro closed 2 years ago

DanielContrerasAladro commented 2 years ago

Fixing large number transformation like

components: schemas: NumberLimitsSchema: type: number maximum: 9999999999999.99999 minimum: 0 multipleOf: 0.0000000001

  to 

components: schemas: NumberLimitsSchema: type: number maximum: 10000000000000 minimum: 0 multipleOf: 1e-10

allowing to parse to a readable data instead of rounded or scientific notation

thim81 commented 2 years ago

@DanielContrerasAladro Thank you very much for the contribution.

The PR looks really good, I wanted to understand when (what use-case) the number transformation would happen?

The example above is the result after the openapi-format, what would be the initial state of the "maximum" & "minimum" number? That way I can create a test for it to prevent any future regression from happening.

DanielContrerasAladro commented 2 years ago

Because I need to see numbers like that on the output

components: schemas: NumberLimitsSchema: type: number maximum: 9999999999999.99999 minimum: 0 multipleOf: 0.0000000001

thim81 commented 2 years ago

So the steps to reproduce would be:

Is that correct?

DanielContrerasAladro commented 2 years ago

Exactly, it's exactly the case. That's the reason I've created the '-b' flag to activate this fix on the CLI

thim81 commented 2 years ago

If the reproduce behaviour is correct, than I would not make it an CLI option but default behaviour, since it seems like an unwanted number conversion done by openapi-format.

Would you agree that this should be default?

DanielContrerasAladro commented 2 years ago

Perfect for me

thim81 commented 2 years ago

Would it be OK if I keep only the 2 parts:

fileContent = fileContent.replace( /\b([0-9]*\.?[0-9]+)\b/g, ( number ) => {
      return `'${ number }'`;
    } );
o = o.replace( /'([0-9]*\.?[0-9]+)'/g, ( number ) => {
      return number.replace( /'/g, '' );
    } );

Would you prefer that you modify your PR or that I patch it in a new PR?

DanielContrerasAladro commented 2 years ago

I've just removed

thim81 commented 2 years ago

@DanielContrerasAladro Thanks again for the contribution. I'm going to merge it in and create an additional test for it so that I can release it this week.

DanielContrerasAladro commented 2 years ago

You're welcome!

thim81 commented 2 years ago

@DanielContrerasAladro After running some tests, is it OK that openapi-format target specifically the "minimum" & "maximum" properties? Or would this unwanted conversion also would happen on other properties?

The test suite was complaining about unwanted conversions. So I might tweak your regex or target specific properties.

DanielContrerasAladro commented 2 years ago

It would happen on other properties too, 'multipleOf' for example....and could happen on more other properties

thim81 commented 2 years ago

FYI: I reported the issue with the YAML @stoplight/yaml package, which we are using behind the scenes: https://github.com/stoplightio/yaml/issues/53

thim81 commented 2 years ago

@DanielContrerasAladro I might have to revert the PR, since the impact is quite big as currently all number values are being converted, which is not desired and can cause for invalid OpenAPI documents.

I'll continue this week to figure out a safe way to handle these large numbers.