Nullable enums not validating

[idesoto-rover]

Hi, I'm having the following issue. drf-spectacular for some reason generates nullable enums like a oneOf of two enum types, one which contains the enum options and a NullEnum containing only the None option. So for example, if I have the following field in my model:

    building_type = models.CharField(
        max_length=25,
        choices=(('hs', 'House'), ('apt', 'Apartment'), ('farm', 'Farm')),
        default=None,
        null=True,
    )

It will generate the following field in my schema:

...
        building_type:
          nullable: true
          oneOf:
          - $ref: '#/components/schemas/BuildingTypeEnum'
          - $ref: '#/components/schemas/NullEnum'
...
    BuildingTypeEnum:
      enum:
      - hs
      - apt
      - farm
      type: string
    NullEnum:
        enum:
        - null

So the issue I'm having is that drf-openapi-tester fails to validate this when using a valid enum option (e.g. hs in the example). This is because handle_one_of method will fail if the value matches both of the types. It's matching the BuildingTypeEnum correctly because hs is one of the options in the enum, but it's also matching the NullEnum because of these two lines in test_schema_section:

        schema_section_type = self.get_schema_type(schema_section)
        if not schema_section_type:
            return

Since NullEnum doesn't have a type or a properties field, get_schema_type is returning None and so it will exit early without running the validators.

The quickest fix I can think of to support this case is to change those two lines to:

        schema_section_type = self.get_schema_type(schema_section)
        if not schema_section_type and 'enum' not in schema_section:
            return

but maybe theres' some more generic fix?

FTR, this is what drf-spectacular says about this NullEnum type: tfranzel/drf-spectacular#235

[Goldziher]

Hi, thanks for your report.

I believe the behaviour of DRF spectacular is in fact wrong. Use of oneOf and the null enum are a proposal, not a part of the spec.

I will see about extending support for this. For the time being though, you can turn off the the DRF spectacular behaviour in that library's configs.

[idesoto-rover]

Thanks! Yes, that was my plan B, to disable the NullEnum in drf-spectacular. But when I do that it will generate something like this:

        building_type:
          allOf:
          - $ref: '#/components/schemas/BuildingTypeEnum'
          nullable: true

which is not correct according to the OpenAPI Enums documentation because it should contain a null option.

Note that null must be explicitly included in the list of enum values. Using nullable: true alone is not enough here.

But I guess that would be an issue for drf-spectacular 🙂

[Goldziher]

TBH the way drf spectacular is behaving here is very weird. Why allOf? ffs.

Anyhow, we are always happy for contributions :). But the fix you are proposing is not really a fix - this piece of logic is used not only for enums:

        schema_section_type = self.get_schema_type(schema_section)
        if not schema_section_type and 'enum' not in schema_section:
            return

If you would like to offer a fix PR on your own, please start with adding a test case for the behaviour that is currently buggy - that will be a big help. A followup PR can perhaps be (without actually going into the code) to make a proper enum validator in the validators.py file.

[idesoto-rover]

I know this method is not only used for enums, but this extra condition will not affect anything else. The problem is that this check is currently assuming that anything that doesn't have a type or properties doesn't need to be validated, which is not the case of the NullEnum example (assuming that is valid, which I think it is because it's allowed to not have an explicit type and then it just adds an enums property that would have to be validated).

The project already has an enum validator in validators.py, but the problem is that because of this check that we're discussing it is not ran for the NullEnum because the method just returns before it is ran.

I'd be happy to add a PR including tests, but I think i need permissions to be able to create a branch?

[JonasKs]

I'd be happy to add a PR including tests, but I think i need permissions to be able to create a branch?

See CONTRIBUTING.md ☺️

[idesoto-rover]

Thanks 🙂

[tfranzel]

Hey! the author of drf-spectacular here 😄 this issue has been bugging me for a while too.

TBH the way drf spectacular is behaving here is very weird. Why allOf? ffs.

        building_type:
          allOf:
          - $ref: '#/components/schemas/BuildingTypeEnum'
          nullable: true

@Goldziher the reason we do this is because of the $ref semantics. $ref will replace the scope it is located in and thus would wipe out the nulllable. nesting inside a allOf, oneOf creates a new scope and thus leaves the nullable untouched. https://swagger.io/docs/specification/using-ref/ ($ref and Sibling Elements)

I believe the behaviour of DRF spectacular is in fact wrong. Use of oneOf and the null enum are a proposal, not a part of the spec.

i admit i went creative there in order to prevent noise via trivial duplication. @Goldziher what exactly is the proposal part you are refering to (null in enum or oneOf semantics)? the oneOf certainly makes the parsing more difficult, but to my current understanding this is absolutely valid and also the right thing semantically.

happy to support the resolution of this. please point out if i said anything questionable or wrong.

[Goldziher]

Howdy @tfranzel, welcome :)

Thanks for the explanation - I now understand why you are using allOf and it does make sense for schema reusability.

The proposal component is regarding what "nullable: true/false" means, e.g. whether or not it prohibits / allows null values and how to interpret this. If I understood this link correctly: https://github.com/OAI/OpenAPI-Specification/blob/master/proposals/003_Clarify-Nullable.md#if-a-schema-specifies-nullable-true-and-enum-1-2-3-does-that-schema-allow-null-values-see-1900

But TBH I left academia to not argue about "semantics" :), so let's not do this.

I will take a look at how we are handling enums later tonight or tomorrow morning. @idesoto-rover are you working on this actively?

[idesoto-rover]

@Goldziher No, sorry, I haven't had time to start working on this yet.

[Goldziher]

Ok, so I have been digging into this on my end. I do find the specs generated by drf-spectacular problematic @tfranzel. I have the following model:

class Names(models.Model):
    custom_id_field = models.IntegerField(primary_key=True)
    name = models.CharField(
        max_length=254,
        choices=(("mo", "Moses"), ("moi", "Moishe"), ("mu", "Mush")),
        default=None,
        null=True,
        blank=True
    )

Then a serializer / view setup like so:

class NamesSerializer(serializers.ModelSerializer):
    class Meta:
        model = Names
        fields = "__all__"


class NamesRetrieveView(RetrieveAPIView):
    model = Names
    serializer_class = NamesSerializer
    queryset = Names.objects

    def get_object(self):
        return Names.objects.get(custom_id_field=int(self.kwargs["pk"]))

The resulting schema looks like this:

components:
  schemas:
    BlankEnum:
      enum:
        - ''
    NameEnum:
      enum:
        - mo
        - moi
        - mu
    NullEnum:
      enum:
        - null
    Names:
      type: object
      properties:
        custom_id_field:
          type: integer
        name:
          nullable: true
          oneOf:
            - $ref: '#/components/schemas/NameEnum'
            - $ref: '#/components/schemas/BlankEnum'
            - $ref: '#/components/schemas/NullEnum'
        required:
            - custom_id_field

The problem from my side is that there is no type in the "name", rather the type property is present only in the NameEnum. This is a problem because oneOf does not mean I need to merge sub-schemas, hence the type cannot be inferred there inside the oneOf validation logic itself.

I will need to work around this.

[Goldziher]

PR added, I think this resolves the issue well - I basically replace the oneOf with a merge in the particular case of having multiple enums inside the oneOf.

Feel free to review

[Goldziher]

@idesoto-rover - v1.3.4 was released with the fix. Please test if it resolves your issue.

[idesoto-rover]

Solves the issue! Thank you so much!!! 👏