Skip to content

Fix schema generation for PrimaryKeyRelatedField #5764

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Feb 1, 2018
Merged

Fix schema generation for PrimaryKeyRelatedField #5764

merged 1 commit into from
Feb 1, 2018

Conversation

jlaine
Copy link
Contributor

@jlaine jlaine commented Jan 23, 2018

By default all subclasses of RelatedField are output as string fields in
the schema, which works well for StringRelatedField, SlugRelatedField or
HyperlinkedRelatedField.

Handle the common case of a PrimaryKeyRelatedField pointing to an
AutoField.

By default all subclasses of RelatedField are output as string fields in
the schema, which works well for StringRelatedField, SlugRelatedField or
HyperlinkedRelatedField.

Handle the common case of a PrimaryKeyRelatedField pointing to an
AutoField.
@jlaine jlaine changed the title Detect AutoField foreign keys when generating schema Fix schema generation for PrimaryKeyRelatedField Jan 23, 2018
@jlaine
Copy link
Contributor Author

jlaine commented Jan 24, 2018

On a related note, I noticed that including a foreign key field in "read_only_fields" nukes the "queryset" kwarg:

'validators', 'queryset'

I understand why having this attribute is not required for a read-only field, but what is the harm in keeping it? Nuking this attribute removes the link to the foreign model and makes it much harder to generate a schema for the output of a serializer.

@carltongibson
Copy link
Collaborator

@jlaine Just on your related note: my immediate thought is that if read_only_fields is misbehaving for you is to explicitly declare the field. read_only_fields is a short-cut, it's task is to cover the common-case. The canonical approach is always to explicitly declare the field.

If that doesn't suit then put together a proper description and open a new ticket and we'll have a look.

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Yep. This looks good. Thanks!

@carltongibson carltongibson added this to the 3.8 Release milestone Feb 1, 2018
@carltongibson carltongibson merged commit 27f32fa into encode:master Feb 1, 2018
@jlaine
Copy link
Contributor Author

jlaine commented Feb 1, 2018

Thanks for merging this! Concerning the read_only_fields, explicitly declaring the field doesn't help : there is code to prevent you from providing a queryset on a field you declare as read_only. Not sure what the rationale is:

assert not (self.queryset is not None and kwargs.get('read_only', None)), (

@carltongibson
Copy link
Collaborator

OK. Open a new ticket and we’ll think it through

@jlaine jlaine deleted the foreign-key-schema branch April 9, 2018 06:43
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
By default all subclasses of RelatedField are output as string fields in
the schema, which works well for StringRelatedField, SlugRelatedField or
HyperlinkedRelatedField.

Handle the common case of a PrimaryKeyRelatedField pointing to an
AutoField.
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants