[FEATURE] Collection field marked as @Singular should be able to accept null

[KieferSkunk]

The @Singular annotation is an amazingly powerful feature, and it's great especially for dealing with test code where we have to construct objects that contain maps and lists. However, we have use cases where it's appropriate (and sometimes required) that a collection be explicitly null, and the @Singular annotation makes that impossible currently.

  @Builder
  public class Person {
    private final String name;

    @Singular 
    private final List<String> aliases;
  }

  // ...

  public void doSomething() {
    Person person = Person.builder()
      .name("Kiefer")
      .aliases(null)  // <-- Causes NPE
      .build();

    // Desired outcome:
    // person.getAliases == null
  }

We ran into a severe regression in a part of our code where we made use of @Singular to optimize tests that otherwise had to go through a lot more work to create collections and pass them in to the builder. Because @Singular causes the "plural" form of the collection builder method ("aliases" above) to reject null, a part of our code had to be updated to substitute an empty collection, and this triggered a bug in some older deployments of our environments that handled the empty collection differently from null. Since we cannot easily go and patch those environments to fix the bug, we have to stop using @Singular on the class in question and go back to manually constructing collections in the test code.

It should be possible to explicitly set a @Singular collection to null in the builder and get null as the result, with appropriate error handling for if you then try to call the singularized method. E.g.:

  public void doSomething() {
    Person person = Person.builder()
      .name("Kiefer")
      .aliases(null)
      .alias("A different name") <-- ERROR, collection is null.
      .build();
  }

[KieferSkunk]

To add some more context: While I agree that in general, null and empty collection should be treated the same way (and in most cases they are), when we find a bug in our code, we can't just automatically go and fix it for everyone - we have customers that cannot migrate to more recent deployments for various reasons, but still depend on published objects from a centralized system that is more up to date than their deployments. As a result, Lombok's default behavior with the @Singular annotation actually causes backwards-incompatible changes when used in a context where it hadn't been used previously.

Also, since we serialize our objects via JSON, setting these collections to null when they don't contain any elements is both a memory and serialization optimization. @Singular currently makes that optimization impossible - it results in extra memory allocations for empty lists, and occupies extra bytes in the JSON payload - for especially large payloads, this can actually add up quite a bit.

As it stands right now, the only acceptable course of action is to stop using @Singular anywhere that I want to be able to accept a null, which defeats the purpose of having that annotation in the first place.

[KieferSkunk]

Also left a comment in #819 explaining why @Singular actually represents a breaking change to existing code if someone decides to add it to a class that's already been in use for a while (the issue I ran into).

[cezar-tech]

I think it would be a good idea to create a parameter for the annotation to allow nulls optionally, as in some places one wouldn't want to have nulls going around inside thee collections.

[KieferSkunk]

@cezar-tech - Just to be clear, this is about making it so calling the PLURAL version of the builder method with null would be acceptable (so that the collection itself is null), NOT about whether you can insert a null value into the collection.

[FatCash]

Voting for this issue, as it causes NPE when using JacksonDeserialize with builder pattern.

[rzwitserloot]

A parameter to work around a scenario where code treats a null collection different from an empty one?

No, we don't cater to obviously outdated, badly in need of an update weirdo code.

In general, the 'eh just add a parameter' mindset is unlikely to be accepted; we are quite sensitive to growing the param list to hundreds of entries.

I can see a change where .plural(null) is allowed. It still is real weird though, and I think that causes more harm than good, because it's so unclear what that means: the .plural(collection) method ADDS each element in the provided collection to the builder, it does not REPLACE, hence, accepting nulls doesn't really make sense.

Can't you just work around this stuff by manually writing the getter for the collection field, and explicitly returning null if it is empty?

[KieferSkunk]

@rzwitserloot - In retrospect, the specific scenario I found myself in that prevented the use of Singular is indeed a silly one-off, and not one I expect you to address specifically. However, I still believe that allowing developers to use Singular while still being able to set the collection itself to a specific instance (or to null) has value, and that it can be done cheaply and in a very user-friendly way.

First off, if nothing else, could we get something other than a generic NullPointerException in this case? Perhaps a message like "Cannot pass a null array/collection into a Singular builder"? This would make Lombok much easier and friendlier to use, and it would improve debugging significantly.

Second, it still would be nice to support a case where you CAN set a collection explicitly while still getting the benefits of the Singular builder. The two suggestions I have for this are: Either make it possible to set the default state of the field until a value is added to it (e.g. make it null or THE empty collection, etc.), or add another method like "replace/setMyCollection" that can replace the field reference. Singular already adds "clearMyCollection()" to clear the contents of a collection, so it should be reasonably easy to also include one where the collection itself is replaced. And this wouldn't need to break the contract of the existing methods.

Being able to explicitly set (or default) an instance to have an empty, immutable collection like java.util.Collections.emptyList() would be a benefit - among other things, it can allow us to save on allocations in cases where we know ahead of time that the object in question will never have any values in the collection. The Singular as it is right now allocates a new collection for every object built, which may be fine as a default behavior. But I feel it would be very helpful to your users to give us the option of overriding that default behavior without forcing us to avoid the feature altogether.

Thank you for your time.

[rzwitserloot]

First off, if nothing else, could we get something other than a generic NullPointerException in this case? Perhaps a message like "Cannot pass a null array/collection into a Singular builder"? This would make Lombok much easier and friendlier to use, and it would improve debugging significantly.

The case being, specifically, passing null to the plural(Collection<? extends T> collectionToAddFrom) method, right? Yeah that's a great idea!

As for a replace option.. I can see that, too. The issue with set is that you can make your builder's setters all already start with set, and then you'd have... setSetNames which sounds suboptimal. I think we need to ignore configged prefixes for this (let's say it is with, I'd think you want setNames or replaceNames, not setWithNames or replaceWithNames.

replace is more palatable; I doubt replace as a prefix is at all common.

It is, however, quite complex to let you replace the entire thing. The field that backs a singular-collection need not be of the type at all; for example, maps have 2 lists as backing collection!

Thus, the creation of this method would require 4 fields in the builder for a single field: a list for the keys, a list for the values, a boolean to know that an explicit map was provided, and a field of type map, for this default (and we can't use null as flag, as you want to be able to call replaceNames(null) and have that work. Then there's the further issue of what we do with any furhter calls to, say, name() (the singular method, or the adding plural one). I'm pretty sure we just have to crash on this (runtime exception) which is a bit ugly.

There is absolutely no need whatsoever to being able to explicit set an instant to have an empty, immutable collection – you already get this with @Singular today! – an immutable empty collection is exactly what you get already.

Lombok is extremely intelligent and efficient about this stuff: If you don't call the singular method at all, we don't even make those lists, we leave em as null, and then call Collections.emptyList() (which just returns a global singleton; it's a method invoke solely to sort out some generics, its impl is literally return EMPTY_LIST;) – and your confusion on this topic makes me hesitate more to add a replace.

Effectively, then, there is really only one open use-case, which is leaving the field as null if no elements are added. I am loathe to cater to this, for all the reasons mentioned so far.

[rzwitserloot]

@FatCash I guess the way to solve that deserializer issue is if .plural(null) works and silently does nothing? I guess then we sort of break the 'contract', so to speak, because we are now 'deserializing' a nullref into an immutable empty collection instead.

[rzwitserloot]

I wrote down a full proposal: https://github.com/rzwitserloot/lombok/wiki/FEATURE-PROPOSAL:-null-checking-@Singular - please provide feedback.

[KieferSkunk]

I think that proposal looks great. Thank you for considering it. :)

And thanks for the explanation on why that replace function may be difficult to implement. Ultimately, it is a convenience feature I'm asking for. (I was also unaware that you provide empty collections when not adding anything to them - that's great.)

The case being, specifically, passing null to the plural(Collection<? extends T> collectionToAddFrom) method, right? Yeah that's a great idea!

Yep, that's what I was referring to. If my collection is named things, then calling myBuilder.things(null) should respond with an explanatory message. I believe you already handle the singular myBuilder.thing(null) case.

[KieferSkunk]

Alternatively, can we consider simply null-checking the input and treating myBuilder.things(null) as equivalent to myBuilder.things(Collections.emptyList())? It seems that calling this with null is effectively "I have nothing to add".

[rzwitserloot]

@KieferSkunk is it? See, that's what I was wondering. In my proposal, it is configurable, but defaults to nullpointerexception (WITH a nice message). The general tendency to go 'null? Oh.. just... silently do nothing' is not generally a good idea. Perhaps 9 out of 10 times that is the intended behaviour, but that 10th time is a doozy: Silently doing nothing is, literally, on the order of 100x worse than throwing an exception, because they are that much harder to find. A large part of the 'cost' of writing a software bug is finding out about it, so making that very hard (which 'silently ignore null' does!) is an extremely bad plan. Hence my hesitance in just defaulting to that behaviour.

[KieferSkunk]

This is one of those areas where newer languages like Go and Kotlin are actively trying to deal with this sort of problem, by making it much harder to get into situations where NullPointerException-type errors are even possible. While Java might favor fully-explicit "Nulls are always bad", I feel like we have an opportunity to start moving the needle a bit on that. Especially given that so many third-party libraries (and even some built-in Java functions) exist to work around the fact that collections can be null - e.g. SpringFramework's CollectionUtils.isEmpty() method, which checks for both null and empty.

I don't know about other developers, but in situations where I want to add or copy values from one collection to another, do a transformation, etc., null and empty collections are always equivalent - as in, "The source collection has no entries". It's a fact of life that these two have to be actually treated differently because of Java's C-style management of null pointers, but as a third-party library specifically designed to make Java easier to work with, I feel like Lombok is in a good position to deal with this.

(And I appreciate that you're proposing to make this configurable.)

[rzwitserloot]

Kotlin and Go add nothing relevant here; if java could snap its fingers and toss into the garbage 25 years of built-up community effort, null handling would be better in a heartbeat. But if we start holding 'it has a successful community and a history' against languages, we're signing up as developers to go through a rigamarole where every 15 years or so we gather, build a giant bonfire, ritually burn the world down, and start over.

Count me out! That kind of cheerleading of new languages (it's better cuz no cruft) annoys the heck out of me and sounds like a really, really bad thing to do as citizen of the developer community. Praise kotlin for smart new tricks that are nice; do not praise it for freebies it got by pressing a reset button.

Secondly, what you're describing is a bad way to do it. CollectionUtils.isEmpty is precisely the kind of library method I would never write and would veto: It does nothing, except support and tacitly approve a bad code style.

null is not and never has meant 'empty'. If in your code it means that, that is bad. Stop doing it. null, if it means anything, means 'no value', and 'no value' is not the same as 'empty value'. If EVER you find yourself writing if (x == null || x.isEmpty()), unless it's code explicitly written to bridge and fix an incoming dirty data set, stop what you're doing, go back to whatever code produced the value, and fix it: Return the actual empty value; any code that returns null and means 'empty', is broken. Any code that calls into a method where null does not necessarily mean empty (such as map.get, where it means 'not found', which is not the same thing), but the code intends to treat it that way, should convert on the spot (so, should call map.getOrDefault(k, "") instead of map.get(k)). If it doesn't, it is broken.

If you're not sure, don't 'defensively code' and add the null check, not knowing if it is ever going to come up. Don't do that; let the NPEs guide you, for they are good: They point at where you made a mistake. Now you can fix it. The alternative is that your code is hard to read, hard to test (a null check implies that the value could be null; this would be confusing if it cannot be, and would erroneously lead developers working on this code to be confused about whether some value can contain null or not. That is precisely the kind of thing that got us in this mess in the first place!) – and you're building up tech debt. These are bad things; so don't go there. Write clean code that doesn't null check, and fix methods that are too cavalier in returning null as you hit the NPEs during development. It saves time and makes you more productive in the end, and leads to far cleaner and consistent codebases.

The proposal's default behaviour (throw NPE, with clear messaging) stands, and if it is going to have the ignore option at all, I'm going to write it with pain in my soul. I hope I'm catering to bridge code (jackson deserialization, for example), and not enabling bad code practices.

I know a lot of java coders do the 'null means empty' thing. But many more don't, and I don't want lombok to require a ton of stylistic configuration before it really meshes with your style. I would rather that it then only caters to a single style and has friction with others, especially if the styles it doesn't support well are inferior to the one it works well with.

[KieferSkunk]

That's fine, we can agree to disagree on this point. I've always taken the approach of "Whatever makes sense" when solving problems. Your proposal is fine as it is.

I didn't really want to come here and start another language war. I just felt that these suggestions might be helpful. I'll leave you alone now.

[KieferSkunk]

I will just respond to one thing you said here:

If you're not sure, don't 'defensively code' and add the null check, not knowing if it is ever going to come up. Don't do that; let the NPEs guide you, for they are good: They point at where you made a mistake. Now you can fix it.

This is a very good ideal. Unfortunately, the reality in every business I've been a developer for is that we don't have control over what our external libraries do, we don't necessarily know what other developers in the same company (different groups with their own practices) do, and things slip through the cracks. When NPEs occur in production because they weren't caught in test, they not only cause developer pain, they affect real customers. As such, it's been my practice to always defensively code, to try to catch as many of these potential errors as possible before they happen. It's exhausting to have to be so hyper-vigilant all the time, but it's the reality we find ourselves in frequently in the business world.

[rzwitserloot]

I've written almost all of it, except the bit that tags the param with an appropriate nullity annotation.

My original plan was to look at the source file and take a wild stab as to which one(s) are appropriate. But now that I think about it some more this sounds like a really bad plan. It'd mean that whether or not your plural method gets an annotation depends on whether or not you use it elsewhere. That sounds like a fine way to produce inconsistent code, where some plural methods have em and some don't. I can't 99.5%+ rely on SOMETHING being in the source file I'm in to give away which annotation(s) I can generate. Even if I disregard the various @NonNullByDefault package/module-level annotations out there.

I guess I have to resort to a config key, which is a bit ugly, but at least then there's a way out, and we can apply it to other lombok stuff I guess.