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

Collisions of enum fields in openapi spec #12149

Closed
amhn opened this issue Apr 1, 2023 · 5 comments
Closed

Collisions of enum fields in openapi spec #12149

amhn opened this issue Apr 1, 2023 · 5 comments
Assignees
Labels
beta Concerns a bug/feature in a beta release status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@amhn
Copy link
Contributor

amhn commented Apr 1, 2023

NetBox version

v3.5-beta

Python version

3.10

Steps to Reproduce

Run ./manage.py spectacular --file ../../terraform/go-netbox-openapi/schema.json --validate --format openapi-json with the beta version.

Expected Behavior

Names for enums are predictable and meaningful.

Observed Behavior

Names like Type666Enum, Type2fcEnum, Type6e5Enum, Type653Enum and Type209Enum are chosen for some of the ChoiceFields named Type.

Warning: enum naming encountered a non-optimally resolvable collision for fields named "type". The same name has been used for multiple choice sets in multiple components. The collision was resolved with "Type666Enum". add an entry to ENUM_NAME_OVERRIDES to fix the naming.
Warning: enum naming encountered a non-optimally resolvable collision for fields named "type". The same name has been used for multiple choice sets in multiple components. The collision was resolved with "Type2fcEnum". add an entry to ENUM_NAME_OVERRIDES to fix the naming.
Warning: enum naming encountered a non-optimally resolvable collision for fields named "type". The same name has been used for multiple choice sets in multiple components. The collision was resolved with "Type6e5Enum". add an entry to ENUM_NAME_OVERRIDES to fix the naming.
Warning: enum naming encountered a non-optimally resolvable collision for fields named "type". The same name has been used for multiple choice sets in multiple components. The collision was resolved with "Type653Enum". add an entry to ENUM_NAME_OVERRIDES to fix the naming.
Warning: enum naming encountered a non-optimally resolvable collision for fields named "type". The same name has been used for multiple choice sets in multiple components. The collision was resolved with "Type209Enum". add an entry to ENUM_NAME_OVERRIDES to fix the naming.
Warning: enum naming encountered a non-optimally resolvable collision for fields named "type". The same name has been used for multiple choice sets in multiple components. The collision was resolved with "Type261Enum". add an entry to ENUM_NAME_OVERRIDES to fix the naming.
@amhn amhn added the type: bug A confirmed report of unexpected behavior in the application label Apr 1, 2023
@amhn
Copy link
Contributor Author

amhn commented Apr 1, 2023

To resolve this this the enums shoud be added to ENUM_NAME_OVERRIDES in SPECTACULAR_SETTINGS. Preferably before the 3.5 release to prevent a breaking change after the release.

The following settings should be set. Names are of course only suggestions. For the 3 enums with XX in the name I am not sure what they should be named.

SPECTACULAR_SETTINGS = {
    [...]
    'ENUM_NAME_OVERRIDES': {
        # Used by WritableCableRequest and WritableWirelessLinkRequest
        # ['deprovisioning', 'active', 'planned', 'provisioning', 'offline', 'decommisioned']
        'LinkStatusEnum': 'dcim.choices.LinkStatusChoices',

        # Used by WritableIPRangeRequest and WritableVLANRequest
        # ['active', 'reserved', 'deprecated']
        'XX1XXStatusEnum': 'ipam.choices.IPRangeStatusChoices',
        # or 'XX1XXStatusEnum': 'ipam.choices.VLANStatusChoices',

        # Used by WritableVirtualMachineWithConfigContextRequest and WritableModuleRequest
        # ['offline', 'active', 'planned', 'staged', 'failed', 'decommisioning']
        'XX2XXStatusEnum': 'virtualization.choices.VirtualMachineStatusChoices',
        # or 'XX2XXStatusEnum': 'dcim.choices.ModuleStatusChoices'

        # Used by WritableLocationRequest and WritableSiteRequest
        # ['planned', 'staging', 'active', 'decommisioning', 'retired']
        'XX3XXStatusEnum': 'dcim.choices.LocationStatusChoices',
        # or 'XX3XXStatusEnum': 'dcim.choices.SiteStatusChoices'

        # Used by WritableVirtualDeviceContextRequest and VirtualDeviceContext
        # ['active', 'planned', 'offline']
        'VirtualDeviceContextStatusEnum': 'dcim.choices.VirtualDeviceContextStatusChoices',

        # Used by WritableWirelessLANRequest and WritableWirelessLinkRequest
        # ['open', 'wep', 'wpa-personal', 'wpa-enterprise']
        'WirelessAuthTypeEnum': 'wireless.choices.WirelessAuthTypeChoices',

        # Used by WritableServiceRequest and WritableServiceTemplateRequest
        # ['tcp', 'udp', 'sctp']
        'ServiceProtocolEnum': 'ipam.choices.ServiceProtocolChoices',

        # Used by NestedFHRPGroup, NestedFHRPGroupRequest, FHRPGroup and FHRPGroupRequest
        # ['vrrp2', 'vrrp3', 'hsrp', 'glbp', 'carp', 'clusterxl', 'other']
        'FHRPGroupProtocolEnum': 'ipam.choices.FHRPGroupProtocolChoices',

        # Used by Cable, CableRequest and WritableCableRequest
        # ['cat3', 'cat5', 'cat5e', [...]]
        'CableTypeEnum': 'dcim.choices.CableTypeChoices',

        # Used by WritablePowerPortRequest and WritablePowerPortTemplateRequest
        # ['iec-60320-c6', 'iec-60320-c8', [...]]
        'PowerPortTypeEnum': 'dcim.choices.PowerPortTypeChoices',

        # Used by WritableRearPortRequest, WritableRearPortTemplateRequest, WritableFrontPortRequest and WritableFrontPortTemplateRequest
        # ['8p8c', '8p6c', [...]]
        'PortTypeEnum': 'dcim.choices.PortTypeChoices',

        # Used by WritablePowerOutletRequest and WritablePowerOutletTemplateRequest
        # ['iec-60320-c5', 'iec-60320-c7', [...]]
        'PowerOutletTypeEnum': 'dcim.choices.PowerOutletTypeChoices',

        # Used by WritableConsolePortRequest, WritableConsolePortTemplateRequest, WritableConsoleServerPortTemplateRequest and WritableConsoleServerPortRequest
        # ['de-9', 'db-25', [...]]
        'ConsolePortTypeEnum': 'dcim.choices.ConsolePortTypeChoices',

        # Used by NestedL2VPN, NestedL2VPNRequest, WritableL2VPNRequest and WritableFrontPortTemplateRequest
        # ['vpws', 'vpls', [...]]
        'L2VPNTypeEnum': 'ipam.choices.L2VPNTypeChoices',

        # Used by WritableInterfaceRequest and WritableInterfaceTemplateRequest
        # ['virtual', 'bridge', [...]]
        'InterfaceTypeEnum': 'dcim.choices.InterfaceTypeChoices',
    }
}

I suspect resolving some of the other current warnings will result in further enums that need to be added here.

@abhi1693
Copy link
Member

abhi1693 commented Apr 2, 2023

Resolving this issue as mentioned may not be ideal for plugin developers. This will add unnecessary burden of maintaining enum types by both core and plugin. Probably there is a better way of resolving this.

@kkthxbye-code kkthxbye-code added the beta Concerns a bug/feature in a beta release label Apr 2, 2023
@arthanson arthanson self-assigned this Apr 3, 2023
@jeremystretch jeremystretch added the status: accepted This issue has been accepted for implementation label Apr 3, 2023
@jeremystretch
Copy link
Member

Agreed with @abhi1693. I also worry about the long-term maintainability of this approach.

Can we look deeper into how enums are intended to be discovered dynamically?

@arthanson
Copy link
Collaborator

Note: These are all (?) from the Writable serializers, these are created In core.api.schema.NetBoxAutoSchema.get_writable_class. There is already a check for the ChoiceField so potentially it might be possible to auto-manage the name there to prevent a collision.

@amhn
Copy link
Contributor Author

amhn commented Apr 15, 2023

Most (or all?) of the collisions are from writable serializers because most enums are created from ChoiceField. ChoiceField is rendered as string because of ChoiceFieldFix[1]. This fix is not applied to ChoiceFields from writable serializers.

The easiest way to fix this is disabling postprocess_schema_enums by setting 'POSTPROCESSING_HOOKS': [] in SPECTACULAR_SETTINGS. This disables the deduplication magic in drf-spectacular and all enums are left inline as in the old swagger spec.

Any other way seems more complicated, since postprocess_schema_enums does not take anything except property name and the (hash over the) raw choices into account for naming.

[1] ChoiceFieldFix could be improved to provide the allowed choices by using build_choice_field instead of build_generic_type:

diff --git a/netbox/core/api/schema.py b/netbox/core/api/schema.py
index 0b44a3d52..50d7b4ff8 100644
--- a/netbox/core/api/schema.py
+++ b/netbox/core/api/schema.py
@@ -12,6 +12,7 @@ from drf_spectacular.plumbing import (
     build_basic_type,
     build_media_type_object,
     build_object_type,
+    build_choice_field,
     is_serializer,
 )
 from drf_spectacular.types import OpenApiTypes
@@ -38,7 +39,7 @@ class ChoiceFieldFix(OpenApiSerializerFieldExtension):
 
     def map_serializer_field(self, auto_schema, direction):
         if direction == 'request':
-            return build_basic_type(OpenApiTypes.STR)
+            return build_choice_field(self.target)
 
         elif direction == "response":
             return build_object_type(

At least for requests, responses are more complicated, at least the label part.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beta Concerns a bug/feature in a beta release status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

No branches or pull requests

5 participants