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

Value.isList accepts List<Object> where only the first element is a Value #286

Closed
javicv opened this issue Feb 8, 2023 · 4 comments · Fixed by #350
Closed

Value.isList accepts List<Object> where only the first element is a Value #286

javicv opened this issue Feb 8, 2023 · 4 comments · Fixed by #350

Comments

@javicv
Copy link
Contributor

javicv commented Feb 8, 2023

public boolean isList() {
return this.innerObject instanceof List
&& (((List) this.innerObject).isEmpty()
|| ((List) this.innerObject).get(0) instanceof Value);
}

This method isList() is used in constructor

public Value(Object value) throws InstantiationException {
but it's checking only the first element of the list. With this check a List<Object> can be added if only the first element is a Value while the other aren't.

In my opinion this test must pass, but it fails because the exception isn't thrown

@Test
public void listMustContainOnlyValues() {
    String item = "item";
    List<Object> list = new ArrayList<>();
    list.add(new Value(item));
    list.add(item);
    try {
        new Value((Object) list);
        fail("Should fail because list contains values and non-values and the exception should be thrown.");
    } catch (InstantiationException e) {
        assertEquals("Invalid value type: class java.util.ArrayList", e.getMessage());
    }
}

Maybe the constructor with Object parameter shouldn't accept lists to force the use of constructor with List<Value> where the list will only contain elements of type Value

public Value(List<Value> value) {
this.innerObject = value;
}

@beeme1mr
Copy link
Member

FYI @justinabrahms @Kavindu-Dodan @toddbaert

@Kavindu-Dodan
Copy link
Contributor

@javicv thank you for noticing this. In my view isList() should be expanded to check for all entries of the collection.

I will work to fix this and add tests to cover the scenario.

@Kavindu-Dodan
Copy link
Contributor

@javicv PR is ready for review - #350

@toddbaert @justinabrahms hope you can review :)

@toddbaert
Copy link
Member

This is my concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants