CodeSOD: Annotated Private Members

Shery sends us perfectly innocent looking Java class, marked up with some annotations. The goal of this class is to have an object which contains a list of names that is definitely not null. Let's see how we did.

@Data@Builder@NoArgsConstructor@AllArgsConstructorpublic class NamesHolder { @NotNull private List<String> names;}

So, the first four annotations are from the Lombok library. They're code generation tools that can generate most of your boiler plate for you.

@Data is easy: it just generates getters and setters for every property. Note, these getters/setters don't do any validation, so this won't stop you from setting to null. It also creates a constructor with arguments for all properties.

Which, @AllArgsConstructor also generates a constructor with arguments for all properties, so that's a curious choice.

Neither of these do anything to prevent the list from being initialized to null.

@NoArgsConstructor does exactly what it implies, which is useful here, since that's a default constructor. It does not, however, initialize objects to non-null values.

Finally, @Builder creates a static factory. This is common for classes with lots of properties which need to be initialized, e.g., ComplicatedClass instance = ComplicatedClass.builder().withProp(value).withOtherProp(value).build()

This is completely overkill for a class with only one property, and still hasn't done anything to prevent our list from being null.

But hey, there's that @NotNull annotation on there. That must do something, right? Well, that isn't from the same library, that's from JavaX, and it's meant to support serialization. The annotation is information for when serializing or deserializing data: a valid object can't be null.

The class in question here isn't getting serialized, it's just a lightweight container to pass some data between modules. So @NotNull doesn't do anything in this case.

When you take into account all the autogenerated code in play here, while that list of names is declared private, it is very much public, and very much available to everybody to manipulate. There's no guarantee that it's not null, which was the entire purpose of this class.

Shery adds:

All the original programmer had to do is make sure our original list is not null, chuck it in a box marked 'NamesHolder' and send it on it's way. Instead they've made it as nullable as possible using 3rd party tools that they don't understand and aren't needed.Friends of mine recently contemplated giving up programming, fed up after having to deal with similar stuff for years, and may open a brewery instead. I may join them.

[Advertisement] ProGet's got you covered with security and access controls on your NuGet feeds. Learn more.