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

BUG: Properties of inherited models can be used on different child models #3156

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rPraml
Copy link
Contributor

@rPraml rPraml commented Aug 11, 2023

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:

Property prop = DB.getDefault().pluginApi().beanType(Animal.class).property("name");

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 as name)

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

@@ -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);
Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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;
Copy link
Member

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)) {
Copy link
Member

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

Copy link
Contributor Author

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

@rPraml
Copy link
Contributor Author

rPraml commented Aug 22, 2023

@rbygrave I've updated this PR. The check is now perfomed in the setIntercept/getIntercept code paths, but is more strict:

non inherited beans

  • The property must always be fetched from the descriptor of this bean class.
  • There are slight functional changes (edge cases), like fetching Customer.creTime which is owned by the superclass BasicDomain
    • before: it was possible to read/write all instances of BasicDomain
    • now: it will only work on customer:
    • if you fetch a property with db.beanType(Customer.class).expressionPath("cretime"), use it for Customer only (and not for ContactGroup....)
    • it is (currently) not possible to get a "universal" property with db.beanType(BasicDomain.class).expressionPath("cretime") as there is no descriptor.
    • I think this behaviour is acceptable.

inherited beans

  • If there is a inherit hierarchy, you might get properties from the inheritance-root, which are not part of all child models.
  • Reading such a (non existent) property returns null
  • Writing to a property will fail

@rPraml
Copy link
Contributor Author

rPraml commented Oct 17, 2023

@rbygrave I review all my open PRs. This PR is a small one and IMHO ready to review and merge.

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.

None yet

2 participants