Table editing mode with BeanItemContainer and BeanContainer works incorrectly

[vaadin-bot]

Originally by damian.czernous@gmail.com

---

1. Bean(Item)Container expects JavaBeans that are mutable by definition.
2. Table to bind JavaBean fields with rows uses hashed collections (HashMap) that prefers immutable objects.

When JavaBean overrides hashcode (& equals) method (e.g. to compare by value) Table can't properly refresh rows after finished editing. During Table.setEditable( false ) execution changed rows disappears.

Editing works fine for JavaBeans that don't override hashcode method.

public class JavaBean
{
  // fields with getters & setters

  @Override
  public int hashCode()
  {
    ...
  }
}

public class SomeUI extends UI
{
  private Table table;
  private Button edit;

  @Override
  protected void init( VaadinRequest vaadinRequest )
  {
    BeanItemContainer container = new BeanItemContaner( JavaBean.class );
    container.addBean( new JavaBean() );

    table = new Table( null, container );

    edit = new Button( "edit", new TurnOnOffEditingAction( table ) );

    // set content
  }
}

public class TurnOnOffEditingAction implements Button.ClickListener
{
    private final Table table;

    // constructor

    @Override
    public void buttonClick( Button.ClickEvent event )
    {
        if( table.isEditable() )
        {
            event.getButton().setCaption( "edit" );
            table.setEditable( false );
        }
        else
        {
            event.getButton().setCaption( "save" );
            table.setEditable( true );
        }
    }
}

---

Imported from https://dev.vaadin.com/ issue #13403

[vaadin-bot]

Originally by @jdahlstrom

---

Thanks. I think IdentityHashMap should be used instead in this case and probably several other places in the Vaadin internals as well.

[vaadin-bot]

Originally by @jdahlstrom

---

This is actually not a defect. Beans whose equals and hashCode change during the object lifetime violate the contract of BeanitemContainer, as documented in the JavaDoc:

 - BeanItemContainer uses the beans themselves as identifiers. The
 - {@link Object#hashCode()} of a bean is used when storing and looking up beans
 - so it must not change during the lifetime of the bean (it should not depend
 - on any part of the bean that can be modified). Typically this restricts the
 - implementation of {@link Object#equals(Object)} as well in order for it to
 - fulfill the contract between {@code equals()} and {@code hashCode()}.

BeanContainer should work, though, as long as the ids are invariant.

[vaadin-bot]

Originally by damian.czernous@gmail.com

---

That's right. if JavaBean needs to be compared by value and uses standard algorithm based on bean's values for calculating hashcode BeanContainer should be used instead.

The question is why BeanItemContainer exists at all.

It's not very intuitive & makes development of the software build on the top of Vaadin more tricky. Not doubt that API docs are useful, but it's better when API itself is more self-descriptive & user friendly. When BeanItemConatainer is used thinking about Vaadin internals such as binding JavaBean with table row is surpassing. Actually also BeanContainer solves problem partially. It asks Vaadin users to deal with Table & JavaBean container internals by requesting BeanIdResolver. From user perspective BeanIdResolver is not needed - it exists to ensure uniqueness of row ids in a table. This sounds like a task for Vaadin.

There is also negative side effect that introduces additional human factor.

To prevent future bugs unit test is required that either:

ensures that JavaBean doesn't override hashcode method. That's something really unusual. or ensures that hashcode algorithm is not generated by IDE, but written by hand. This is also more difficult & not worth the effort.

So, each developer needs to remember about this "feature" every time he defines some JavaBean. Tricky.

Maybe it's worth to mark BeanItemContainer as deprecated in the future release? or Mark both classes as deprecated & provide new one that will handle all internals internally? or Mark BeanItemConatiner as deprecated & provide replacement that will handle internals internally?

[vaadin-bot]

Originally by @jdahlstrom

---

Yes, I considered also changing this to an enhancement ticket, or opening a new ticket for seeing what could be done about this. I think the idea of BeanItemContainer is that even with mutable beans, it is in my experience actually quite common to explicitly decide to have equals and hashCode invariant, depending only on some immutable subset of fields providing an invariant unique identity.This compromise between reference identity semantics and value semantics seems to be an often recommended one, especially in case of DB entity-like objects.

I wonder if it would be actually semantically correct in this case to use IdentityHashMap instead. This needs some investigation.

[vaadin-bot]

Originally by damian.czernous@gmail.com

---

I think IdentityHashMap won't help. It only changes the way comparing is done. HashCode policy remains same. I think it's too straight forward.

Instead of treating bean as an ID use some representation of the bean's data. This representation can be treated internally as the ID, so solution remains same. The only difference is when user calls get from container. Container will return new instance of the bean with current data. So container keeps JavaBean data internally instead real beans. Of course container expects JavaBeans so constructing is not really difficult & can be done with JDK tools or Apache Commons.

Some time ago I wrote this container for bean editor (not beans editor). Vaadin ObjectProperty is my mutable data representation for Fields. This class keeps just original JavaBean's class to instantiate it on demand. Can be used also in a table, because does not override hashcode & equals methods.

public class UpdatableBean<T>
    implements Serializable
{
    private final Class<T> beanClass;
    private final Map<String, ObjectProperty> fields;

    public static <T> UpdatableBean<T> create( T bean )
    {
        return new UpdatableBean<T>( (Class<T>)bean.getClass(), BeanTools.extract( bean ) );
    }

    protected UpdatableBean( Class<T> beanClass, Map<String, ObjectProperty> fields )
    {
        this.beanClass = beanClass;
        this.fields = fields;
    }

    public void update( T bean )
    {
        Map<String, ObjectProperty> properties = BeanTools.extract( bean );
        for( String fieldName : properties.keySet() )
        {
            fields.get( fieldName ).setValue( properties.get( fieldName ).getValue() );
        }
    }

    public T get()
    {
        return BeanTools.construct( beanClass, fields.values() );
    }

    public Map<String, ObjectProperty> fields()
    {
        return Collections.unmodifiableMap( fields );
    }
}

Hope this will help in finding right solution.

[stale[bot]]

Hello there!

It looks like this issue hasn't progressed lately. There are so many issues that we just can't deal them all within a reasonable timeframe.

There are a couple of things you could help to get things rolling on this issue (this is an automated message, so expect that some of these are already in use):

• Check if the issue is still valid for the latest version. There are dozens of duplicates in our issue tracker, so it is possible that the issue is already tackled. If it appears to be fixed, close the issue, otherwise report to the issue that it is still valid.
• Provide more details how to reproduce the issue.
• Explain why it is important to get this issue fixed and politely draw others attention to it e.g. via the forum or social media.
• Add a reduced test case about the issue, so it is easier for somebody to start working on a solution.
• Try fixing the issue yourself and create a pull request that contains the test case and/or a fix for it. Handling the pull requests is the top priority for the core team.
• If the issue is clearly a bug, use the Warranty in your Vaadin subscription to raise its priority.

Thanks again for your contributions! Even though we haven't been able to get this issue fixed, we hope you to report your findings and enhancement ideas in the future too!

[stale[bot]]

Hello there!

We are sorry that this issue hasn't progressed lately. We are prioritizing issues by severity and the number of customers we expect are experiencing this and haven't gotten around to fix this issue yet.

There are a couple of things you could help to get things rolling on this issue (this is an automated message, so expect that some of these are already in use):

• Check if the issue is still valid for the latest version. There are dozens of duplicates in our issue tracker, so it is possible that the issue is already tackled. If it appears to be fixed, close the issue, otherwise report to the issue that it is still valid.
• Provide more details how to reproduce the issue.
• Explain why it is important to get this issue fixed and politely draw others attention to it e.g. via the forum or social media.
• Add a reduced test case about the issue, so it is easier for somebody to start working on a solution.
• Try fixing the issue yourself and create a pull request that contains the test case and/or a fix for it. Handling the pull requests is the top priority for the core team.
• If the issue is clearly a bug, use the Warranty in your Vaadin subscription to raise its priority.

Thanks again for your contributions! Even though we haven't been able to get this issue fixed, we hope you to report your findings and enhancement ideas in the future too!