Skip to content

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