Skip to content

Fix passing required=True to DjangoConnectionField#609

Merged
phalt merged 2 commits intographql-python:masterfrom
acu:fix-connection-field-required
May 6, 2019
Merged

Fix passing required=True to DjangoConnectionField#609
phalt merged 2 commits intographql-python:masterfrom
acu:fix-connection-field-required

Conversation

@alexkirsz
Copy link
Copy Markdown
Contributor

@alexkirsz alexkirsz commented Mar 30, 2019

Passing required=True to an instance of DjangoConnectionField currently raises the following exception:

  ...
  File "/env/lib/python3.6/site-packages/graphql/type/typemap.py", line 106, in reducer
    field_map = type.fields
  File "/env/lib/python3.6/site-packages/graphql/pyutils/cached_property.py", line 22, in __get__
    value = obj.__dict__[self.func.__name__] = self.func(obj)
  File "/env/lib/python3.6/site-packages/graphql/type/definition.py", line 221, in fields
    return define_field_map(self, self._fields)
  File "/env/lib/python3.6/site-packages/graphql/type/definition.py", line 235, in define_field_map
    field_map = field_map()
  File "/env/lib/python3.6/site-packages/graphene/types/typemap.py", line 274, in construct_fields_for_type
    map = self.reducer(map, field.type)
  File "/env/lib/python3.6/site-packages/graphene-django/graphene_django/fields.py", line 61, in type
    _type, DjangoObjectType
TypeError: issubclass() arg 1 must be a class

The reason for the exception is that DjangoConnectionField does not expect its type to be wrapped in a NonNull instance. This PR ensures that it handles that case properly.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 30, 2019

Coverage Status

Coverage decreased (-0.3%) to 94.071% when pulling 8beadc7 on acu:fix-connection-field-required into ea2cd98 on graphql-python:master.

3 similar comments
@coveralls
Copy link
Copy Markdown

coveralls commented Mar 30, 2019

Coverage Status

Coverage decreased (-0.3%) to 94.071% when pulling 8beadc7 on acu:fix-connection-field-required into ea2cd98 on graphql-python:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 30, 2019

Coverage Status

Coverage decreased (-0.3%) to 94.071% when pulling 8beadc7 on acu:fix-connection-field-required into ea2cd98 on graphql-python:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 30, 2019

Coverage Status

Coverage decreased (-0.3%) to 94.071% when pulling 8beadc7 on acu:fix-connection-field-required into ea2cd98 on graphql-python:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 30, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 93.934% when pulling b49d315 on acu:fix-connection-field-required into ea2cd98 on graphql-python:master.

@alexkirsz alexkirsz mentioned this pull request Apr 1, 2019
Merged
phalt
phalt suggested changes Apr 30, 2019
View reviewed changes
Comment thread graphene_django/fields.py Outdated
root,
info,
**args
cls,
Copy link
Copy Markdown
Contributor

@phalt phalt Apr 30, 2019

Choose a reason for hiding this comment

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

8 spaces should be 4!

@alexkirsz
Copy link
Copy Markdown
Contributor Author

alexkirsz commented May 1, 2019

@phalt Done!

phalt
phalt approved these changes May 2, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@phalt phalt left a comment

Choose a reason for hiding this comment

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

This looks good to me - @alexkirsz this isn't a breaking change is it?

@alexkirsz
Copy link
Copy Markdown
Contributor Author

alexkirsz commented May 2, 2019

@phalt The previous behaviour was to raise an exception, so I don't believe there's any way in which this could break non-already broken code :)

@phalt
Copy link
Copy Markdown
Contributor

phalt commented May 2, 2019

@alexkirsz approval from me then!

@phalt phalt merged commit 0d178b3 into graphql-python:master May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mvanlonden mvanlonden mvanlonden approved these changes

+1 more reviewer

@phalt phalt phalt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alexkirsz @coveralls @phalt @mvanlonden