Table.ColumnGenerator / IndexedContainer Table.addItem() bug

I hope this is the right forum for bug reports. This is my first post :slight_smile:

I noticed that ItemContainer and BeanItemContainer behavior is inconsistent when overriding a property with a custom ColumnGenerator. In the code sample below you can see that BeanItemContainer works without modification as expected. However when manually adding items using the ItemContainer having a custom ColumnGenerator attached BEFORE items are added to the table results in an empty table (adding the item is ultimately rejected).

See this code below and set it to false to observe the empty table behavior:
// this fails if preload is set to FALSE!
final PersonTable badGuys = new CustomDateTable(items, true);

The root cause appears to be the following code in ‘com.vaadin.ui.Table’:

public Object addItem(Object cells, Object itemId)
throws UnsupportedOperationException {

    // remove generated columns from the list of columns being assigned
    final LinkedList<Object> availableCols = new LinkedList<Object>();
    for (Iterator<Object> it = visibleColumns.iterator(); it.hasNext();) {
        Object id = it.next();
        if (!columnGenerators.containsKey(id)) {
            availableCols.add(id);
        }
    }
    // Checks that a correct number of cells are given
    if (cells.length != availableCols.size()) {
        return null;
    }

If the items are added prior to installing a custom ColumnGenerator everything works as expected. However if the ColumnGenerator is added prior to adding the items it will fail due to the check for ‘cells.length != availableCols.size())’ . The availableCols list would only contain a single entry, i.e. ‘name’ so a call such as
goodGuys.addItem(new Object { austin.getName(), austin.getBirthdate() }, 1L);

would cause it not to add the items to the container and returns null (aside: an exception or at least a log entry seems like a better alternative). IMHO a better check would be to ensure that cells.length is equal to the visible columns. There should always be the option to override the columns presentation.

I would appreciate if you could confirm if this is a bug or if I am doing something wrong.

Thanks!

import java.io.Serializable;
import java.text.DateFormat;
import java.util.Arrays;
import java.util.Date;
import java.util.List;

import com.vaadin.Application;
import com.vaadin.data.Item;
import com.vaadin.data.Property;
import com.vaadin.data.util.BeanItemContainer;
import com.vaadin.ui.Table;
import com.vaadin.ui.Window;

public class TableTestApplication extends Application {

public static class Person implements Serializable {
	private static final long serialVersionUID = 7330952645913510299L;
	
	private final String name;
	private final Date birthdate;
	
	public Person(String name, Date birthdate) {
		this.name = name;
		this.birthdate = birthdate;
	}
	
	public String getName() {
		return name;
	}
	
	public Date getBirthdate() {
		return birthdate;
	}
}

private static class DateColumnGenerator implements Table.ColumnGenerator {
	private static final long serialVersionUID = 4042328102096361804L;
	
	// FIXME java.text.DateFormat is not thread safe (for better alternative see org.apache.commons.lang.time.FastDateFormat)
	private DateFormat dateFormat;
	private Object propertyId;
	
	public DateColumnGenerator(Object propertyId) {
		this(propertyId, DateFormat.getDateInstance(DateFormat.SHORT));
	}
	
	public DateColumnGenerator(Object propertyId, final DateFormat dateFormat) {
		this.propertyId = propertyId;
		this.dateFormat = dateFormat;
	}

	public Object generateCell(Table source, Object itemId, Object columnId) {
		if (itemId != null) {
			Item item = source.getItem(itemId);
			Property property = item.getItemProperty(propertyId);
			if (property.getType() == Date.class && property.getValue() != null) {
				return dateFormat.format(property.getValue());
			}
		}
		return null;
	}
}

private static class PersonTable extends Table {
	
	private static final long serialVersionUID = 2880170015289873665L;

	public PersonTable() {
		setCaption("PersonTable");
		setWidth("100%");
		setSelectable(true);
		addContainerProperty(NAME_PID, String.class, null);
		addContainerProperty(BIRTHDATE_PID, Date.class, null);
	}
}

private static class CustomDateTable extends PersonTable {
	private static final long serialVersionUID = 183015136483518965L;

	public CustomDateTable(List<Object[]> items, boolean preload) {
		super();
		setCaption("CustomDateTable");

		if (preload) {
			load(items);
		}
		addGeneratedColumn(BIRTHDATE_PID, new DateColumnGenerator(BIRTHDATE_PID));
		if (preload == false) {
			load(items);
		}
		
		setVisibleColumns(new Object[] {NAME_PID, BIRTHDATE_PID});
	}

	private void load(final List<Object[]> items) {
		int row = 0;
		for (Object[] item : items) {
			// {@link com.vaadin.ui.Table.addItem(Object[], Object)} 
			// will return null due to mismatch between expected size of
			// Object[] and non-generated columns
			Object itemId = addItem(item, row++);
			if (itemId == null) {
				System.out.println("addItem() failed!");
			}
		}
	}
}

private static class PersonBeanTable extends Table {
	private static final long serialVersionUID = 2322663178312441396L;

	public PersonBeanTable(final BeanItemContainer<Person> personContainer) {
		setCaption("PersonBeanTable");
		setWidth("100%");
		setSelectable(true);
		setContainerDataSource(personContainer);
		addGeneratedColumn(BIRTHDATE_PID, new DateColumnGenerator(BIRTHDATE_PID));
		setVisibleColumns(new Object[] {NAME_PID, BIRTHDATE_PID});
	}
}

private static final long serialVersionUID = -282148020873816151L;

public static final Object NAME_PID = "name";
public static final Object BIRTHDATE_PID = "birthdate";

@Override
public void init() {
	final Person austin = new Person("Austin Powers", new Date(System.currentTimeMillis() - 365*30*1000*60*60*24));
	final Person nigel = new Person("Nigel Powers", new Date(System.currentTimeMillis() - 365*80*1000*60*60*24));
	final Person foxxy = new Person("Foxxy Cleopatra", new Date(System.currentTimeMillis() - 365*20*1000*60*60*24));
	final Person drEvil = new Person("Nigel Powers", new Date(System.currentTimeMillis() - 365*32*1000*60*60*24));
	final Person mrBigglesworth = new Person("Mr. Bigglesworth", new Date(System.currentTimeMillis() - 365*8*1000*60*60*24));
	
	final Window mainWindow = new Window("Generated test");
	
	// this works
	final PersonTable goodGuys =  new PersonTable();
	goodGuys.addItem(new Object[] { austin.getName(), austin.getBirthdate() }, 1L);
	goodGuys.addItem(new Object[] { nigel.getName(), nigel.getBirthdate() } , 2L);
	goodGuys.addItem(new Object[] { foxxy.getName(), foxxy.getBirthdate() } , 3L);
	mainWindow.addComponent(goodGuys);
	
	// this works 
	final BeanItemContainer<Person> personContainer = new BeanItemContainer<Person>(Person.class);
	personContainer.addAll(Arrays.asList(drEvil, mrBigglesworth));
	final PersonBeanTable personBeanTable = new PersonBeanTable(personContainer);
	mainWindow.addComponent(personBeanTable);

	List<Object[]> items = Arrays.asList(
			new Object[] { drEvil.getName(), drEvil.getBirthdate() },
			new Object[] { mrBigglesworth.getName(), mrBigglesworth.getBirthdate() }
	);
	
	// this fails if preload is set to FALSE!
	final PersonTable badGuys =  new CustomDateTable(items, true);
	mainWindow.addComponent(badGuys);
	
	setMainWindow(mainWindow);
}

}