Skip to content

Set up serializer fields lazily on-demand. #1963

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

Merged
merged 1 commit into from
Oct 31, 2014

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Oct 16, 2014

This avoids AppRegistryNotReady problems in Django 1.7 with nested serializers,
which are instantiated at import time, possibly before Django's app registry is
fully populated.

Fixes #1907 and #1943.

@carljm
Copy link
Contributor Author

carljm commented Oct 16, 2014

The "test" here is a bit ugly, since it's hard to effectively test these global-state-setup issues. I'm importing and instantiating a model serializer (with fields) in the pytest_configure hook, before django.setup() is called. Without the fix, this causes the entire test run to blow up :/

I'm not sure how else to effectively test this, but I'll happily remove that "test" if it's considered too ugly.

@xordoquy
Copy link
Collaborator

Thanks a lot. This will definitively help developers porting to Django 1.7.
At first I was a bit afraid it would make the fields attribute impossible to alter but after a look at cached_property this should not be an issue at all.
It looks fine to me. I'm leaving it opened to get another review.

@carljm
Copy link
Contributor Author

carljm commented Oct 16, 2014

@xordoquy Yes, cached_property is really quite simple and clever.

@carljm
Copy link
Contributor Author

carljm commented Oct 16, 2014

Oh, I should explicitly note too that I had to modify a couple other tests asserting that field-configuration errors would be raised on model instantiation, to instead assert that those errors would be raised on first access of the fields property.

@carljm
Copy link
Contributor Author

carljm commented Oct 16, 2014

One other note - the app-registry errors may still bite anyone who accesses self.fields in an overridden serializer __init__ method, and then uses that serializer as a nested field. But at least with this fix they should be able to find a different approach that avoids the problem.

@carljm
Copy link
Contributor Author

carljm commented Oct 16, 2014

@xordoquy I'm not sure why this is labeled an Enhancement when #1907 and #1943, which it fixes, are both labeled Bugs?

@xordoquy xordoquy added this to the 2.4.4 Release milestone Oct 17, 2014
# called, to ensure that model serializers don't eagerly configure fields.
from .serializers import ModelSerializerWithFields
ModelSerializerWithFields()

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd rather just drop this. The hackery required to hard-test the behaviour doesn't seem worth the trade-off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, force-pushed a new single commit without the test hackery.

@tomchristie
Copy link
Member

Looks like we'd need a bit of minor tweaking of 3.0 to support this, but nothing problematic.

This avoids AppRegistryNotReady problems in Django 1.7 with nested serializers,
which are instantiated at import time, possibly before Django's app registry is
fully populated.
@tomchristie tomchristie modified the milestones: 3.0 Release, 2.4.4 Release Oct 31, 2014
@tomchristie
Copy link
Member

Marginally more involved in 3.0 to do this, but resolved there now.

tomchristie added a commit that referenced this pull request Oct 31, 2014
Set up serializer fields lazily on-demand.
@tomchristie tomchristie merged commit 65a0d08 into encode:master Oct 31, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using serializer as field fails at import time (e.g. with unit tests) with django 1.7
4 participants