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

Binder: Add method to write only the changed properties to the bean #18583

Closed
TatuLund opened this issue Feb 1, 2024 · 5 comments · Fixed by #18636
Closed

Binder: Add method to write only the changed properties to the bean #18583

TatuLund opened this issue Feb 1, 2024 · 5 comments · Fixed by #18636

Comments

@TatuLund
Copy link
Contributor

TatuLund commented Feb 1, 2024

It is not uncommon that setters of the bean may have side effects that application developer would not like to get triggered when writing bean with value which is not changed. E.g. This is how jOOQ tracks changes on records and will then generate minimal update statements.

Current implementation is

​public void writeBean(BEAN bean) throws ValidationException {
    BinderValidationStatus<BEAN> status = doWriteIfValid(bean, bindings);
    if (status.hasErrors()) {
        throw new ValidationException(status.getFieldValidationErrors(),
                status.getBeanValidationErrors());
    }
}

  1. One implementation would be to add a new method updateBean.
​public void updateBean(BEAN bean) throws ValidationException {
    BinderValidationStatus<BEAN> status = doWriteIfValid(bean, changedBindings);
    if (status.hasErrors()) {
        throw new ValidationException(status.getFieldValidationErrors(),
                status.getBeanValidationErrors());
    }
}
  1. Another implementation could be refactor and add method boolean parameter
​public void writeBean(BEAN bean) throws ValidationException {
    writeBean(bean, false);
}
​
​public void writeBean(BEAN bean, boolean changedFields) throws ValidationException {
    if (changedFields) {
       BinderValidationStatus<BEAN> status = doWriteIfValid(bean, bindings);
    } else {
        BinderValidationStatus<BEAN> status = doWriteIfValid(bean, changedBindings);
    }
    if (status.hasErrors()) {
        throw new ValidationException(status.getFieldValidationErrors(),
                status.getBeanValidationErrors());
    }
}


Unfortunately changedBindings is private and you can just extend Binder

  1. One could consider also another approach of having methods like
Set<Binding<BEAN, ?>> getChangedBindings() {
    return changedBindings;
}
​public void writeBean(BEAN bean) throws ValidationException {
    writeBean(bean, bindings);
}
​
​public void writeBean(BEAN bean, Set<Binding<BEAN, ?>> bindingsToWrite) throws ValidationException {
    BinderValidationStatus<BEAN> status = doWriteIfValid(bean, bindingsToWrite);
    if (status.hasErrors()) {
        throw new ValidationException(status.getFieldValidationErrors(),
                status.getBeanValidationErrors());
    }
}

​public void updateBean(BEAN bean) throws ValidationException {
    writeBean(bean, changedBindings);
}

This would allow greater flexibility to write any subset of bindings for other edge cases i am not able to figure out now. Having getter for changedBindings could also serve other purposes (see also #15266).

What you would vote for 1., 2. or 3.?

@Legioth
Copy link
Member

Legioth commented Feb 1, 2024

I think we should avoid the boolean parameter since you never remember what true means when you read code like binder.writeBean(person, true).

The difference between 1. and 3. is mostly about implementation effort. 1. is less to implement (and document and test) but 3. is certainly more powerful. 1. is forward compatible which means that we can start with 1. today and then we still have the option to extend that to 3. at any point in the future without breaking changes.

One small additional suggestion to 3. is that updateBean should use getChangedBindings() so that an overridden implementation in a sub class will also affect what updateBean does.

@OlliTietavainenVaadin
Copy link
Member

OlliTietavainenVaadin commented Feb 1, 2024

On naming: I can see that writeBean and updateBean can easily get mixed up; updateBean is not that more distinctive than writeBean(foo, true). How about a more descriptive name, in classic verbose Java style: writeChangedPropertiesToBean or writeChangedBindingsToBean?

@Legioth
Copy link
Member

Legioth commented Feb 1, 2024

I think the distinction is relatively compatible with what I expect to be common mental models. "Overwrite" and "incremental update" are established concepts whereas "overupdate" and "incremental write" are not.

At the same time, a more explicit name might also be useful.

@mshabarov
Copy link
Contributor

I'd go with the option 3 but probably without new updateBean method:

Set<Binding<BEAN, ?>> getChangedBindings() {
    return changedBindings;
}
​public void writeBean(BEAN bean) throws ValidationException {
    writeBean(bean, bindings);
}
​
​public void writeBean(BEAN bean, Set<Binding<BEAN, ?>> bindingsToWrite) throws ValidationException {
    BinderValidationStatus<BEAN> status = doWriteIfValid(bean, bindingsToWrite);
    if (status.hasErrors()) {
        throw new ValidationException(status.getFieldValidationErrors(),
                status.getBeanValidationErrors());
    }
}

// usage example:
writeBean(bean, getChangedBindings());

@OlliTietavainenVaadin
Copy link
Member

Hopefully the getChangedBindings() is either public or protected. Package protection is just annoying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: June 2024 (24.4)
Vaadin Flow enhancements backlog (Vaa...
  
Done / Pending Release
Development

Successfully merging a pull request may close this issue.

4 participants