Start using Optional for return values when they can be null

Development
#1

Hi devs,

I’d like to propose that we start using Optional as return values in our APIs, in order to make it clear the user that the value can be null.

See https://dzone.com/articles/java-8-optional-usage-and-best-practices

For example for the User API this would mean:

public interface UserProperties extends ConfigurationSource
{
    /**
     * @return true if the user is configured to display hidden documents in the wiki
     */
    boolean displayHiddenDocuments();

    /**
     * @return true if the user is active in the wiki. An active user can log in.
     */
    boolean isActive();

    /**
     * @return the first name of the user or null if not set
     */
    Optional<String> getFirstName();

    /**
     * @return the last name of the user or null if not set
     */
    Optional<String> getLastName();

    /**
     * @return the email address of the user and null if not set
     */
    Optional<String> getEmail();

    /**
     * @return the type of the user (simple user, advanced user)
     * @see <a href="https://bit.ly/37TUlCp">user profile</a>
     */
    UserType getType();

    /**
     * @return the default editor to use when editing content for this user (text editor, wysiwyg editor)
     */
    Editor getEditor();
...

WDYT?

Thanks

#2

That’s definitely a great idea. When I noticed that XWiki codebase doesn’t use it, I was kinda surprised. So yes, this is a great move.

Also, give this guarantee that if a method returns T, not Optional<T>, then such a function must NOT return null for a T, as it defeats the purpose of returning Optional<U> for some other API. In the example, getType() and getEditor must not return null in any case.

In other words, no function shall return null.

I wish I could also contribute refactoring the codebase.

#3

Good idea (for new APIs). For existing API it’s going hard to move them to use Optional since that would break backward compatibility.

Yup

And you can, it’s open and community-based development! :slight_smile:

You’re most welcome. You can talk with the xwiki dev community on our Matrix channel (there’s an IRC bridge too), see https://dev.xwiki.org/xwiki/bin/view/Community/Chat

See also https://dev.xwiki.org/xwiki/bin/view/Community/Contributing

Thanks for answering and providing feedback. Keep it up!

#4

+1

#5

+1 but not sure why it needs a proposal: you plan to enforce that everywhere in the API? using a checker?

#6

I hesitated between a proposal and a VOTE.

Once we agree, it means any new API from now on that can return null must use an Optional.

PS: The fact that there’s an automated check or not is a different topic. What’s important is that we agree about the strategy.

#7

It would be nice but hard to find one and impossible that it would work 100% of the time :slight_smile: Sonarqube does this kind of analysis.

#8

One detail I didn’t mention, based on the user api example, is that it means that from Velocity, instead of calling:

Email is: $services.user.poperties.email

We would need to use something like:

Email is: $services.user.poperties.email.orElse('john@doe.com')

or

#set ($email = $services.user.poperties.email)
#if ($email.isPresent())
...
#else
...
#end

Which means we won’t be able to use the following (AFAIK, since it would throw a NoSuchElementException which is not handled in a special way by Velocity):

#if ("$!services.user.poperties.email.get()" != '')
...
#end

PS: That’s until Velocity gets some special support for Optional. I don’t think they have that in 2.x but maybe, I didn’t check TBH.

#9

One thing that is very important to understand is that Optional.get is unsafe.
Using this method without checks only replaces NullPointerException by NoSuchElementException.

The use of isPresent before calling get, or even using the other provided operations (e.g. orElseGet, map) must be done as much as possible.

#10

Definitely!

#11

+1

Thanks,
Marius

#12

ok seems everyone who’s answered agree so let’s make it our best practice from now on to use Optional in new APIs and to retrofit old one when we can.

I’ll document it today.

#13

Stuff like ifPresent and orElse are nice helpers (but totally unusable in Velocity for example) but I’m not really convinced it’s enough to make it mandatory (definitely a bad idea for script services IMO). Note that I did not had to manipulate Optional in any of the Java libraries I used so far.

#14

It not just that, it’s a way to cleanly express to api user that some method can return null (without them having to rely on not up todate or incomplete javadoc). And yes it allows to write stuff without having checking for null everywhere. You can chain lambdas nicely and remove the need for IFs.

#15

TBH after I did my research I haven’t seen any counter argument to not using Optional for return values that can be null. There are arguments to not use it elsewhere but I haven’t see that for returns.

Now there’s velocity for which we need to decide. It’s not a real problem since using isPresent() or get() will work fine. It just makes it a bit more complex/unnatural to use. Hence, my idea that we could have an uberspector (or modify our existing one) that would call the get() automatically if what is returned is Optional, and return null if the value is null. That should be doable.

#16

That’s enough of a problem to not enforce it…

#17

Definitely not. There’s no reason we shouldn’t use Optional in our Java API that are not scripting APIs .

And for Velocity, I’ve proposed something but you didn’t reply to it.

#18

Where did I say we should never use Optional ? I don’t agree with making it mandatory.

What you proposed does not exist right now.

#19

Why not? What problems do you see for objects not used in scripting APIs?

And? There’s no law that forbids to add it…

#20

FWIW here’s an example of using Optional:

(findFirst() returns an Optional).