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

allow overriding of fields in subclassed serializers. #1053

Closed
wants to merge 3 commits into from

Conversation

craigds
Copy link
Contributor

@craigds craigds commented Aug 21, 2013

Django allows overriding of fields (on forms, not on models), so it seems DRF could too.

It mostly works; the problem is the order the fields get passed to the SortedDict causes them to be "un-overridden" by the superclass fields again.

I will add tests, if this is otherwise OK.

@tomchristie
Copy link
Member

Thanks!
Needs a test to ensure we don't regress the behaviour at some point.
Also, not sure I get it at first glance, and test would help with review.

@craigds
Copy link
Contributor Author

craigds commented Aug 21, 2013

I added some tests for various cases, which now pass. My initial fix actually broke things but that's fixed now.


# if there are fields in both base_fields and fields, SortedDict
# uses the *last* one defined. So fields needs to go last.
return SortedDict(base_fields + fields)
Copy link
Member

Choose a reason for hiding this comment

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

Still struggling to review. C'mon brain. :-/

@tomchristie
Copy link
Member

One possible issue is that this allows overriding Serializers and gets the fields right, but doesn't allow overriding of Meta options, which is non-obvious. It could go in anyway, but might be good if we could comprehensively fix serializer overriding?

@craigds
Copy link
Contributor Author

craigds commented Aug 28, 2013

You're saying that... if I define a MySerializerBase.Meta with some options, I can't then define a MySerializerSubclass.Meta that overrides the options?

I haven't really used Meta much but I guess I can work up a test case for that.

@craigds
Copy link
Contributor Author

craigds commented Aug 28, 2013

Seems like it should be overridden via the normal class mro; I don't see any stuff in the metaclass that messes with Meta particularly.

If you want to inherit the Meta options from a base serializer, I think you do have to subclass the base serializer's Meta:

class MySerializerSubclass(MySerializerBase):
    class Meta(MySerializerBase.Meta):
         extra_options = ...

That's ... slightly unexpected since Django doesn't quite do it that way for models (some options get inherited automatically, others don't). But that seems unrelated to this issue to me.

@craigds
Copy link
Contributor Author

craigds commented Aug 29, 2013

Actually, that Meta overriding thing is consistent with Django's forms: https://docs.djangoproject.com/en/dev/topics/forms/modelforms/#form-inheritance

(just not consistent with models)

@xordoquy xordoquy self-assigned this Apr 30, 2014
@xordoquy xordoquy removed their assignment Jun 16, 2014
@tomchristie
Copy link
Member

We've made the decision to close of all currently open serializer tickets, pending the 3.0 release.

The 3.0 release will involve a major redesign and improvement of the current serializer implementation and should invalidate the large majority of these outstanding tickets.

We will be reviewing the closed tickets prior to the 3.0 launch and try to ensure that we have fully covered any long-standing issues, and adequately dealt with the outstanding problems.

If your issue does not appear to be addressed by the upcoming 3.0 release please do comment on your ticket with the current status, and we can reopen any valid tickets as required. In particular, if your ticket represents a long-standing or fundamental problem please do make sure to follow the mailing list and review any 3.0 pre-release candidates where possible.

Note that you can review the closed tickets with the following searches:

Serializer tickets: https://github.com/tomchristie/django-rest-framework/issues?q=label%3ASerializers
Writable nested serializer tickets: https://github.com/tomchristie/django-rest-framework/issues?q=label%3A%22Writable+nested+serializers%22

Many thanks for your understanding and here's looking forward to a new and improved version, and a cleaner more actionable issue list.

All the best,

Tom

@tomchristie tomchristie added this to the 3.0 pre-release reassessment milestone Aug 19, 2014
craigds added a commit to koordinates/django-rest-framework that referenced this pull request Dec 4, 2015
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request Dec 4, 2015
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request Jan 6, 2016
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request Jan 20, 2016
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request Apr 22, 2016
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request Dec 16, 2016
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request Feb 23, 2017
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request Feb 24, 2017
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request May 9, 2018
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request May 9, 2018
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request Jun 14, 2018
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request Jun 14, 2018
This is an updated version of encode#1053 applied to current master.
hamishcampbell pushed a commit to koordinates/django-rest-framework that referenced this pull request Oct 8, 2018
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request Jun 25, 2019
This is an updated version of encode#1053 applied to current master.
craigds added a commit to koordinates/django-rest-framework that referenced this pull request Jun 1, 2021
This is an updated version of encode#1053 applied to current master.
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.

3 participants