yannh / kubernetes-json-schema

JSON Schemas for every version of every object in every version of Kubernetes
Other
341 stars 51 forks source link

Invalid definition for quantity type #11

Open mik-laj opened 2 years ago

mik-laj commented 2 years ago

Hi.

It seems to me that the definition of Quantity type is incorrect. https://github.com/yannh/kubernetes-json-schema/blob/8f0b46a91da2412c58a8a1afa55cf8377597750c/v1.21.0/quantity.json#L1-L12 The fields oneOf and type are mutually exclusive. The field type is redundant in this context.

The standalone specification defines the type correctly when a quantity type is used.

https://github.com/yannh/kubernetes-json-schema/blob/8f0b46a91da2412c58a8a1afa55cf8377597750c/v1.21.0-standalone-strict/resourcequotaspec.json#L5-L20

eyarz commented 2 years ago

This JSON schema is auto-generated from the OpenAPI specifications (swagger.json) for each particular Kubernetes version. Therefore, I think there is no reason to fix it here, because it should be fixed in the origin swagger file.

yannh commented 2 years ago

Hello! I'm not sure, both are generated from the same file, right now I don't know why the standalone and non-standalone would be different :thinking:

mik-laj commented 2 years ago

@eyarz Swagger.json describes this type as follows:

    "io.k8s.apimachinery.pkg.api.resource.Quantity": {
      "description": "Quantity is a fixed-point representation of a number. It provides convenient marshaling/unmarshaling in JSON and YAML, in addition to String() and AsInt64() accessors.\n\nThe serialization format is:\n\n<quantity>        ::= <signedNumber><suffix>\n  (Note that <suffix> may be empty, from the \"\" case in <decimalSI>.)\n<digit>           ::= 0 | 1 | ... | 9 <digits>          ::= <digit> | <digit><digits> <number>          ::= <digits> | <digits>.<digits> | <digits>. | .<digits> <sign>            ::= \"+\" | \"-\" <signedNumber>    ::= <number> | <sign><number> <suffix>          ::= <binarySI> | <decimalExponent> | <decimalSI> <binarySI>        ::= Ki | Mi | Gi | Ti | Pi | Ei\n  (International System of units; See: http://physics.nist.gov/cuu/Units/binary.html)\n<decimalSI>       ::= m | \"\" | k | M | G | T | P | E\n  (Note that 1024 = 1Ki but 1000 = 1k; I didn't choose the capitalization.)\n<decimalExponent> ::= \"e\" <signedNumber> | \"E\" <signedNumber>\n\nNo matter which of the three exponent forms is used, no quantity may represent a number greater than 2^63-1 in magnitude, nor may it have more than 3 decimal places. Numbers larger or more precise will be capped or rounded up. (E.g.: 0.1m will rounded up to 1m.) This may be extended in the future if we require larger or smaller quantities.\n\nWhen a Quantity is parsed from a string, it will remember the type of suffix it had, and will use the same type again when it is serialized.\n\nBefore serializing, Quantity will be put in \"canonical form\". This means that Exponent/suffix will be adjusted up or down (with a corresponding increase or decrease in Mantissa) such that:\n  a. No precision is lost\n  b. No fractional digits will be emitted\n  c. The exponent (or suffix) is as large as possible.\nThe sign will be omitted unless the number is negative.\n\nExamples:\n  1.5 will be serialized as \"1500m\"\n  1.5Gi will be serialized as \"1536Mi\"\n\nNote that the quantity will NEVER be internally represented by a floating point number. That is the whole point of this exercise.\n\nNon-canonical values will still parse as long as they are well formed, but will be re-emitted in their canonical form. (So always use canonical form, or don't diff.)\n\nThis format is intended to make it difficult to use these numbers without writing some sort of special handling code in the hopes that that will cause implementors to also use a fixed point implementation.",
      "type": "string"
    },

The oneOf field is not used here, so this problem does not occur. I guess probably some post transformer that relaxes the specs to allow numbers as text or native value modifies those specs. Therefore, we cannot fix this problem in the swagger.json file.

@yannh This looks like a bug in the splitting process because it adds a type field that is not present in the previous step.

mik-laj commented 2 years ago

@yannh See: https://github.com/yannh/openapi2jsonschema/blob/09bbcef0ed5f0f70ee033834637e10e7035b6787/openapi2jsonschema/command.py#L136

A type field should only be added if it has not been defined in oneOf fields. Now we have a self-contradictory type. The object cannot be both an object and text, or object and null(null != object and string != object).

yannh commented 2 years ago

If you have a fix that would be much appreciated :bow: This script desperately needs more tests :(

mik-laj commented 2 years ago

@yannh Unfortunately I cannot contribute to some OSS projects. 😿

NBardelot commented 2 months ago

Hi. I Ended here after bumping into the same issue.

Maybe a quick workaround would be to specifically patch the resulting quantity.json file after it has been generated in order to remove the "type": "object", since for the moment it is the only one causing a headache.