Skip to content

CellEditEvent

Jump to bottom
Jeanette Winzenburg edited this page Aug 24, 2021 · 2 revisions

CellEditEvent - Contract and Evolution

While working on cleanup of cell edit related issues, two somewhat related issues bubbled up:

Fixing JDK-8269871

(Unspecified) NPE for null TableView

Failing test:

@Test
public void testNullTable() {
    new CellEditEvent<Object, Object>(null, // null table must not throw NPE
            new TablePosition<>(null, -1, null), editAnyEvent(), null);
}

stacktrace:

   java.lang.NullPointerException: TableView can not be null
at javafx.controls/javafx.scene.control.TableColumn$CellEditEvent.<init>(TableColumn.java:787)

due to

public CellEditEvent(TableView<S> table, TablePosition<S,T> pos,
       EventType<CellEditEvent<S,T>> eventType, T newValue) {
    super(table, Event.NULL_SOURCE_TARGET, eventType);
    if (table == null) {
        throw new NullPointerException("TableView can not be null");
    }
    // ..
}

Options to fix would be

  • either remove the null check

  • or specify that null isn’t allowed

Could not find any reason for the second - the table is passed as source to super and never used internally - decided to go for the first. Doing so produced a failing test in TableColumnTest (see below).

NPE on accessing event state

Failing test:

@Test
public void testNullTablePositionGetTableView() {
    CellEditEvent<String, String> ev = new CellEditEvent<>(table, null, editAnyEvent(), null);
    assertNull("table must be null if pos is null", ev.getTableView());
}

Due to:

public TableView<S> getTableView() {
    return pos.getTableView();
}

Obvious fix - guard against null:

public TableView<S> getTableView() {
   return pos != null ? pos.getTableView() : null;
}

Trip into history

Test failure after fix (in particular, after removing the fast-fail code for table null in constructor)

@Test(expected=NullPointerException.class)
public void defaultOnEditCommitHandlerDealsWithNullTableView() {
    table.getColumns().add(column);
    column.setCellValueFactory(param -> param.getValue().firstNameProperty());
    TablePosition<Person,String> pos = new TablePosition<Person, String>(table, 0, column);
    EventType<TableColumn.CellEditEvent<Person,String>> eventType = TableColumn.editCommitEvent();
    column.getOnEditCommit().handle(new TableColumn.CellEditEvent<Person, String>(
            null, pos, (EventType) eventType, "Richard Bair"));
}

Which looks a bit weird:

  • misleading name: sounds like it would test the default handler, but doesn’t because the NPE is thrown before it reaches the handler

  • friction between name and exception annotation: dealsWith and expecting an exception seems contradictory

Removing the annotation makes the test pass .. but doesn’t really test how the default handler copes with null table on the event: as long as the tablePosition carries a not null table, it will pass even though the handler is misbehaving.

There’s also a similar old test for null TablePosition:

@Test(expected=NullPointerException.class)
public void defaultOnEditCommitHandlerDealsWithNullTablePosition() {
    table.getColumns().add(column);
    column.setCellValueFactory(param -> param.getValue().firstNameProperty());
    TablePosition<Person,String> pos = new TablePosition<Person, String>(table, 0, column);
    EventType<TableColumn.CellEditEvent<Person,String>> eventType = TableColumn.editCommitEvent();
    column.getOnEditCommit().handle(new TableColumn.CellEditEvent<Person, String>(
            table, null, (EventType) eventType, "Richard Bair"));
}

Which passes before/after the fix even though the default handler misbehaves - as can be seen by commenting the annotation and see it failing with

   java.lang.NullPointerException
at javafx.controls/javafx.scene.control.TableColumn.lambda$0(TableColumn.java:293)

pointing to the handler:

private EventHandler<CellEditEvent<S,T>> DEFAULT_EDIT_COMMIT_HANDLER = t -> {
    int index = t.getTablePosition().getRow(); // <-- NPE because tablePosition on event is null
    List<S> list = t.getTableView().getItems();

Both test methods are available since the beginning of modern history - that is the initial commit into mercurial (== open sourcing?). At that time, the event constructor also guarded itself against null tablePosition:

public CellEditEvent(TableView<S> table, TablePosition<S,T> pos,
       EventType<CellEditEvent<S,T>> eventType, T newValue) {
    super(table, Event.NULL_SOURCE_TARGET, eventType);
    // ..
    if (pos == null) {
        throw new NullPointerException("tablePosition can not be null");
    }
    // ..
}

Reinserting the guard makes the test (with removed expected annotation) fail with the stacktrace pointing to the fast-fail code:

   java.lang.NullPointerException: TablePosition can not be null
at javafx.controls/javafx.scene.control.TableColumn$CellEditEvent.<init>(TableColumn.java:790)

The guard against TablePosition was removed while fixing JDK-8120610 - EditingCellProperty is not set to null (old was: RT-31727).

Clone this wiki locally