-
Notifications
You must be signed in to change notification settings - Fork 122
handle default issue with multiple inheritance #687
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
handle default issue with multiple inheritance #687
Conversation
tests/test_hash_model.py
Outdated
@@ -976,3 +976,40 @@ class Child(Model): | |||
|
|||
assert rematerialized.age == 18 | |||
assert rematerialized.bio is None | |||
|
|||
@py_test_mark_asyncio | |||
async def test_grandchild_class_expression_proxy(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests pass on the current state of main, I see what you are getting at though, you are talking about multiple inheritance - so if you try to run this test on main:
@py_test_mark_asyncio
async def test_multiple_inheritance_class_expression_proxy():
class Model(HashModel):
first_name: str
last_name: str
age: int = Field(default=18)
bio: Optional[str] = Field(default=None)
class Sibling():
is_sibling: bool = Field(default=True)
class Child(Model, Sibling):
is_child: bool = True
await Migrator().run()
m = Child(first_name="Steve", last_name="Lorello")
assert m.age == 18
await m.save()
assert m.age == 18
assert m.is_sibling
assert m.is_child
rematerialized = await Child.find(Child.pk == m.pk).first()
assert rematerialized.age == 18
assert rematerialized.age != 19
assert rematerialized.bio is None
assert rematerialized.is_sibling
assert rematerialized.is_child
it will fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is exactly it. I updated the test to reflect that. I wasn't totally understanding what the issue was on our side but now that I do, we can definitely fix it in our implementation. Either way, wanted to leave this up in case it is helpful.
aredis_om/model/model.py
Outdated
model_fields = getattr(bases[base_index], "model_fields", []) | ||
for f_name in model_fields: | ||
field = model_fields[f_name] | ||
print(field) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind deleting this extra print while you are in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to push a commit removing it to your branch but my push was rejected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
This is something we can handle in our implementation but, we wanted to put this up in case it would be helpful to others.
Prior to the pydantic 2.0 change, we were able to define abstract classes as
RedisModel
's and then, in implementation we could define whether we wanted it to be aHashModel
or aJsonModel
. After the change, any defaults we had were coming through asExpressionProxy
objects for the models with the mixins.This fix handles multiple bases on a model to prevent this from happening.