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

detach_object_resources now works for deferred objects #974

Merged
merged 1 commit into from
Dec 8, 2016

Conversation

linzjax
Copy link
Contributor

@linzjax linzjax commented Dec 6, 2016

Proposed changes in this pull request

When should this PR be merged

Whenever, unless this is a big enough bug that it needs to be added before releasing to platform.

Risks

None

Follow up actions

None

@amplifi amplifi added this to the Sprint 11 milestone Dec 7, 2016
Copy link
Member

@oliverroick oliverroick left a comment

Choose a reason for hiding this comment

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

Just left to commends about querying the database with Django (nothing to change in this PR; just some general advice). Please ignore if you know this already.

assert ContentObject.objects.filter(
object_id=party.id, resource=resource).exists()

party_deferred = Party.objects.all().defer('attributes')[0]
Copy link
Member

Choose a reason for hiding this comment

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

Just a little hint about one of the quirks of Django: In this case, it's better to write Party.objects.defer('attributes').first(). When you call Party.objects.all() the ORM gets all Party records from the database, even though you only want the first one. So when you have a huge table, your query wastes resources because the database needs to compile the records and send them across the network, which takes longer the bigger your dataset is. Party.objects.defer('attributes').first() gets only one record instead. (Nothing you need to change now because it doesn't make a big difference in this case, but something to keep in mind when constructing queries.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not know that, that's really good to know. Thanks!

party_deferred.delete()
assert not ContentObject.objects.filter(
object_id=party.id, resource=resource).exists()
assert Party.objects.all().count() == 0
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the above: If you want to check whether there is a record or not, it's better to use Party.objects.exists() because that just checks if it can find one record in the table, whereas Party.objects.all().count() has to find all the records first to count them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also did not know you could use exists when you weren't trying to get a specific record. Neat!

@amplifi amplifi force-pushed the bug/#803-detach-resources branch from 7d0fcc7 to d3d1859 Compare December 8, 2016 08:14
@amplifi amplifi merged commit 4cba99a into master Dec 8, 2016
@amplifi amplifi deleted the bug/#803-detach-resources branch December 8, 2016 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants