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

[REQ] Add ability to differentiate between integer and int32 schemas #9447

Closed
spacether opened this issue May 11, 2021 · 7 comments · Fixed by #9519
Closed

[REQ] Add ability to differentiate between integer and int32 schemas #9447

spacether opened this issue May 11, 2021 · 7 comments · Fixed by #9519
Labels
Enhancement: Feature Good First Issue An issue that could be fixed with a simple PR

Comments

@spacether
Copy link
Contributor

spacether commented May 11, 2021

Is your feature request related to a problem? Please describe.

Our current java code cannot differentiate between these two use cases:

# int32 integer
type: integer
format: int32

# integer of unbounded size
type: integer

in classes that implement IJsonSchemaValidationProperties.

  • codegenProperty
  • codegenModel
  • codegenParameter
  • codegenResponse

Both schemas define this property as codegenProperty.isInteger
so we have no way of differentiating between the two above use cases.

Describe the solution you'd like

I would like two booleans to describe each of these use cases:

  • isUnboundedInteger
  • isShortInteger
    We can keep the existing isInteger property for existing generators.
    This way the addition will be a non-breaking change.

Describe alternatives you've considered

One could use isInteger as isUnboundedInteger but that would be a bad way to do it because our current code is using it for both isUnboundedInteger + isShortInteger

Better to make two new boolean fields which fully represent these formats.
These two new fields should be added to https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/IJsonSchemaValidationProperties.java

Additional context

This came up when working on imposing format constraints in
#8325

@spacether spacether added Enhancement: Feature Good First Issue An issue that could be fixed with a simple PR labels May 11, 2021
@spacether spacether changed the title [REQ] Add ability to differentiate between integer, int32, and int64 [REQ] Add ability to differentiate between integer and int32 schemas May 11, 2021
@SivaTharun
Copy link

@spacether after seeing your problem statement, i was wondering can we use java BigInteger object, instead of integer type in implementation of IJsonSchemaValidationProperties.java. BigInteger can accommodate both 32 bit java integer as well 64 bit java long values.
However you have mentioned, that the changes should be done a non breaking way.

@spacether
Copy link
Contributor Author

spacether commented May 13, 2021

Which specific use case are you describing? Are you asking if you can swap in BigInteger in place of the current isInteger?
Or are you asking for isBigInteger rather than isUnboundedInteger?
Note: isUnboundedInteger can hold integers that are larger than 64bit I think.

@SivaTharun
Copy link

@spacether yes, i was asking if we can use BigInteger inplace of isUnboundedInteger, even BigInteger supports numerical values greater than 64 bit.
Apart from that, we can easily have the range check between 32 bit and 64 bit integers, by using the constants, Long.MAX_VALUE , Long.MIN_VALUE, INTEGER.MIN_VALUE and INTEGER.MIN_VALUE , which java provides, to differentiate between 32bit and 64 bit integers.

@spacether
Copy link
Contributor Author

spacether commented May 14, 2021

@SivaTharun yes that is a good solution for that use case. Another solution you could consider is using java BigDecimal to store all numeric types (isNumber). In a new generator that I am working on I store all numeric types in python Decimal:
https://github.com/spacether/openapi-generator/blob/adds_python_experimental_dynamic_base_classes/samples/openapi3/client/petstore/python-experimental/petstore_api/model_utils.py#L867
And I decide to send it as a int or float in json depending on how its exponent values are stored:
https://github.com/spacether/openapi-generator/blob/adds_python_experimental_dynamic_base_classes/samples/openapi3/client/petstore/python-experimental/petstore_api/api_client.py#L52
and format max and min values can be imposed in subclasses like Int64.
All other integer and float classes can then be subclasses of your BigDecimal (isNumber) class.

@SivaTharun
Copy link

@spacether The above alternative that you are proposing makes sense , we can use BigDecimal in place of BigInt that works as well, but i had a query related to type conversion of BigDecimal type to int/float type.
Python 3 int does not define any max. value, so theoretically it can take values larger than 32 bit, and for even larger values beyond the limit of sys.maxsize (which defines the max size of int in a python environment) , the corresponding is auto converted to float.

This any way would work fine, in python.
But i feel it would this would lead to overflow issues in java, considering that java unlike python, strictly adheres to the range of integer (32 bit) and float (64 bit) data types.

So basically my question revolves around the case that, what if a number greater than Long.MAX_VALUE, is sent for Json encoding like in this method.

do we need to use the same logic of Type conversion to int/ float type in java too. i fell we will have to directly use BigDecimal here for Json encoding.

@spacether
Copy link
Contributor Author

spacether commented May 17, 2021

But i feel it would this would lead to overflow issues in java, considering that java unlike python, strictly adheres to the range of integer (32 bit) and float (64 bit) data types.

It would if you were strictly casting into classes that allow for overflow but how will you instantiate these classes/schemas?

Number:
type number (int + floats)

Integer:
type: integer (ints only)

Int32:
type: integer
format: int32 (int32 only)

Combo:
  allOf:
    - $ref: '#/components/schemas/Number'
    - $ref: '#/components/schemas/Integer'
    - $ref: '#/components/schemas/Int32'

If you use BigDecimal as the base class as all of those schemas and impose the max and min constraints in the constructor for the Int32 schema it will work. If you assume that you have to choose a class that will have overflows, how will Combo work?

I think that casting into the most permissive container class (python Decimal/java BigDecimal) and then imposing constraints on top of that type at instantiation or validation time is the easiest way to do it.

do we need to use the same logic of Type conversion to int/ float type in java too. i fell we will have to directly use BigDecimal here for Json encoding.

You can choose how you want to send number over the wire. I did it this way to preserve an ingested int being sent as an int and an ingested float being sent as a float. From my reading it sounds like some languages choose to send 9.0 as 9 which is a reason to describe a payload of that type as number and NOT int or float.

Maybe where I cast to int to send it in json you would cast it to BigInt to prevent overflow issues?

@spacether
Copy link
Contributor Author

Per a discussion with @wing328 it is okay to add just isShortInteger and have isInteger cover unbounded integers from now on. It will be a breaking change with fallback, where the fallback is for users to update their template isInteger usages to isShortInteger if they need int32 behavior, or they can update their specs to specify format: int32.
I will work on adding this in the new minor version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement: Feature Good First Issue An issue that could be fixed with a simple PR
Projects
None yet
2 participants