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

Fixed #30199 -- Safer docs for QuerySet.get_or_create(). #11017

Merged
merged 1 commit into from May 17, 2019

Conversation

alexbecker
Copy link
Contributor

@alexbecker alexbecker commented Feb 22, 2019

Clarified how get_or_create() relates to the example try/except code,
which has a race condition that get_or_create() exists in part to
prevent.

Made the warning about non-unique kwargs an explicit warning box, moved
it up in the section so more people will read it, and made it more
actionable.

Stopped advising MySQL users to lower the isolation level, and instead
explained the advantage of doing so but also the problems it may cause.
Added links to the relevant MySQL documentation.

@alexbecker alexbecker changed the title Fixes #30199 -- Safer documentation for QuerySet.get_or_create(). Fixes #30199 -- Safer docs for QuerySet.get_or_create(). Feb 22, 2019
@alexbecker alexbecker changed the title Fixes #30199 -- Safer docs for QuerySet.get_or_create(). Fixed #30199 -- Safer docs for QuerySet.get_or_create(). Feb 22, 2019
@carltongibson
Copy link
Member

Hey @adamchainz. Can I just ask for your input (if you have any 🙂) on this?

(In seems a bit nitty-gritty for the QuerySet docs but...)

@carltongibson
Copy link
Member

OK, I'm going to mark this as "needs improvement" because there's at least Tim's point, but also, it looks to me as if Django's default configuration is now largely correct. (Tell me I'm wrong...:)

As such, I think moving this to the DB ref would be better. (Again...:)

I didn't follow-up the "phantom rows" link, but again is that appropriate for the QuerySets docs? (Rather than specific DB notes, if at all.)

As I say, if I missed something, please say.

Copy link
Sponsor Member

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

LGTM overall, just some small tweaks/agreements

docs/ref/models/querysets.txt Outdated Show resolved Hide resolved
docs/ref/models/querysets.txt Outdated Show resolved Hide resolved
@alexbecker
Copy link
Contributor Author

@carltongibson I think I've addressed the review issues. Thanks for the feedback and sorry for letting this PR linger.

docs/ref/models/querysets.txt Outdated Show resolved Hide resolved
docs/ref/databases.txt Outdated Show resolved Hide resolved
@alexbecker
Copy link
Contributor Author

I believe this is ready.

@carltongibson
Copy link
Member

OK, @adamchainz has approved it so I'll mark it RFC — "Ready for checkin" on Trac — (which any reviewer is welcome to do BTW... hint, hint... 🙂) — This'll help me get to it quicker.

Thanks for the effort @alexbecker — I've seen this go past a few times. 🙂

Clarified how get_or_create() relates to the example try/except code,
which has a race condition that get_or_create() exists in part to
prevent, rather than just being more verbose.

Made the warning about non-unique kwargs an explicit warning box, moved
it up in the section so more people will read it, and made it more
actionable.

Removed the advice to change the MySQL isolation level to "READ
COMMITTED", since that is now django's default. Moved the warning about
data loss at "REPEATABLE READ" into the "Isolation level" section of the
MySQL database documentation.
@carltongibson carltongibson merged commit 1686dce into django:master May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants