antlr / stringtemplate4

StringTemplate 4
http://www.stringtemplate.org
Other
956 stars 231 forks source link

null pointer exception on multiple object adapters #122

Closed richard-melvin closed 4 years ago

richard-melvin commented 9 years ago

Given an inheritance hierarchy of adapted model objects ClassA -> ClassB -> java.land.Object, get a NPE

    at java.lang.Class.isAssignableFrom(Native Method)
    at org.stringtemplate.v4.misc.TypeRegistry.get(TypeRegistry.java:123)
    at java.util.Collections$SynchronizedMap.get(Collections.java:2584)
    at org.stringtemplate.v4.STGroup.getModelAdaptor(STGroup.java:683)
    at org.stringtemplate.v4.Interpreter.getObjectProperty(Interpreter.java:1199)

This is because the logic in TypeRegistry::get for selecting between multiple candidate adaptors can fail when there are more than two involved (candidates.get(i) is null-checked, but not candidates.get(j)). `

sharwell commented 9 years ago

Well, that would be my code! :frowning:

Thanks for taking the time to report this!

Clashsoft commented 4 years ago

Hi @richard-melvin , can you please explain further what the issue is? I put together the following test that is supposed to mimic your situation, but couldn't reproduce the error.

public class TestTypeRegistry {
    // https://github.com/antlr/stringtemplate4/issues/122

    static class A {}

    static class B extends A {}

    @Test
    public void registryWithObject() {
        TypeRegistry<String> registryWithObject = new TypeRegistry<String>();
        registryWithObject.put(Object.class, "Object");
        assertEquals("Object", registryWithObject.get(Object.class));
        assertEquals("Object", registryWithObject.get(A.class));
        assertEquals("Object", registryWithObject.get(B.class));
    }

    @Test
    public void registryWithA() {
        TypeRegistry<String> registryWithA = new TypeRegistry<String>();
        registryWithA.put(A.class, "A");
        assertNull(registryWithA.get(Object.class));
        assertEquals("A", registryWithA.get(A.class));
        assertEquals("A", registryWithA.get(B.class));
    }

    @Test
    public void registryWithObjectAndA() {
        TypeRegistry<String> registryWithObjectAndA = new TypeRegistry<String>();
        registryWithObjectAndA.put(Object.class, "Object");
        registryWithObjectAndA.put(A.class, "A");
        assertEquals("Object", registryWithObjectAndA.get(Object.class));
        assertEquals("A", registryWithObjectAndA.get(A.class));
        assertEquals("A", registryWithObjectAndA.get(B.class));
    }

    @Test
    public void registryWithObjectAndAAndB() {
        TypeRegistry<String> registryWithObjectAndAAndB = new TypeRegistry<String>();
        registryWithObjectAndAAndB.put(Object.class, "Object");
        registryWithObjectAndAAndB.put(A.class, "A");
        registryWithObjectAndAAndB.put(B.class, "B");
        assertEquals("Object", registryWithObjectAndAAndB.get(Object.class));
        assertEquals("A", registryWithObjectAndAAndB.get(A.class));
        assertEquals("B", registryWithObjectAndAAndB.get(B.class));
    }
}
richard-melvin commented 4 years ago

The code is:

                for (int j = i + 1; j < candidates.size(); j++) {
                    if (candidates.get(i).isAssignableFrom(candidates.get(j))) {
                        candidates.set(i, null);
                        break;
                    }
                    else if (candidates.get(j).isAssignableFrom(candidates.get(i))) {
                        candidates.set(j, null);
                    }
                }

It crashes when candidates.get(j) return null, i.e. there are three classes, one of which isn't registered. In your test, the only coverage for more than two has everything registered.

I no longer have the original test case, but I think the failure comes if you have an adapter of Object and B, but not A. Or something like that.

I'd suggest adding tests cases until that bit of code is fully covered.

Clashsoft commented 4 years ago

Here are the other two cases with two registered values:

    @Test
    public void registryWithObjectAndB() {
        TypeRegistry<String> registry = new TypeRegistry<String>();
        registry.put(Object.class, "Object");
        registry.put(B.class, "B");
        assertEquals("Object", registry.get(Object.class));
        assertEquals("Object", registry.get(A.class));
        assertEquals("B", registry.get(B.class));
    }

    @Test
    public void registryWithAAndB() {
        TypeRegistry<String> registry = new TypeRegistry<String>();
        registry.put(A.class, "A");
        registry.put(B.class, "B");
        assertNull(registry.get(Object.class));
        assertEquals("A", registry.get(A.class));
        assertEquals("B", registry.get(B.class));
    }

Also, one with only B:

    @Test
    public void registryWithB() {
        TypeRegistry<String> registry = new TypeRegistry<String>();
        registry.put(B.class, "B");
        assertNull(registry.get(Object.class));
        assertNull(registry.get(A.class));
        assertEquals("B", registry.get(B.class));
    }

That should cover all situations with 3 classes (except empty registry). They were all successful.

Clashsoft commented 4 years ago

I think this can be closed as it can no longer be reproduced.