Incorrect validations for eg. CreateIndex, JobModifyIndex, ModifyIndex

[schlumpfit]

As an example the JobListStub is defined in the openapi.yml as:

JobListStub:
      properties:
        CreateIndex:
          maximum: 1.8446744073709552e+19
          minimum: 0
          type: integer

Which seems to generate the golang code without further validations:

// JobListStub struct for JobListStub
type JobListStub struct {
	CreateIndex *int32 `json:"CreateIndex,omitempty"`
	...
}

But for ruby/python validations are added:

    validations = {
        ('create_index',): {
            'inclusive_maximum': 384,
            'inclusive_minimum': 0,
        },
         ...
    }

    # Check to see if the all the properties in the model are valid
    # @return true if the model is valid
    def valid?
      return false if !@create_index.nil? && @create_index > 384
      return false if !@create_index.nil? && @create_index < 0
      return false if !@job_modify_index.nil? && @job_modify_index > 384
      return false if !@job_modify_index.nil? && @job_modify_index < 0
      return false if !@modify_index.nil? && @modify_index > 384
      return false if !@modify_index.nil? && @modify_index < 0
      true
    end

[DerekStrickland]

Hi @schlumpfit

Thanks so much for taking a look at the project!

The code for each target client actually gets generated by the OpenAPI Generator. It's not super obvious how the flow works, so I'll try to explain here and look at ways to make the docs better in general.

The generator directory in the project contains code to generate an OpenAPI specification, not the client code. Once the specification is generated, you can use it to generate a client or server using any OpenAPI code generator. There are several, but we chose to use the one linked above. To see how it is used, take a look at the Makefile at the root of this repository.

To change the output of the generated code, you would have to affect the change using whatever tool you are using to generate the clients. In our case, the OpenAPI Generator allows us to configure custom templates in the config.yaml for any target client. You can see an example of where we've done that in a couple of cases to fix some mistakes in the upstream README templates for the Rust targets. You could also define template overrides for the code generation files, not just the doc generation files.

All that said, these examples look a little off to me. The Go one does what I expect. The minimum and maximum values in the specification define the range of integer values allowed. In essence, it specifies that this is an int32. The Go generator sees that and says "Ok. This is an int32. I'll define that on the model as the type of the field." Then if a user passes an int64 the compiler will stop them because the value is out of range. So the validation happens at compile time for strongly typed languages like Go.

In the cases of Ruby and Python that you point out, that generated code looks wrong. I'm not overly familiar with either language, but a quick internet search leads me to believe that neither has the concept of an explicit int32. Please let me know if I am wrong.

Either way, if I understand the example you posted, the issue lies somewhere in the upstream OpenAPI Generator templates for Ruby and Python. An int32 can definitely hold a value greater than 384. It would seem their bounds checking template logic is off.

We maintain a Go implementation in the v1 directory for the community to use, but we don't internally manage the other languages other than to provide a starting point for community use. We're actively looking for community partners to champion the other language targets, and I hope you'll consider lending your expertise for the benefit of all.

Again, thanks for trying out the project, and let me know if I can help in any way. Also, if you are interested in helping improve the quality of the Ruby or Python clients, let me know. We have monthly community meetings where we can interact synchronously, and as always, PRs are welcome.

[schlumpfit]

Hi @DerekStrickland,
thanks for the feedback.

TLDR: It is not an issue of the generator per se but of the openapi document describing the model

I am familiar with OpenAPI and Swagger and already used it in some of my projects (ruby/python/rust). According to my understanding the issue here comes from the generator that generates the OAS and not the client generators directly (although they should catch this glitch).
The OAS datatypes are described here: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#dataTypes.
Also the values for min//max values make no sense for an int32, but would make sense for an uint64.
From the spec I would read that only int64 is supported and not uint64 which could cause some kind of overflow.

This quick example seems to proof my understanding:

openapi: 3.0.3
info:
  description: Example
  version: "0.1.0"
  title: OpenApiDataTypes

paths:
  /some_model:
    get:
      description: Get some model
      responses:
        '200':
          description: success
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/SomeModel"

components:
  schemas:
    SomeModel:
      type: object
      properties:
        value_integer:
          type: integer
        value_with_format_int32:
          type: integer
          format: int32
        value_with_format_int64:
          type: integer
          format: int64
        value_with_validation_and_format_int32:
          type: integer
          format: int32
          minimum: "-2147483648"
          maximum: 2147483647
        value_with_validation_and_format_int64:
          type: integer
          format: int64
          minimum: "-9223372036854775808"
          maximum: 9223372036854775807
        value_with_validation_and_format_uint64_overflow:
          type: integer
          minimum: 0
          maximum: 1.8446744073709552e+19
#        value_with_validation_and_format_uint64_overflow:
#          type: integer
#          format: int64
#          minimum: 0
#          maximum: 1.8446744073709552e+19

Given this spec above only properties with min/max values get a validation. It seems to me the go-generator just ignores them. So I think it is more by luck that the go client works.

The produced ruby validations look as follow:

...
    # Check to see if the all the properties in the model are valid
    # @return true if the model is valid
    def valid?
      return false if !@value_with_validation_and_format_int32.nil? && @value_with_validation_and_format_int32 > 2147483647
      return false if !@value_with_validation_and_format_int32.nil? && @value_with_validation_and_format_int32 < -2147483648
      return false if !@value_with_validation_and_format_int64.nil? && @value_with_validation_and_format_int64 > 9223372036854775807
      return false if !@value_with_validation_and_format_int64.nil? && @value_with_validation_and_format_int64 < -9223372036854775808
      return false if !@value_with_validation_and_format_uint64_overflow.nil? && @value_with_validation_and_format_uint64_overflow > 384
      return false if !@value_with_validation_and_format_uint64_overflow.nil? && @value_with_validation_and_format_uint64_overflow < 0
      true
    end
...

The commented out part actually raises an generator error (without useful information)

#        value_with_validation_and_format_uint64_overflow:
#          type: integer
#          format: int64
#          minimum: 0
#          maximum: 1.8446744073709552e+19

Could you please provide me the reason why you closed the ticket? The code/behaviour I am referring to was generated using the openapi-generator with the document under v1/openapi.yml and not the code from this repository (which has the same issues).

[DerekStrickland]

Hi again @schlumpfit,

Oh, how embarrassing! Thank you for being so kind and detailed in your response. Your observation looks absolutely correct. I closed the issue thinking that the problem was in the upstream generator, but am definitely re-opening this now. Please forgive my mistake. In my haste to get you a timely response, I didn't dig deep enough and relied on what I thought I saw. 😢

Based on what you've shared with me here, I'll have to go take a look at the code in generator/specbuilder.go. I see your point about these fields being a uint64. If I look at the spec a little closer, I now see where some integer fields do end up getting generated with a format specifier and others do not. Also, interestingly it is not just uint64 fields.

The generator package was written as a way to retro-actively document the existent Nomad HTTP API. Running that package will look at the endpoint configuration set up for each API area, and then use that to build an in-memory representation of an OpenAPI spec using kin-openapi. Part of the configuration includes passing an instance of the Request or Response struct as part of the configuration. When the configuration is loaded it turns those struct instances into a reflect.Type, and then passes that to kin's GenerateSchemaRef method.

I'll have to dig into whether there is a bug in kin or if I am expected to provide it with more configuration to get the desired results. It really could be either. Also, if you want to take a stab at it, I won't complain. If the issue does turn out to be on the kin side, I'm happy share that I've submitted some PR's there before and they are really great to work with.

I totally missed this, so thank you again for pointing it out, and thank you for correcting my misfire on the original response with patience, kindness, and examples!

Cheers,
@DerekStrickland

[yihuaf]

@DerekStrickland Hi I am running into this issue and am interested in fixing it. Can you give me a bit more pointer on where to start looking at this issue? This is my first time dealing with code generator code, so I am a bit lost on where to start looking. If I understand correctly, "EvalCreateIndex" is an int32 defined in the struct field. Currently the output openapi.yaml is:

        EvalCreateIndex:
          maximum: 1.8446744073709552e+19
          minimum: 0
          type: integer

Note, the range is uin64. The expected output should be:

        EvalCreateIndex:
          format: int32 # or int64?
          type: integer

Is this correct understanding?

[yihuaf]

Update, actually the generator is behaving correctly because the actual schema it reads from is:

// JobRegisterResponse is used to respond to a job registration
type JobRegisterResponse struct {
	EvalID          string
	EvalCreateIndex uint64
	JobModifyIndex  uint64

	// Warnings contains any warnings about the given job. These may include
	// deprecation warnings.
	Warnings string

	QueryMeta
}

Note, the EvalCreateIndex does have uint64 types and it is correctly passed to kingen. So the issue is kingen's GenerateSchemaRef for a field if uint64 returns:

          maximum: 1.8446744073709552e+19
          minimum: 0
          type: integer

The min and max are correct, but what is the openapi expectation of uin64?

[yihuaf]

Update, there is the issue. OpenAPITools/openapi-generator#11940

In all honesty, python openapi generator should fix this. The integer type with min 0 and max 1.8446744073709552e+19 is a reasonable interpretation of the integer type, especially since there is no uint64 format in the openapi spec. With that being said, OAS doesn't have unsigned int types. I see a lot of other static typed languages also struggle to deal with big int and unsigned int 64.

The alternative is to only generate the minimum for uint64 types and remove the maximum. Either kingen can implement the fix or this repo can implement the fix.

If you are OK with the approach to removing maximum when the type is uint64 from the generated schema ref, I am happy to help to create a PR. However, I do understand if you don't want to fix it this way.

[DerekStrickland]

Hi @yihuaf

Thanks for looking into this! I'm sorry it took so long to respond. I was on vacation last week.

I'm seeing a lot of discussion around this topic, and the approach kin is taking appears to be the recommended approach. I'd rather not make a change that affects all language targets because of an incompatibility in the generator for one language. If you want to remove the maximum in a language specific way using a user-defined template specifically for python, I think that would be a path to explore. We did that to fix a problem in the README template for Rust for example.

How does that sound?