tfranzel / drf-spectacular

Sane and flexible OpenAPI 3 schema generation for Django REST framework.
https://drf-spectacular.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2.37k stars 263 forks source link

Can't understand ENUM_NAME_OVERRIDES documentation #482

Closed spookylukey closed 3 years ago

spookylukey commented 3 years ago

After reading the docs on ENUM_NAME_OVERRIDES:

and the error messages:

...I still don't know what it is talking about. What are the keys of the dictionary, and what are the values? What entry do I add, and how does that help?

Sorry for being dense, but if this is unclear to me probably someone else is struggling too. Thanks in advance!

tfranzel commented 3 years ago

Hi @spookylukey, i tried to explain it to the best to my ability. let me try to give some further details.

does that explanation help?

spookylukey commented 3 years ago

@tfranzel Thanks so much that really helps. If you don't mind I'll try to come up with a patch for the docs using this explanation.

johnthagen commented 3 years ago

For me as a new user, I think the primary source of confusion is that really we want to map/assign values to a name so at first blush it seems like the settings should be reversed?

'ENUM_NAME_OVERRIDES': {
    (('ON', 'ON'), ('OFF', 'OFF'),): 'DeviceStatusEnum'
},

It was confusing to me at first that I needed to come up with key first for given values instead of the other way around.

tfranzel commented 3 years ago

@johnthagen from a logical point of view you are absolutely right. I made this choice for 2 reasons.

  1. keys must be hashable, so a list would fail. its easy to convert but I tried to make it easy on the users.
  2. enforce the enum names are unique.

argument 2 is a bit weak as the same choice set can be used for multiple names (keys), but we do catch that and emit a warning. I think you can make a valid case for both directions, but in principle it should be a 1:1 mapping anyway.

johnthagen commented 3 years ago

@tfranzel After thinking about it some more, you're right it could go either way. I think that the docs @spookylukey added helped, after I wrapped my head around them some more.

tu-pm commented 2 years ago

Is there anyway to get around this?

Most of my choice set constants are defined within the scope of my apps, and many of them name "status". So importing them in the settings.py file raises two problems:

  1. Importing constants from apps to settings does not work
  2. Manually specify all the constants in the settings is quite cumbersome

I think there should be a better way to automatically resolve naming conflicts, for example by prepending the enum name by the serializer/model name.

tfranzel commented 2 years ago

Is there anyway to get around this?

technically impossible, but it is not as bad as you think.

  1. Importing constants from apps to settings does not work

no need. you can -- in django fashion -- use import strings and point them to your enum (UserStatusTypeEnum). we accept Enum, Choices, List[str] and List[Tuple[str, Any]], both as import strings and as literal objects. (see example)

  1. Manually specify all the constants in the settings is quite cumbersome

as mentioned above, manually stating them is not the normal flow but rather a convenience shortcut for one-offs.

I think there should be a better way to automatically resolve naming conflicts

We do actually. If a conflicting enum is only used in one component, that enum is prefixed with the component/serializer name. Otherwise we simply don't know which name to choose.

Example:

SPECTACULAR_SETTINGS = {
    "VERSION": "0.0.1",
    "TITLE": "Your API",
    "DESCRIPTION": "your API",
    "SERVE_INCLUDE_SCHEMA": False,
    "ENUM_NAME_OVERRIDES": {
        "UserStatusTypeEnum": "core.choices.UserEventTypes",
        "GroupStatusTypeEnum": ['AA', 'BB'],
    },
}
tu-pm commented 1 year ago

Hi @tfranzel , thanks for answering. But there's still a slight problem with this approach that I'd like to discuss:

When there are multiple model serializers based on the same model, each one contains a different set of fields, but all share the same enum field, then these ugly warnings are flooded all over the console log, making it harder to find actual swagger errors.

For example: The model User has three different serializers, UserBriefSerializer, UserDetailSerializer, UserAdminSerializer, all of them share the same enum field called "type", so there are three warnings that get emitted. It gets worse when there are many serializers that follow the same pattern.

I think there should be a mechanism to omit these kinds of warning, since it's very excessive..

tu-pm commented 1 year ago

After taking a peak at the source code, I see that the general approach is to collapse all fields with the same choice set and the same prop name to one Enum, which results in naming conflicts. Isn't it easier to just prepend component_name to all Enums so that there are no conflicts?

tfranzel commented 1 year ago

@tu-pm can you please an concrete example? It is easier to reason about.

Isn't it easier to just prepend component_name to all Enums so that there are no conflicts?

That would be true, but you would also have potentially a lot of duplicated components. It was a design decision I made, and now changing this needs really good reason, due to this potentially breaking a lot of people.

I actually tried to fix this in django, but retaining a reference to the enum was rejected: https://code.djangoproject.com/ticket/34388#ticket