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

prefetch_related_objects fails to cache UUID FKs when the string representation of a UUID is used #18132

Closed
wants to merge 4 commits into from

Conversation

selcuk
Copy link

@selcuk selcuk commented May 5, 2024

Trac ticket number

ticket-35434

Branch description

Added unit test to reproduce the issue in the title.

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.
  • I have attached screenshots in both light and dark modes for any UI changes.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Thanks for the patch.

Had a look at whether the model with ManyToManyField could be used to test this and didn't find a way, so I think the new test model is fine.

One suggested change to clarify that the underlying functionality still works (and not merely doesn't crash).

Also verified this fixes the issue I encountered in my project (mentioned on ticket) 🚀 .

tests/prefetch_related/test_uuid.py Outdated Show resolved Hide resolved
Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>
@jacobtylerwalls
Copy link
Member

I can mark this "ready for checkin" on Trac if you can address two things from the patch review checklist:

  • commit message & pr title: use ticket number and period, like: Fixed #XXXXX -- Avoided bad thing.
  • release note in docs/releases/5.0.5.txt. Although it's not a regression in a recent version (I had this problem on 4.2), ObjectDoesNotExist inappropriately escaping qualifies as a "crashing bug" under my understanding of the backport policy. Someone may correct me on this point.

@sarahboyce
Copy link
Contributor

Based off the ticket discussion, I think this PR won't be accepted

@sarahboyce sarahboyce closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants