projectlombok / lombok

Very spicy additions to the Java programming language.
https://projectlombok.org/
Other
12.91k stars 2.39k forks source link

Any reason why we can't have an OptionalGetter annotation? #1214

Closed ownaginatious closed 5 years ago

ownaginatious commented 8 years ago

I've read the responses to Optional around here and how the authors seem to generally hate it, and I agree that it pretty much never makes any sense to have an API take in some type Optional<T> parameter.

However, are there any arguments against having something like this? I saw something similar posted in this issue here, but it was never addressed before being closed.

public class Foo {
    @NonNull @OptionalGetter @Setter
    private String bar;
}
public class Foo {
    private String bar;

    public Optional<String> getBar() {
        return Optional.ofNullable(bar);
    }

    public void setBar(@NonNull String bar) {
        this.bar = bar;
    }
}

There are a lot of instances where I'd like to more clearly indicate "no value set yet" rather than just returning null to the caller.

lpandzic commented 8 years ago

Just to throw in a couple more arguments for adding this feature:

Maaartinus commented 8 years ago
public class Foo {
    @NonNull @OptionalGetter @Setter
    private String bar;
}

IMHO, @NonNull shouldn't be there, as otherwise your "Optional" would be "Mandatory" (as you could never get absent). I guess, @OptionalGetter should imply @Nullable on the field (and setter argument), but the generated getter should be annotated with the @No[tn][Nn]ulls the user wants (configuration).

ownaginatious commented 8 years ago

The purpose of @NonNull in this particular example was to be more restrictive in rejecting any nulls as arguments to the setter, which effectively allows the optional getter to only be empty if it hasn't been set yet. In other words if Foo.getBar() is empty, then it's guaranteed that hasn't been set yet. @NonNull just affects generated constructors/getters, right?

Perhaps it would have been more clear if I'd written the example like this:

public class Foo {
    @NonNull @OptionalGetter @Setter
    private String bar = null;
}

Anyway, after looking through the Lombok code, I think it might be easier to implement this feature instead as:

@Getter(optional=True)

There seems to be a lot of code to detect stacking Getter annotations that would need to be duplicated to handle OptionalGetter. I may try to implement support myself at some point, but in general, the scope of this project is way beyond my expertise :stuck_out_tongue:

Maaartinus commented 7 years ago

@NonNull just affects generated constructors/getters, right?

Right. And you're right: Without generating a constructor, the field may be null even when annotated @NonNull. Forbidding to set it to null explicitly might make sense.

it might be easier to implement this feature instead as @Getter(optional=True)

I guess so. This precludes the possibility to generate also a normal getter, but having two getters would be pretty strange anyway. I wouldn't call the argument just optional, but rather useOptional or alike (as the getter isn't really optional).

A nicer possibility could be to use @UseOptionalGetters as a modifier, possibly on class level, or even a configuration, similar like @Accessors works.

lpandzic commented 7 years ago

In case of modifier at type level, it would be nice if bean validation annotations were also supported (@NotNull).

bb010g commented 7 years ago

:+1: for an optional getters configuration.

wodencafe commented 6 years ago

+1 please implement this feature, I came here to request this feature and did a search and found this github issue. I am in this same situation, specifically I am using Lombok with JPA, to generate my Business Entities. That works great, but I want to have Optional Getters, as I must leave the field itself as the real type, because JPA doesn't know how to translate Optional Fields. Something like this would be amazing:

@OptionalGetter // Would create a "public Optional<VehicleType> getType()" method
@Column(nullable = true) // JPA Column Mapping
private VehicleType type;

01/06/2018 Update: This could also be a useful syntax, as referenced earlier by @ownaginatious

@Getter(optional=true)
ryonday commented 6 years ago

If you add this, I will dance at your wedding.

+1

jotaen commented 6 years ago

Not sure whether we should create two methods, a “plain” value return and a separate “wrapped-in-optional” return. The point about Optional is to make the code safer and to avoid NPEs. But for achieving this, it shouldn’t be up to the consumer to decide which of the two accessors to use. A asOptional suffix also feels a bit like hungarian notation and only repeats information that is already provided in the signature. (This is a personal preference, though.)

I would prefer the following usage. It would be straightforward and avoids the ambiguity that arises when two annotations are present (e.g. @OptionalGetter and @Getter).

@Getter(optional=true)
class MyClass {

 String foo; // -> Optional<String> getFoo()

 @Getter(optional=false) // override for individual field
 String bar; // -> String getBar()
}

(optional=false) would be the default, for backwards compatibility. It can be set on class and on field level, where field level would take precedence.

gerardbosch commented 5 years ago

I use Lombok but I am currently writing my own getters to return Optional for the nullable fields of my classes. In this use case, users must worry with this boilerplate. I think this feature (as previously said @Getter(optional = true) or something similar) would be a very desirable feature for Lombok, letting the user to decide if work with Optionals or not.

Lombok already provides mechanisms to indicate which fields are nullable so I see this one a very viable feature.

randakar commented 5 years ago

Why not just make the field itself Optional?

On Tue, Jan 22, 2019 at 11:26 AM Gerard Bosch notifications@github.com wrote:

I use Lombok but I am currently writing my own getters to return Optional for the nullable fields of my classes. In this use case, users must worry with this boilerplate. I think this feature (as previously said @Getter(optional = true) or something similar) would be a very desirable feature for Lombok, letting the user to decide if work with Optionals or not.

Lombok already provides mechanisms to indicate which fields are nullable so I see this one a very viable feature.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1214#issuecomment-456348301, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRblG6QzXS4h81S12yt0-3rPwvFALks5vFudFgaJpZM4KccUJ .

-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven

rzwitserloot commented 5 years ago

Lots of traffic on this. Sorry to disappoint you all: We're not adding this. PRs with this feature will not be accepted. Discussing the merits and downsides of optional would probably take more time and words than working out how to achieve world peace, so, please don't try to extol optional's virtues here.

ownaginatious commented 5 years ago

@rzwitserloot you should probably list out at least a few of the major reasons for not wanting to support Optional at all if you don't want someone to bring this up again.

benjaminjbachman commented 5 years ago

They've talked about it before https://stackoverflow.com/questions/31670785/optional-in-lombok https://www.voxxed.com/2015/01/embracing-void-6-refined-tricks-dealing-nulls-java/ They even quote Brian Goetz, the Oracle engineer who introduced Optional:

You should almost never use it as a field of something or a method parameter. I think routinely using it as a return value for getters would definitely be over-use.

That said, I strongly disagree with their stance and I think this feature to allow Optional getters should be implemented. Even in that Goetz quote its clear that sometimes using it as a return value for getters makes sense, which is all we're asking for here.

gerardbosch commented 5 years ago

I think the arguments about not passing Optionals around are based in the fact that Optional itself it is not serializable and it was deliberately designed this way to avoid using it that way if I am not wrong. That's why the reccommendation is against class members of type Optional.

But on contrary I can see use cases where the return of a getter can box the field in an optional, that's why I also would like the Lombok optional getter.

P.S. vavr (a great Java lib that provides functional programming constructs) Option is serializable!

On Wed, Feb 13, 2019, 16:02 Ben <notifications@github.com wrote:

They've talked about it before https://stackoverflow.com/questions/31670785/optional-in-lombok

https://www.voxxed.com/2015/01/embracing-void-6-refined-tricks-dealing-nulls-java/ They even quote Brian Goetz, the Oracle engineer who introduced Optional:

You should almost never use it as a field of something or a method parameter. I think routinely using it as a return value for getters would definitely be over-use.

That said, I strongly disagree with their stance and I think this feature to allow Optional getters should be implemented. Even in that Goetz quote its clear that sometimes using it as a return value for getters makes sense, which is all we're asking for here.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1214#issuecomment-463231712, or mute the thread https://github.com/notifications/unsubscribe-auth/AdT09IvmurwI0IHfHi-zwUq094R-AMFdks5vNCkWgaJpZM4KccUJ .

jotaen commented 5 years ago

Optional is not a replacement for null nor a fancy way to prevent NullPointerException.

(@rspilker, https://stackoverflow.com/questions/31670785/optional-in-lombok)

This is an unnecessarily limited view. The Optional interface follows monadic laws (comparable to Maybe in Haskell and the like) and the above sentence is the exact purpose of such data structures when it comes to return types: representing something that can either be present or absent, and providing a standardised (i.e. common) way of safely interacting with it.

You should almost never use it as a field of something or a method parameter. I think routinely using it as a return value for getters would definitely be over-use.

(Brian Goetz, https://stackoverflow.com/questions/26327957/should-java-8-getters-return-optional-type)

Optional shouldn’t be used as field type, there is no doubt in that. And it’s certainly a bad practice to use it for method input parameters either. However, it makes a lot of sense as the return type for accessors of nullable (!) fields, because that ultimately allows for a safer and stricter programming style. (“not routinely” != “never”)

So it’s either of:

// We might or might not find it:
Optional<Something> findSomethingById(@NonNull String id);

// We can guarantee at compile-time that it will always be there:
@NonNull
Something getSomething();

(Whether or not you write out the annotations is a different story of course, but you get the idea.)

randakar commented 5 years ago

Maybe it's just me but outside of the J2EE world I really do not see the point of making anything Serializable. As long as you do not use RMI there are other ways to serialise and deserialise data that do not require that interface. As far as I can tell not even Hibernate really needs it anymore for simple applications.

Given that Serializable feels like old deprecated nonsense I do not see the point of not using Optional wherever you can, either. There is nothing wrong with Optional<> fields per sé. Frameworks nowadays usually support this just fine. Jackson does. JPA does. Hibernate sorta can but doesn't (yet). So what is the reason that using this thing for fields is discouraged?

Seriously: Why? People aren't discouraging the use of Collection<>, List<> or Set<> either, so why is Optional<> any different?

On Thu, Feb 14, 2019 at 11:35 AM Jan Heuermann notifications@github.com wrote:

Optional is not a replacement for null nor a fancy way to prevent NullPointerException.

(@rspilker https://github.com/rspilker, https://stackoverflow.com/questions/31670785/optional-in-lombok)

This is an unnecessary limited view. The Optional interface follows monadic laws (comparable to Maybe in Haskell and the like) and the above sentence is the exact purpose of such data structures when it comes to return types: representing something that can either be present or absent, and providing a standardised (i.e. common) way of safely interacting with it.

You should almost never use it as a field of something or a method parameter. I think routinely using it as a return value for getters would definitely be over-use.

(Brian Goetz, https://stackoverflow.com/questions/26327957/should-java-8-getters-return-optional-type )

Optional shouldn’t be used as field type, there is no doubt in that. And it’s certainly a bad practice to use it for method input parameters either. However, it makes a lot of sense as the return type for accessors of nullable (!) fields, because that ultimately allows for a safer and stricter programming style. (“not routinely” != “never”)

So it’s either:

// We might or might not find it: Optional findSomethingById(@NonNull String id);

// We can guarantee at compile-time that it will always be there: @NonNull Something getSomething();

(Whether or not you write out the annotations is a different story of course, but you get the idea.)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1214#issuecomment-463578014, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRauMa0rwQEP5-rHq-XO5T82VXRv5ks5vNTvfgaJpZM4KccUJ .

-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven

jotaen commented 5 years ago

@randakar if you are fine with declaring fields as Optional, then you can do this today already and Lombok would generate the respective getter for you:

@Getter
private Optional<Something> sth;

The point of this thread is to introduce an accessor for nullable fields to Lombok that would wrap it in Optional “on the fly”. In order for this thread to stay focussed on this initial request I suggest to move the general discussion about optional fields to a different place.

raffig commented 5 years ago

Well, I don't get the problem with introducing Optional getter either.

Let's make it clear: optional is for API designers, so:

  1. Optional should not be used as an argument, thus declaring field as Optional does not make any sense. Optional is NOT for replacing null, so if one wants to set "no value" then null should be passed. API designer dictates if nulls are allowed or not, not you as a user.

  2. Declaring a field as an Optional is asking for a trouble. Try anything that requires serialization into string, like converting to/from JSON representation and it will become clear what pain it is. That's because Optional was NOT designed to store value.

  3. Optional however WAS designed for APIs. It exists to indicate to the user that returned value MAY not exist. This means that is perfectly fine to have: Optional getSomePossiblyOptionalProperty(). Consider a case of driver class often recalled. API may specify that some property of driver Optional getCache(). That's how user knows that cache may or may not be present and should handle the situation properly. It is much easier to make a mistake and ignore possible null if it would only be Cache getCache().

Summarizing I don't see any cons for Getter(optional = true), only pros.

Please reconsider @rzwitserloot .

randakar commented 5 years ago
  1. "should not be used" - says who? And more importantly, why? I haven't seen a single good reason to accept nulls anywhere, whereas I've seen lots of reasons to not do so. The only good reason against Optional I've heard so far is the serializable interface, which I consider a misdesigned malfeature. And if you really need that there's still Vavr's alternative version of this.
  2. Serialisation into string works fine. Jackson does it without any problems, just make sure you have the dependencies right and put a default in. And if it doesn't - that's a bug in the serialisation framework, not the use of Optional.
  3. That distinction between use in an API and regular use is therefore entirely artificial personal preference.

Summarizing: just make your field Optional<> and you will actually get all the benefits of compile-time null check enforcement, instead of asking for a workaround for something that doesn't make sense in the first place.

On Thu, Feb 21, 2019 at 10:47 AM raffig notifications@github.com wrote:

Well, I don't get the problem with introducing Optional getter either.

Let's make it clear: optional is for API designers, so:

1.

Optional should not be used as an argument, thus declaring field as Optional does not make any sense. Optional is NOT for replacing null, so if one wants to set "no value" then null should be passed. API designer dictates if nulls are allowed or not, not you as a user. 2.

Declaring a field as an Optional is asking for a trouble. Try anything that requires serialization into string, like converting to/from JSON representation and it will become clear what pain it is. That's because Optional was NOT designed to store value. 3.

Optional however WAS designed for APIs. It exists to indicate to the user that returned value MAY not exist. This means that is perfectly fine to have: Optional getSomePossiblyOptionalProperty(). Consider a case of driver class often recalled. API may specify that some property of driver Optional getCache(). That's how user knows that cache may or may not be present and should handle the situation properly. It is much easier to make a mistake and ignore possible null if it would only be Cache getCache().

Summarizing and don't see any cons for Getter(optional = true), only pros.

Please reconsider @rzwitserloot https://github.com/rzwitserloot .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1214#issuecomment-465932929, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRVJkQZ3fpcnNwD91HJ6kl-NjTkFmks5vPmsjgaJpZM4KccUJ .

-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven

raffig commented 5 years ago
  1. As per question why it should not be used as arguments short answer is: because there are better approaches as method overloading. For longer explanation see this article: http://dolszewski.com/java/java-8-optional-use-cases/

Also you can take a look at this discussion: https://stackoverflow.com/questions/31922866/why-should-java-8s-optional-not-be-used-in-arguments

  1. Sometimes works, sometimes not. Consider it as a bug and wait forever to be fixed. Even if it works it is a waste of resources IMHO for packing/unpacking when it has no practical advantage over simple null.

  2. I don't get your point.

Summarizing - no, because with Setter annotation I get the setter(Optional) then, what is bad design as per point no. 1. So it is not a solution, sorry.

raffig commented 5 years ago

The builder would look even funnier. Imagine: WhateverBuilder.property(Optional).

Making field optional solves nothing and uses more resources.

randakar commented 5 years ago
  1. I don't see how method overloading has anything to do with making fields Optional<>. And that link you provide mentions that exact case as the most controversional one, with " the main obstacle which prevents using Optional as a POJO field is support of libraries and frameworks". Well, it's now one and a half year later, that support has arrived or is in the process of arriving, it's time to reconsider this.

  2. I use it all the time. It works. A year or two ago it was a different story maybe, but now I never have problems with it.

As for the practical advantage? Well. NullPointerException is the most seen bug in production environments across the globe. https://blog.overops.com/the-top-10-exceptions-types-in-production-java-applications-based-on-1b-events/ This means that anything you can do to prevent that exception from happing will have a measureable impact on your bug count. And Optional<> was designed to force developers to check this, so it will help prevent that problem.

As for the setter .. don't really care. The method argument overloading argument from that article doesn't apply. That's about having methods that actually take decisions based on whether the argument is optional or not. A setter however just puts the object somewhere and forgets about it. No bad design at all, it's even faster than instantiating new Optionals whenever someone calls the getter on that field.

On Thu, Feb 21, 2019 at 11:31 AM raffig notifications@github.com wrote:

The builder would look even funnier. Imagine: WhateverBuilder.property(Optional).

Making field optional solves nothing and uses more resources.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1214#issuecomment-465947741, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRVXxOeNRr3zq8ZZtkLngUjEHWtbkks5vPnWJgaJpZM4KccUJ .

-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven

raffig commented 5 years ago

ad. 1. It is easy: Imagine two objects. One has: makeSomething(Optional a, Optional b, Optional c)

How do you know what combinations are allowed? All parameters may be empty? Or maybe a & b must be present or b & c? Or mayby at least one? Nobody knows, unless you read the javadoc. If it exists :-). Imagine length of that javadoc. Imagine length of code for such method. It breaks most rules of good design. Waste of CPU, memory and what is most important your own, expensive time for reading two screens javadoc explaing how the hell this method should be used.

And now imagine other object with 3 methods: makeSomething(a, b) makeSomething(a, b, c) makeSomething(b)

Ah, much clearer. Passing only a and c is not possible. I know that within 3 seconds. No javadoc? No problem! I still know that a & c is not enough. Each method has short, clear, easy to test implementation and short javadoc.

Maybe you would like to say that setA(Optional) is good example, because maybe value can be empty? Well, for what reason are you calling that setter at all then? To cleanup setA? Hey, method clearA() is much more obvious.

Sorry, I can't find any good example of using optional as an method argument. It only encourages bad, far from obvious design.

raffig commented 5 years ago

And yes, optional helps with NPE. As a returned value. For field it is at least debatable and for argument it is simply a bad design.

raffig commented 5 years ago

And here you go an answer from coauthor of Optional Brian Goetz: https://stackoverflow.com/questions/26327957/should-java-8-getters-return-optional-type/26328555#26328555

I agree that people can do what they want, one can even stick nails with screwdriver, however I see no reason why that Optional Getter could not be introduced to lombok for people who still prefer hammers.

jotaen commented 5 years ago

Let us please keep this thread focused on the initial request, which is to add a getter annotation to Lombok that would wrap a nullable field into Optional on the fly. While the general debate about Optional field types or Optional method parameters is certainly interesting, it doesn’t bring us forward in that matter here.

raffig commented 5 years ago

I agree, I just understood that there is no need or no reason to do this, regarding that this issue is rejected and close. This is why I provided number of reasons why Optional is useful and Optional Getter annotation would be great feature.

randakar commented 5 years ago

Hard to stop arguing when somebody is wrong on the internet. https://xkcd.com/386/ ;-) So feel free to stop reading.

Raffig: Your "makeSomething()" call is besides the point. I agree that is how you should do that. But it's not talking about making a field Optional at all so .. irrelevant.

Besides, I actually try to avoid setters. Setters are for mutable objects, and objects should be immutable if at all possible. Put @Value on your class and you won't have any setters. Even if you had to have setters, if you read the argument against method arguments it's talking about methods that have conditional logic based on the content of the optional. A method that just copies a value into a field of the same type is imho fine. Calling it with an empty optional is fine as well - if this is just passing in data coming from somewhere else it simply isn't the responsibility of that piece of code to care about it.

Finally, Brian Goetz - yes, that's that famously quoted example. It's appeal to authority. He argues that it's not serializable (see earlier) and that the get() method is poorly named and should not be used. Fair enough. Doesn't mean Optional is unsuitable for a field, though.

On Thu, Feb 21, 2019 at 2:22 PM raffig notifications@github.com wrote:

I agree, I just understood that there is no need or no reason to do this, regarding that this issue is rejected and close. This is why I provided number of reasons why Optional is useful and Optional Getter annotation would be great feature.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1214#issuecomment-465997417, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRY1vJQzcLjPuk5sADsQo2gNIJjR2ks5vPp2jgaJpZM4KccUJ .

-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven

raffig commented 5 years ago

Summarizing:

  1. Yes, getters (and other methods returning value) should return Optional if it is possible that there is no value. If calculate(arg1, arg2) cannot calculate something and there is no reason for exception the return value should be Optional as well.
  2. No, Optional should not be used as parameter - we have found no just cause to do this, every argument is against using Optional this way.
  3. Optional actually could be used as field. Potential problem are serializations of any kind (Java serialization, JSON, Hibernate etc.). If that is not the case, then actually we found no good reason for not using Optional as field - use it or not, it is a matter of taste.

BUT, regarding Lombok features: Making field Optional when getter AND setter are required does not solve the problem of missing Optional Getter, as in this case Setter would be bad (see point no. 2 above). Also Builder should be considered with Optional fields so no strange methods were created.

This is why this issue is still valid and justified and I advocate reopen and accept :-)

jroper commented 5 years ago

I don't think it makes sense to argue here about how someone should use Optional. The fact is, there are multiple different opinions, with large groups behind each. For example, talk to anyone with a Scala background that is doing Java, and they will tell you that Optional should be used as much as possible in place of nulls everywhere, because that's exactly how Scala uses it with its equivalent Option type. Now whether they are right or wrong is beside the point, people with this opinion exist, and they form a significantly sized portion of the lombok user community, and would benefit from better support for Optional in lombok. As an example of such a community, the Java APIs for Play Framework, Lagom and Akka all make strong use of Optional in fields and as method parameters, and all three of those frameworks recommend using lombok for immutable Java classes. These are large communities that have this opinion, it's not a fringe view.

So the question is, is the use of Optional something that lombok itself should have an opinion on? I don't see why it should. Providing better support for Optional fields won't have any negative impacts on people whose opinion it is that you shouldn't use Optional fields, because they're not going to use Optional fields and so they'll never see that support. But it will certainly help those whose opinion it is that you can/should. And it doesn't seem to be a feature that would have a very high cost to implement or require significant refactoring in lombok. So why hold out on this portion of your users?