Java Clients: Support Optional<T> on model classes when a field is not required

[kungfoo]

I think (similar to the swift client) that is would be nice, if the generated clients had an option to enable the use of the Optional<T> type on model fields that are not required.

This should both include and option to use

• com.google.common.base.Optional
• java.util.Optional<T>

Supporting guava is probably needed for all those people that are not using java8 for whatever reason (for instance Android projects).

[ePaul]

Not just for clients, also for generated server models this might be useful.

[kungfoo]

Of course you are right. My team is currently really suffering from fields becoming required and vice versa (don't ask) and this would be a most useful feature to counter this. I might give it a shot in the client libs (retrofit with rx) that we use to actually implement this.

[tadhgpearson]

It's not clear to me - are we tallking about Optional request parameters for each operation in the DefaultAPI class, or optionals fields and getters / setters in the model classes used for POST requests, and responses?

(Both might be good - but, in my opinion, would benefit from separate issues)

[ePaul]

@tadhgpearson as I understand it, this is mainly about fields in the model classes (there is no "parameter" at all in the issue). Feel free to open another issue for the operation's parameters, and link to this one.

[cbornet]

Just a note: Optional should not be put used on class atributes. It only has a meaning on methods return types.

[ePaul]

@cbornet Could you elaborate why (or link to a point where this is elaborated)? Do you prefer using just normal reference values which can be null for fields/attributes?

I've heard this several times, but I don't quite understand the reasoning.

[cbornet]

See http://blog.joda.org/2014/11/optional-in-java-se-8.html http://stackoverflow.com/questions/26327957/should-java-8-getters-return-optional-type/26328555#26328555 (Brian Goetz is the man who specified Optional in java8) Note that's for java8 Optional, I don't know about Guava's Optional.

[tadhgpearson]

Thanks for the clarification and education @ePaul @cbornet As you can see, I opened and referenced a new issue

[kungfoo]

The explanation of Brian Goetz absolutely makes sense but in this case I'd argue that the value of getting a compile error, when referencing a return value (in DTOs) that became Optional<T> instead of T when the API changes, far outweighs the other points against using Optional.

My idea was initially to use it in DTOs for fields that are marked optional in the spec of the REST API, similar to how other languages (for instance swift) deal with this.

So instead of creating this DTO (and having the required flag in an annotation)

class Foo {
  Bar getBar() {} // might return null at any time. Or not. Depends on required flag.
  Zap getZap() {} // will not return null, since the field is required.
}

creating this class would be better:

class Foo {
  Optional<Bar> getBar() {} // Optional forces the user of the API to check presence of the value
  Zap getZap() {} // any methods not returning Optional values return non-null values
}

Of course: Lists, Arrays, Sets and any other Collections should never be null (or Optional) for that matter. Whether this is implemented by wrapping the nullable value in the class using Optional.fromNullable() or a field that is in fact Optional<T> is probably up for discussion.

[cbornet]

One issue I see with putting Optional on the getter is that it breaks the javaBean contract. Jackson doesn't use javaBean reflection but people might want to use the generated DTOs with libraries that rely on javaBean spec.

[kungfoo]

@cbornet You are of course right, but then again one should be able to turn the feature on and off.

[cbornet]

Yes, as long as Optional is optional :smile:

[kungfoo]

I would use it in conjunction with the retrofit library with the RxJava adapter. I think most people who use this combo (I have no idea how many that is), would also profit from -optionally- using Optional. :smile:

[cbornet]

My opinion is that you SHOULD use retrofit on android and you SHOULD use RxJava :smile: (with retrolambdas or Jack)

[kungfoo]

@cbornet You are absolutely right. We do and it's a pretty deep rabbit hole.

[cbornet]

On further thought, I think we should generate instead

class Foo {
  public Optional<Bar> getOptionalBar() {}
  public Bar getBar() {}
}

This way, the javaBean contract is still respected. It would be tempting to set getBar() as private but this is not allowed by the javaBean spec "All of the bean’s persistent state must be accessible via getFoo/setFoo properties"

[tadhgpearson]

I would tend to disagree - while this allows backwards compatibility, usage is now unclear for the user. It won't be immediately obvious whether I should use getOptionalBar or getBar, but anyway, since I want to get bar, I probably won't even notice getOptionalBar, much less understand the advantages of using it.

I think it would be better to put each field only once, pick a default way-of-doing things and allow the behaviour to be configurable. (My personal preference isOptional field + Optional getter, but all the options mentioned are both legitimate and imperfect ;) )

[kungfoo]

I'd also argue that this is confusing and not really practical. The idea is to make as much type information as possible explicitly available in the API, so only the methods that provide more information about the types can be alternatives to the current way of doing it. This would have the opposite effect, masking whether a field, method can return absent() (or null) even more.

The Java bean contract is really not important to me/us, what really hurts is fields becoming optional and nobody getting any errors or warnings about it.

On Mon, Oct 3, 2016 at 4:31 PM, Tadhg Pearson notifications@github.com wrote:

I would tend to disagree - while this allows backwards compatibility, usage is now unclear for the user. It won't be immediately obvious whether I should use getOptionalBar or getBar, but anyway, since I want to get bar, I probably won't even notice getOptionalBar, much less understand the advantages of using it.

I think it would be better to put each field only once, pick a default way-of-doing things and allow the behaviour to be configurable. (My personal preference isOptional field + Optional getter, but all the options mentioned are both legitimate and imperfect ;) )

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/swagger-api/swagger-codegen/issues/2485#issuecomment-251121261, or mute the thread https://github.com/notifications/unsubscribe-auth/AACgKLZOxfkK9qJUk73uinjy68z_jpFSks5qwRHKgaJpZM4H9ve7 .

[cbornet]

Well, some people might want to have serializable javabeans and benefit from fluent Optional getters... Now transforming a getter into an optional is just calling Optional.ofNullable(foo.getBar()) so that's not much work to do.

[tadhgpearson]

Well... I guess then we're back to

private Optional<T> bar

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

which comes back to the Optional on class attributes argument :/

It sounds like a tradeoff - you have the options of

1. Usability of one method per field
2. Meeting the Javabeans contract
3. Using optional as designed

and you can only pick 2 :smile:

Personally, I prefer usability over specifications, so I would allow user config and pick 1 & 3 to be my defaults... but I think you know the target audience best @cbornet :D

[cbornet]

In that case, at least keep serializability

private Bar bar;

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

[JLLeitschuh]

This would be awesome on query parameters, at least in spring.

[cbornet]

The JDK made clearer the use of Optional : http://cr.openjdk.java.net/~smarks/reviews/8167981/webrev.0/src/java.base/share/classes/java/util/Optional.java.cdiff.html

[dgreene1]

Can we revive interest in this issue? Because it's a really valuable tool for many of us working in languages we're implicitly null reference types are diminishing productivity. Furthermore, being able to communicate when a reference simply won't be there is useful, in fact it seems to be the goal of swagger documentation: being clear about your contracts.

Is anyone aware of a workaround for the time being? Such as a custom annotation?

[JiangHongTiao]

I also vote to reopen the discussion and move forward. The optional was introduced to represent the third state, that cannot be represented by null value. The thing is, that you want to provide a functionality through REST to update specific record in the database. However you want to allow user to update just specific fields of the object and not whole object. So the user is able to provide only subset of arguments. So the question is, how to represent those attributes in the back-end? If you set in your DTO null value to the property, how do you differentiate between:

1. user wants to keep argument unchanged;
2. user wants to set the value to null?

Without Optional you can do:

• Read whole object with all current values and send it to user, then change whole record again (not ecological)
• Add an extra attribute for each parameter whether to update it or not (makes the model complex and unreadable)

Optional.empty() seems promising, since when it holds value, you want to update it. When it is empty, you do not touch that property. When it is null, you set it to null. I've tried to make a model, however now it looks like:

{
  attrA Optional«string»{...}
  attrB Optional«string»{...}
  attrC Optional«string»{...}
  attrD Optional«string»{...}
}

You can specify @ApiModelProperty(dataType = "java.lang.String") as a workaround, however it would be more nice to have it automatic ;)

[kungfoo]

Because it's a really valuable tool for many of us working in languages we're implicitly null reference types are diminishing productivity.

You could argue that implicitly null reference types are diminishing productivity regardless of the language. It's called the Billion Dollar Mistake by it's inventor for a reason.

[tadhgpearson]

It sounds like a tradeoff - you have the options of

1. Usability of one method per field
2. Meeting the Javabeans contract
3. Using optional as designed

and you can only pick 2 😄

In that case, at least keep serializability

private Bar bar;

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

I think this basically says, you want to pick 1 & 3 :D I think that's reasonable!

[abrudin]

It would help even to annotate fields that are not required (to have a value) with @javax.annotation.Nullable or even some custom @Nullable annotation to avoid unnecessary dependencies. Then the IDE could help out a great deal with warnings. Now one has to inspect the @ApiModelProperty required parameter, which is much more cumbersome.