Skip to content

Check for None on real_model in get_real_instance #632

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 2 commits into from
May 1, 2025

Conversation

nlsfnr
Copy link
Contributor

@nlsfnr nlsfnr commented Apr 20, 2025

Fixes a bug where PolymorphicModel's get_real_instance() raises an exception when get_real_instance_class() returns None.

@bckohan
Copy link
Contributor

bckohan commented Apr 30, 2025

@nlsfnr how did this situation arise?

@nlsfnr
Copy link
Contributor Author

nlsfnr commented Apr 30, 2025

Slightly weird scenario -- I use Docker containers for local development with a persitent volume for the DB. When switching between branches, where one branch has a new subclass of a polymorphic model that the other branch does not, the ContentType for said subclass still exists in the DB's volume but does not correspond to a Django model in the code base. That means that get_real_instance_class returns None and get_real_instance therefore crashes.

It's more or less equivalent to removing a Django model from the code base without adding the appropriate migration.

@bckohan
Copy link
Contributor

bckohan commented Apr 30, 2025

My only hesitation is that this might hide when something else is very wrong, like having unapplied migrations or other edge cases. I'm new to this project and still not the officially blessed maintainer so I'll revisit this when a decision is made on that.

thanks!

@nlsfnr
Copy link
Contributor Author

nlsfnr commented Apr 30, 2025

Yes good point.

I was mostly matching what was best for my use case, but looking at the implementation of get_real_instance_class, it may be more consistent to raise a PolymorphicTypeInvalid (or a new exception, e.g. PolymorphicTypeUnknown) exception instead.

If downstream applications prefer returning self, they could simply overwrite get_real_instance, which is what I am currently doing in my project.

Happy to change the PR accordingly.

@bckohan
Copy link
Contributor

bckohan commented Apr 30, 2025

PolymorphicTypeInvalid certainly seems appropriate.

@bckohan bckohan merged commit 6347e90 into jazzband:master May 1, 2025
9 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
Development

Successfully merging this pull request may close these issues.

2 participants