-
-
Notifications
You must be signed in to change notification settings - Fork 263
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
BUG: Properties of inherited models can be used on different child models #3156
base: master
Are you sure you want to change the base?
Conversation
@@ -759,12 +765,17 @@ public Object value(Object bean) { | |||
return getValueIntercept((EntityBean) bean); | |||
} | |||
|
|||
private boolean beanHasProperty(EntityBean bean) { | |||
return descriptor.inheritInfo() == null // fast exit for non inherited beans | |||
|| owningType.isInstance(bean); |
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.
PLEASE CHECK:
the idea is, not to call isInstance
when no inheritance is used (90% of use cases)
BUT there is no barrier somewhere that prevents the user to use a property from model A on model B.
So the question is, should we pay an additional price for the instance check here to make ebean more robust.
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.
This seems reasonable.
Q: The only way to use an incorrect property is via io.ebean.plugin.Property
correct?
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.
We sometimes use ebeaninternal.BeanProperty :-) but there is also ExpressionPath.pathGet/Set in the public API
/** | ||
* Return the value of the property method. | ||
*/ | ||
public Object getValue(EntityBean bean) { | ||
try { | ||
return getter.get(bean); | ||
return beanHasProperty(bean) ? getter.get(bean) : null; |
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.
Technically I don't think this is needed on getValue()
but instead on getValueIntercept()
only
@@ -652,6 +652,9 @@ public void setTenantValue(EntityBean entityBean, Object tenantId) { | |||
*/ | |||
public void setValue(EntityBean bean, Object value) { | |||
try { | |||
if (!beanHasProperty(bean)) { |
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.
Technically I don't think this is needed on setValue()
but instead on setValueIntercept()
only
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, setValueIntercept should be enough in both cases
@rbygrave I've updated this PR. The check is now perfomed in the setIntercept/getIntercept code paths, but is more strict: non inherited beans
inherited beans
|
@rbygrave I review all my open PRs. This PR is a small one and IMHO ready to review and merge. |
36f6c91
to
9d85df0
Compare
Hello @rbygrave
we currently are updating to 13.20 and noticed, that #3057 has triggered a bug.
What we do is, that we fetch a property of an inherited root, but the property itself lives in one child node:
As this property seems to be "animal" compatible, we use
prop.value(animal)
to read the property value of (any) animal.But if the animal is a dog, and the property comes from cat, you are just reading the same property index from an other bean.
(Note
DB.getDefault().pluginApi().beanType(Animal.class).property("registrationNumber")
has the same property index asname
)before #3057 this works by luck in our use case, as the property was unloaded and so, it just returns null.
Now as a lazy load occurs, we were wondering, why we get a value, where we do not expect one
The PR provides a test and a suggested fix