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

Configuration validation breaks serializer inheritance #2388

Closed
gaffney opened this issue Jan 8, 2015 · 8 comments
Closed

Configuration validation breaks serializer inheritance #2388

gaffney opened this issue Jan 8, 2015 · 8 comments

Comments

@gaffney
Copy link
Contributor

gaffney commented Jan 8, 2015

Hey Tom,

We have quite a few base serializers that have their own methods and fields, etc., and serializer subclasses that use a subset of these fields declared in the parent.

This setup breaks after upgrading to 3.0 due to this particular configuration assertion:

ImproperlyConfigured: Field `blah_field` has been declared on serializer
`MySerializer`, but is missing from `Meta.fields`.

I do not see the point of this assertion, as inheritance worked as expected before, which allowed for cleaner and more extensible serializers. Do you remember why this was added?

Also, is there a clean way to work around this assertion? Currently I have a hack in the top-level serializer to work around this limitation, which makes this aspect of rest_framework to behave how it used to in 2.4:

class BaseSerializer(serializers.ModelSerializer):

  def get_fields(self):
    # hack; see rest_framework issue #2388
    declared_fields     = copy.deepcopy(self._declared_fields)
    meta_field_names    = getattr(self.Meta, 'fields', None)
    new_declared_fields = OrderedDict()

    for field_name in meta_field_names:
      if field_name not in declared_fields:
        continue
      new_declared_fields[field_name] = declared_fields[field_name]

    self._declared_fields = new_declared_fields

    return super(BaseSerializer, self).get_fields()

P.S. For reference, this validation addition was made in this commit: 87734be

@gaffney gaffney changed the title (seemingly unnecessary) configuration validation breaks serializer inheritance Configuration validation breaks serializer inheritance Jan 8, 2015
@tomchristie
Copy link
Member

tomchristie commented Jan 8, 2015

why

Enforcing a single canonical style is a good thing - it's confusing and non-sensical to include a field on the serializer class but not include it in the fields list.

Having said that I don't think I'd really considered the inheritance case, so not sure how we'd best support that.

@gaffney
Copy link
Contributor Author

gaffney commented Jan 8, 2015

Enforcing a single canonical style is a good thing

Yeah, I agree. But also the benefit of being able to use inheritance on serializers is pretty big, so it would be nice if there was a clean work-around so these features/characteristics could co-exist. When I have some spare cycles I will think about how this can be solved.

@tomchristie tomchristie added this to the 3.0.4 Release milestone Jan 12, 2015
@tomchristie
Copy link
Member

tomchristie commented Jan 12, 2015

Added 3.0.4 milestone. We may or amy not actually hit that, but let's at least raise this one as a valid issue for any further 3.0.x releases.

@tomchristie
Copy link
Member

tomchristie commented Jan 13, 2015

it would be nice if there was a clean work-around so these features/characteristics could co-exist.

Agreed.

@gtaylor
Copy link

gtaylor commented Jan 14, 2015

This is tripping us up currently, too. We have to end up making pretty extensive use of inheritance for some of our more complex endpoints.

@tomchristie
Copy link
Member

tomchristie commented Jan 14, 2015

Gotcha. Let's def try to make this happen for 3.0.4 then. Either store state that let's us determine if it's declare on the same class, or else otherwise infer it. Could prob check if the class above has any declared fields and remove those from the check if so. Failing test case pr would prob be a useful start (use Serializers not ModelSerializer there to keep the problem simple.)

@gaffney
Copy link
Contributor Author

gaffney commented Jan 21, 2015

Awesomeness. I see the 3.0.4 milestone is at 100% -- is a release being made soon?

@tomchristie
Copy link
Member

tomchristie commented Jan 21, 2015

No blockers left, yeah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants