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 #35350 -- Fixed save() with pk set on models with GeneratedFields. #18058

Merged
merged 1 commit into from Apr 10, 2024

Conversation

sarahboyce
Copy link
Contributor

@sarahboyce sarahboyce commented Apr 8, 2024

Thanks Matt Hegarty for the report.

Regression in f333e35.

Trac ticket number

ticket-35350

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.

Comment on lines +1066 to +1070
non_pks_non_generated = [
f
for f in meta.local_concrete_fields
if not f.primary_key and not f.generated
]
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't have to done here as this is meant as a non-invasive backport but since non_pks_non_generated is only used for containment checks and doesn't need to preserve ordering it might be a good candidate to use a set instead of a list.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

It could also be cached up on Options, it doesn’t need regenerating on every save() call.

@sarahboyce
Copy link
Contributor Author

sarahboyce commented Apr 9, 2024

Note that I have just finished testing this against the test project from @matthewhegarty (thank you for proving that ⭐) and this works for me there too. 👍

Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! 🌟

docs/releases/5.0.5.txt Outdated Show resolved Hide resolved
Thanks Matt Hegarty for the report and Simon Charette and Natalia Bidart for the reviews.

Regression in f333e35.
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Thank you!

@sarahboyce sarahboyce merged commit 8b53560 into django:main Apr 10, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants