NPE on OptionSpec.getValue()

[rsenden]

For options defined in an ArgGroup, a call to OptionSpec.getValue() will fail due to a NullPointerException if none of the options contained in the ArgGroup have been specified on the command line.

The following command class demonstrates this issue:

@Command(name = "testGetValue")
public class TestGetValueCommand implements Runnable {
	@Spec CommandSpec spec;
	@Option(names="--arg1") private String arg1;
	@ArgGroup private MyArgGroup argGroup;
	
	private static final class MyArgGroup {
		@Option(names="--arg2") private String arg2;
		@Option(names="--arg3") private String arg3;
	}
	
	@Override
	public void run() {
		spec.options().stream().map(OptionSpec::getValue).forEach(this::printOptionValue);
	}
	
	private void printOptionValue(Object value) {
		System.out.println("Option value: "+value);
	}
}

This command will fail with an exception on OptionSpec::getValue if neither --arg2 nor --arg3 have been specified on the command line.

A simple null-check in the FieldBinding#get() method fixes this issue; we will be submitting a PR for this:

@SuppressWarnings("unchecked") T result = obj==null ? null : (T) field.get(obj);

Note that the FieldBinding#set() method likely exhibits the same problem, however this may be more difficult to fix; if an ArgGroup is null, then there's no object to set the option value on. As such, we won't be including any fix for the set() method in the PR.

It might make sense to always initialize ArgGroups with a non-null value, as that would avoid these kind of issues, and would also reduce the number of null checks in command implementations when trying to access options defined in an ArgGroup. Not sure though whether there are any command implementations that rely on the fact that an ArgGroup is null if none of the options in that ArgGroup have been specified on the command line, so this would potentially be a breaking change.

[MikeTheSnowman]

@remkop . Rsenden and myself enountered another issue that we wanted to get your opinion on.

We tried to implement a simple null check, as rsenden has already mentioned. But in doing so, this caused the unit test "testIssue1300BooleanInitialization" (for isue #1300 ) to fail. On investigation, the unit test is failing because the code in ArgSpec#initialValue() expects the get() method to throw an exception for uninitalized arggroups.

I did make an attempt to also update ArgSpec#initialValue() (Link), but I have doubts that this update is appropiate.

So because of this issue, do you have a recommendation on how to proceed?

[rsenden]

@remkop Any suggestions on how to fix this issue? Now, whenever I want to call OptionSpec#getValue(), I have to embed this call into a try/catch block to cater for non-initialized ArgGroups.

Ideally, we should implement proper null checks in FieldBinding, but as mentioned this breaks ArgSpec#initialValue(). I've looked at adding additional methods to FieldBinding, either:

• Having two variants of the get() method; the original method that throws an exception if obj==null, and another that returns null if obj==null (but potentially still throwing an exception in other, real error situations)
• Having an additional isAccessible() method, that allows callers to check beforehand whether a call to either get() or set() would succeed

However, we'd need to add those methods to the IGetter (and ISetter) interface (or introduce a new interface), which will affect various other classes that implement those interfaces.

So, unless we're willing to do some refactoring, I think we're left with the following options:

• Add a try/catch block in OptionSpec#getValue() to have that method return null if an exception occurs
• Add a new OptionSpec#getValueOrNull() method that has this try/catch block

The first option results in a cleaner interface, but could potentially break existing code which expects this method to throw an exception for non-initialized ArgGroups or other error situations. And although easy to implement, neither of these options are ideal; I think it's against best practices to throw and catch exception in non-exceptional situations (I would consider a non-initialized ArgGroup as a common, non-exceptional situation).

What's your opinion on this?

[remkop]

I can try giving a more elaborate answer tomorrow but what comes to mind now is to work with what’s available; maybe something like this:

public void run() {
    spec.options().stream()
        .map(this::safeGetValue)
        .forEach(this::printOptionValue);
}

Object safeGetValue(OptionSpec o) {
    try {
        return o.getValue();
    } catch (NullPointerException npe) {
        return null;
    }
}

[remkop]

Another way to avoid the NullPointerException might be to check if the OptionSpec has a group, and if it does, check if the group is null.
But away from my pc now, not sure what that would look like in code…

[rsenden]

@remkop The safeGetValue(OptionSpec) approach is what I'm using now (although picocli wraps the NPE in another exception, so we can't directly catch the NPE), but this approach is somewhat annoying (and easy to forget) if you are calling OptionSpec#getValue() in multiple places.

In general, I think people would expect this option to simply return null if an option hasn't been given on the command line and no default value is defined, without having to take into consideration that the ArgGroup in which the option has been defined may not have been initialized (just because none of the options in the ArgGroup have been specified on the command line).

So, I think it would be better to either change the behavior of getValue() to not throw an exception when encountering uninitialized ArgGroups, or to add a new OptionSpec#safeGetValue() method (which either catches the exception, or even better, checks beforehand whether the OptionSpec group is not null, as you proposed in your other comment).

[remkop]

I am not a fan of the OptionSpec#safeGetValue() idea. The method name just feels wrong for a library API.

I am not considering modifying the IGetter/ISetter interfaces.

One idea would be to add a method to ArgSpec (that returns a boolean) that can be used to detect if ArgSpec#getValue() can safely be called. Similar to Iterator::hasNext and Iterator::next. Similar to your FieldBinding.isAccessible idea in an earlier comment.

I don't oppose modifying ArgSpec::getValue to not throw an exception. I doubt that applications rely on that exception.
However, I am not convinced that FieldBinding#get() is the best place to fix that. That would result in other bindings like method bindings still throwing the exception and would not be consistent.

Looking at the code, there is no obvious clean way to do this.

Perhaps one idea would be to introduce a new interface IScoped:

interface IScoped {
    IScope getScope();
}

Then, FieldBinding and MethodBinding can implement the IScoped interface.
This can then be used to implement the ArgSpec::isValueGettable (better name suggestions welcome 😅 ) method, with something like this:

// TODO javadoc
public boolean isValueGettable() {
    if (getter instanceof IScoped) {
        IScoped scoped = (IScoped) getter;
        IScope scope = scoped.getScope();
        Object obj = scope.get();
        return obj != null;
    }
    return true;
}

/** Returns the current value of this argument. 
    Returns {@code null} if the getter's scope is null.
    Delegates to the current {@link #getter()}. */
public <T> T getValue() throws PicocliException {
    if (!isValueGettable()) { return null; }
    try {
        return getter.<T>get();
    } catch (PicocliException ex) { throw ex;
    } catch (Exception ex) {        throw new PicocliException("Could not get value for " + this + ": " + ex, ex);
    }
}

Thoughts?

[rsenden]

@remkop Sorry for the late reply, I was occupied with other things. I guess your suggestion should work, but it feels like this exposes too many implementation details from the Binding classes (i.e. the fact that a value is not gettable if the scoped object is null). Why not combine the two ideas?

Create a new interface IAccessible:

interface IAccessible {
    boolean isAccessible();
}

Have FieldBinding and MethodBinding implement the IAccessible interface:

public isAccessible() {
    return scope.get() != null;
}

And finally have getValue() check whether getter implements the IAccessible interface, and if so, return null if the isAccessible() method returns false:

public <T> T getValue() throws PicocliException {
    if ( getter instanceof IAccessible && !((IAccessible)getter).isAccessible() ) { return null; }
    try {
        return getter.<T>get();
    } catch (PicocliException ex) { throw ex;
    } catch (Exception ex) {        throw new PicocliException("Could not get value for " + this + ": " + ex, ex);
    }
}

This way, the implementation for determining whether a value is accessible is nicely encapsulated in the FieldBinding and MethodBinding classes.

Thoughts?

[remkop]

Thanks for helping think this through!

I see the benefits of the IAccessible interface idea and how it fits the use case of avoiding a NPE, but I can also see a future use case where people may actually want to obtain the IScope object itself for another reason than avoiding a NPE.
So I think the IScoped interface idea is a more general solution that solves both this current ticket and future concerns.

Another thing I noticed is that in your comment you mention encapsulating all logic inside the ArgSpec::getValue method.
My previous comment suggested adding a ArgSpec::isValueGettable method, that can be used to distinguish whether the value is actually null or whether its scope is null. Do you prefer not to introduce such a ArgSpec::isValueGettable method?

Thoughts?

[rsenden]

@remkop From an encapsulation perspective, I think the logic for determining whether the getter can be called belongs in classes that implement the IGetter interface. Maybe at some point in the future, the implementation of FieldBinding#get() or MethodBinding#get() is changed in such a way that the logic for determining whether the getter may be called also needs to be changed. From that perspective, it doesn't make sense to implement this logic in ArgSpec.

I've actually already implemented the approach that I described, was just about to commit ;). Using slightly different naming though, as eventually you may want to to have separate isAccessible() methods for getters and setters. New name (for now) is IGetterAccessChecker#isGetterAccessible.

As for the ArgSpec#isValueGettable method, I don't see a need for such a public method. If you think it's better, we can move the instanceof check, and related cast and method call to a separate private method. However, what if getter doesn't implement the new interface? ArgSpec#isValueGettable would then need to return true, but the correct return value would be 'unknown'.

[remkop]

The semantics of the ArgSpec::isValueGettable method is to return true if getValue won’t throw an exception. That should be known.

We introduced this method because we’re thinking of changing the behavior of the ArgSpec::getValue method and isValueGettable allows applications to simulate the old behavior.

About the new interface naming and semantics,
I still like the IScoped idea better than IxxxAcessible since it seems more generic and more future-proof.

[rsenden]

As for the semantics of the ArgSpec::isValueGettable method, this relies on the (implicit) fact that getter is always an instance of IScoped. In theory, if getter is not an instance of IScoped, isValueGettable will return true, and getValue will call getter.get() without knowing for sure whether the value is actually gettable, possibly still resulting in an exception being thrown.

Similarly, deciding whether the value is gettable based on whether the scoped object is null or not, implicitly assumes that the IGetter::get operation will fail if the scoped object is null (and will not fail if the scoped object is not null). That's mostly true for the current FieldBinding and MethodBinding implementations, but potentially this behavior could change in the future, for example if a new Binding class is introduced for which this assumption isn't true.

This is all theory though, in practice the IScoped idea will likely work just as well as the more explicit approach that I proposed, and at the same time is more generic for other use cases where access to the scoped object may be needed. So, unless you have any concerns based on the above, I'll go ahead with adding the IScoped interface and other updates as you suggested.

[rsenden]

@remkop I had already implemented the IGetterAccessChecker approach before, and have now also implemented the IScoped based approach. I submitted two PR's, one for each implementation, so you get to choose and can close the other one ;)

[remkop]

Okay great thank you! 🙏
I’ll take a look.
I’m pretty swamped with other stuff so please give me some time. 😅