[python] required AND nullable

[cognifloyd]

Description

I'm modeling an existing api in an OpenAPI 3.0 spec.
A key thing across most of this particular API (like it or not) is that all fields must be present (they are all required), but most of the time the fields are nullable. Some APIs include the fields and never use them (ie they are always set to null), but try to send a request without the field and it fails. (A replacement API is in the works, but until then, legacy prevails...)

So, I tried to model this by listing the requestBody parameters, and adding all of them to required. Then, I marked the fields that can be null as nullable: true.

However, in the python model template, required results in:

openapi-generator/modules/openapi-generator/src/main/resources/python/model.mustache

Lines 97 to 100 in 7184f1e

	{{#required}}
	if {{name}} is None:
	raise ValueError("Invalid value for `{{name}}`, must not be `None`") # noqa: E501
	{{/required}}

Could this be updated to support nullable? So that it raises the ValueError on None only if required and NOT nullable?

openapi-generator version

master

OpenAPI declaration file content or url

components:
  schemas:
    MySomethingOrOther:
      type: object
      required:
        - id
        - required_nullable_option
      properties:
        id:
          type: integer
          example: 96
        required_nullable_option:
          type: string
          nullable: true
          example: "something"

Command line used for generation

python is the target

Steps to reproduce

Mark something as both required and nullable, and then pass None as the value.

Suggest a fix/enhancement

check for required and not nullable

openapi-generator/modules/openapi-generator/src/main/resources/python/model.mustache

Lines 97 to 100 in 7184f1e

	{{#required}}
	if {{name}} is None:
	raise ValueError("Invalid value for `{{name}}`, must not be `None`") # noqa: E501
	{{/required}}

I'm not familiar with what sets the vars like "required" in the template, so I'm not sure where to get the nullable option to modify this.

[wing328]

Could this be updated to support nullable? So that it raises the ValueError on None only if required and NOT nullable?

@cognifloyd yes, may I know if you've time to contribute the enhancement? I can show you some good starting points.

[cognifloyd]

I looked to see if nullable was set injected into the templates, but it's not. A couple of the templates use x-nullable, which gets passed in thanks to the vendor x- prefix. I don't have time or inclination to deal with the Java side of this. If someone wants to add injecting nullable into templates, then I'd be happy--assuming I can find some time--to take care of the mustache template + python portion.

[cognifloyd]

I found the inclination and so went digging through the Java even though I'm a rookie with Java. I started with swagger-codegen (because I'm using it via swaggerhub right now and I'm getting irritated with OAS 2.0) and then came to do the same here and discovered that the Java side is done.

• Add nullable support #873
• Add x-nullable (OAS2) support #889
• Remove x-nullable extension support to support nullable #930

And go figure. I didn't know what I was doing in swagger-codegen, so I updated (squash + force push) my PRs there based on your excellent work here.

• Add nullable property support from OAS 3.0 spec swagger-api/swagger-codegen#8809
• Add nullable property support from OAS 3.0 spec  swagger-api/swagger-codegen-generators#213

Now, I hope I can find some more time to actually work on the python side.

[wing328]

@cognifloyd I've submitted #1073 before. Is that what you're looking for?

(sorry that I forgot to update you about this enhancement before)

[cognifloyd]

Wow. yeah. That's great! 🎉 Thank you @wing328 for doing that. Now to play with and use it.

[wing328]

@cognifloyd you're welcome. Please give it a try and let us know if you've any feedback.

[sanderegg]

it seems to be again a problem since release 3.3.2

[wing328]

@sanderegg can you please elaborate on the issue with a spec to reproduce the problem and the actual vs expected output?

[sanderegg]

@wing328 sorry yes, actually the exact same case described in here.
I was happily using version 3.2.3 till now and moved to 3.3.2 and found out that a required and nullable field would now throw a ValueError from the generated python code.

• my openapi spec looks like this:

ErrorEnveloped:
# - notice that data is defaulted to null
#
  type: object
  required:
    - data
    - error
  properties:
    data:
      nullable: true
      default: null
    error:
      $ref: "#/ErrorItemType"

ErrorItemType:
  type: object
  required:
    - code
    - message
  properties:
    code:
      type: string
      description: Typically the name of the exception that produced it otherwise some known error code
    message:
      type: string
      description: Error message specific to this item
    resource:
      type: string
      description: API resource affected by this error
    field:
      type: string
      description: Specific field within the resource

The generated code for this part now creates:

  if data is None:
    raise ValueError("Invalid value for 'data', must not be 'None'")

And it is not the case with version 3.3.1

[wing328]

@cognifloyd thanks for sharing the spec. Are you using OpenAPI spec v2 or v3?

Only v3 officially supports nullable. For v2, you can use x-nullable (vendor extension) instead.

[wing328]

I also did a test locally and it's working fine.

[sanderegg]

@wing328 using spec v3
here is a screenshot of the differences I see between 3.3.1 and 3.3.2 in the python generated code:
left is 3.3.1, right is 3.3.2 for the same specs

[wing328]

@cognifloyd I was not able to repeat the issue with OAS3 spec using the latest master. The nullable support was introduced to v3.3.0 via #1073.

Can you please try the latest master instead? You can find the links to the SNAPSHOT version in the project's README.

[cognifloyd]

@wing328 I think you meant @sanderegg :)

[sanderegg]

I'm using the docker images, is there one for master as well? like latest? it seems to be recent.

[sanderegg]

well using the latest docker (which is an hour old at this time of writing) I get the same issue.

[wing328]

@cognifloyd sorry :(

@sanderegg can you please share the full spec with me so that I can repeat the issue locally? You can find my email address in my Github profile page.

[sanderegg]

yes will do in the evening... going home at the moment...

[sanderegg]

Ok I think I found the issue. It seems the problem arises when the property is nullable and a ref (see schema that reproduces the issue below). I confirm that the problem with nullable of type string works fine. The problem arises when the property is a ref and nullable.
Now searching a bit, I am not quite sure if this is accepted as such or if one should use some allOf trick to make that work? What I am sure is that the very same spec did work until the latest release...

openapi: 3.0.0
info:
  version: 0.1.0
  title: XXXXXXXXXXXXXXXXXXXXXX

paths:
  /:
    get:
      responses:
        "200":
          description: Service information
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/HealthCheckEnveloped'
components:
  schemas:
    HealthCheckEnveloped:
      type: object
      required:
        - data
        - error
      properties:
        data:
          nullable: true
          default: null
          $ref: '#/components/schemas/HealthCheckType'
          
        error:
          nullable: true
          default: null
          type: string

    HealthCheckType:
      type: object
      nullable: true
      default: null
      properties:
        name:
          type: string
        status:
          type: string
        api_version:
          type: string
        version:
          type: string

[sanderegg]

so replacing the above specs, by

data:
          nullable: true
          default: null
          anyOf:
            - $ref: '#/components/schemas/HealthCheckType'

works!

Thanks for your time, I think it's all good! probably made my api better;)