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

Use DjangoObject from Graphene lib. Remove GQL types for models that already use the decorator. #37

Merged
merged 2 commits into from Dec 8, 2021

Conversation

progala
Copy link
Contributor

@progala progala commented Nov 24, 2021

  • Imports DjangoObject from Graphene lib directly instead of using the Nautobot reference.
  • Remove GraphQL types for models that already have GraphQL types through the @extras_features decorator.
  • Add to the plugin's ready() method GraphQL override for the TaggableManager representation.
    This is copied from the core. Without it GraphQL can't auto-convert tags field on manually defined types. This worked previously because importing DjangoObject from the Nautobot modules triggerred execution of the code that does it in the core.

@progala progala linked an issue Nov 24, 2021 that may be closed by this pull request
Copy link

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like duplicating convert_django_field.register code from core - what happens if core changes how it renders tags, won't this end up conflicting with it?

I thought we'd fixed this class of initialization-order issues in nautobot/nautobot#1048 - is there still a problem in 1.2.0b1?

import nautobot_device_lifecycle_mgmt.signals # pylint: disable=C0415,W0611 # noqa: F401

@convert_django_field.register(TaggableManager)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be required because this is already part of code https://github.com/nautobot/nautobot/blob/fcfd50e7e8aeb1d46d8fe6419d5db7e6a8f33ef3/nautobot/extras/apps.py#L25
Did you try without it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's failing in Nautobot 1.1.3 without it, for instance. It looks like when we did import with from nautobot.extras.graphql.types import DjangoObjectType the relevant code was triggered before GraphQL field conversion.

Once we changed the import to from graphene_django import DjangoObjectType we get the below error unless the code is added to __init__. Perhaps dummy import from nautobot.extras.graphql.types to trigger the execution could work as a workaround with Nautobot < 1.2.0.b1 ?

Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/nautobot/core/cli.py", line 182, in <module>
    main()
  File "/usr/local/lib/python3.7/site-packages/nautobot/core/cli.py", line 62, in main
    initializer=_configure_settings,  # Called after defaults
  File "/usr/local/lib/python3.7/site-packages/nautobot/core/runner/runner.py", line 266, in run_app
    management.execute_from_command_line([runner_name, command] + command_args)
  File "/usr/local/lib/python3.7/site-packages/django/core/management/__init__.py", line 401, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python3.7/site-packages/django/core/management/__init__.py", line 377, in execute
    django.setup()
  File "/usr/local/lib/python3.7/site-packages/django/__init__.py", line 24, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/usr/local/lib/python3.7/site-packages/django/apps/registry.py", line 122, in populate
    app_config.ready()
  File "/source/nautobot_device_lifecycle_mgmt/__init__.py", line 59, in ready
    super().ready()
  File "/usr/local/lib/python3.7/site-packages/nautobot/extras/plugins/__init__.py", line 89, in ready
    graphql_types = import_object(f"{self.__module__}.{self.graphql_types}")
  File "/usr/local/lib/python3.7/site-packages/nautobot/extras/plugins/utils.py", line 44, in import_object
    spec.loader.exec_module(module)
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/source/nautobot_device_lifecycle_mgmt/graphql/types.py", line 14, in <module>
    class ValidatedSoftwareLCMType(DjangoObjectType):
  File "/usr/local/lib/python3.7/site-packages/graphene/utils/subclass_with_meta.py", line 52, in __init_subclass__
    super_class.__init_subclass_with_meta__(**options)
  File "/usr/local/lib/python3.7/site-packages/graphene_django/types.py", line 227, in __init_subclass_with_meta__
    construct_fields(model, registry, fields, exclude, convert_choices_to_enum),
  File "/usr/local/lib/python3.7/site-packages/graphene_django/types.py", line 64, in construct_fields
    field, registry, convert_choices_to_enum=_convert_choices_to_enum
  File "/usr/local/lib/python3.7/site-packages/graphene_django/converter.py", line 113, in convert_django_field_with_choices
    converted = convert_django_field(field, registry)
  File "/usr/local/lib/python3.7/functools.py", line 840, in wrapper
    return dispatch(args[0].__class__)(*args, **kw)
  File "/usr/local/lib/python3.7/site-packages/graphene_django/converter.py", line 122, in convert_django_field
    "Don't know how to convert the Django field %s (%s)" % (field, field.__class__)
Exception: Don't know how to convert the Django field nautobot_device_lifecycle_mgmt.ValidatedSoftwareLCM.tags (<class 'taggit.managers.TaggableManager'>)

@progala
Copy link
Contributor Author

progala commented Nov 29, 2021

I don't like duplicating convert_django_field.register code from core - what happens if core changes how it renders tags, won't this end up conflicting with it?

I thought we'd fixed this class of initialization-order issues in nautobot/nautobot#1048 - is there still a problem in 1.2.0b1?

Thanks @glennmatthews. Is there a way to make it work with Nautobot < 1.2.0b1? Perhaps dummy import from nautobot.extras.graphql.types to trigger the execution of the existing code?

@glennmatthews
Copy link

Dummy import is definitely worth a try.

Copy link
Contributor

@josh-silvas josh-silvas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @progala, maybe we can merge this one since you have it functioning for both versions. Then investigate other possible solutions so that we have a working 1.2 version for now.

@progala
Copy link
Contributor Author

progala commented Dec 2, 2021

Looks good @progala, maybe we can merge this one since you have it functioning for both versions. Then investigate other possible solutions so that we have a working 1.2 version for now.

I just committed an update with conditional dummy import of nautobot.extras.graphql.types and confirmed it's working for both Nautobot 1.1.3 and 1.2.0b1.

@progala
Copy link
Contributor Author

progala commented Dec 2, 2021

Dummy import is definitely worth a try.

Dummy import did the trick. I got rid of the duplicated code from the core.

Copy link
Contributor

@josh-silvas josh-silvas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dgarros dgarros merged commit 830c6ae into develop Dec 8, 2021
@progala progala deleted the pr-graphql-fix branch February 11, 2022 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

graphql in multiple places
5 participants