Hi,

Are there any plans to support support for Java's 8 Optional<T>. I guess however due to the type erasure you would always have to specify the wrapped javaType.

If not plans, did anybody developed either a plugin or type handler?

Thanks.,

Raphael

Comment From: christianpoitras

That be great!

I don't think it would be difficult to implement a TypeHandler. And the type will be accessible to reflection as long as it's a property/attribute of a bean which should cover around 90% of the cases, I guess.

The only type erasure problem might be for constructors because I think Java erases types for method arguments. In this case we will have a problem. Guessing the constructor will become messy if the javaType attribute specifies the wrapped type. And setting java.util.Optional as the javaType will make it easy to find the constructor, but will make it difficult to guess the wrapped type...

Comment From: checketts

Has a TypeHandler for Optional been created yet? I'm cleaning up my code to avoid nulls and using Optional with MyBatis feels like a great fit.

I realize it wouldn't be in MyBatis proper yet to keep it backwards compatible. But either an example or an existing supplemental lib would be great.

Comment From: tmackenzie

I've attempted to add support for Optionals. However I cannot get the project to build. I'm seeing a failure with animal-sniffer-maven-plugin. I've looked through the pom and couldn't find the plugin, animal-sniffer-maven-plugin.

Here is the output of the build.

I think the build will have to target Java 8 in order to add support for Optionals.

Are there any objections to bumping the project to Java 8? Does anyone have any recommendations on how to get the project to build?

Comment From: emacarron

Tom, current policy is current jdk - 2. Note that there are lots of existing servers with versions older than 1.8. Websphere 8 for example, is still supported by IBM and is jdk 1.6.

2016-03-02 15:21 GMT+01:00 Tom Mackenzie notifications@github.com:

I've attempted to add support for Optionals. However I cannot get the project to build. I'm seeing a failure with animal-sniffer-maven-plugin. I've looked through the pom and couldn't find the plugin, animal-sniffer-maven-plugin.

Here is the output of the build https://gist.github.com/tmackenzie/888df2654e515b030365.

I think the build will have to target Java 8 in order to add support for Optionals.

Are there any objections to bumping the project to Java 8? Does anyone have any recommendations on how to get the project to build?

β€” Reply to this email directly or view it on GitHub https://github.com/mybatis/mybatis-3/issues/228#issuecomment-191254847.

Comment From: tmackenzie

@emacarron Thanks for the reply. Is supporting optionals not something you'd like to do then? Since optionals will require Java 8 as the minimum version.

Comment From: mallim

Is it possible to come out another module e.g. mybatis-jdk8 to provides support for JDK 8 features?

Comment From: hazendaz

@tmackenzie animal sniffer is in the mybatis parent pom. Current requirement is java 6 binary compatibility. I think there could be an argument for bumping to java 7 in the core but I highly doubt anyone is fully ready to bump to java 8 as that would cause a significant loss in corporate users and/or diverting branches to support old and new which would be highly difficult to maintain given how large mybatis in general has grown.

@mallim @tmackenzie If a separate module is built for java 8 features that compliments mybatis, that could be done.

Comment From: tmackenzie

@hazendaz thanks for the tip on the parent pom. @mallim I agree I like that approach. I think TypeHandlerRegistry would need to be extended. I'll see if I can make some time to investigate.

Comment From: fengkuok

It seems easy to hack, just wrap DefaultSqlSession and return optional result.

Comment From: derrley

Seems this thread has gone dark. I was wondering if anyone's picked up the work and run with it?

Comment From: harawata

There is an experimental patch that I wrote a while ago. I plan to rebase the branch once other devs have some time to review, but it may take some time.

This test case demonstrates how it works. Please see if it meets your requirements.

Comment From: TimYi

@harawata your patch seems great, Optional should be supported long before.

Comment From: asafm

Any reason the patch offered by @harawata would not be merged @christianpoitras ?

Comment From: christianpoitras

His patch cannot be merged directly due to our support policy which is 'current version -2'. This forces us to support Java 7 right now. I believe Spring is able to support multiple versions of Java in a single jar, but I don't think we are doing this yet.

Comment From: asafm

Is it possible to have two branches? One per jvm version ? On Wed, 3 Jan 2018 at 17:15 Christian Poitras notifications@github.com wrote:

His patch cannot be merged directly due to our support policy which is 'current version -2'. This forces us to support Java 7 right now. I believe Spring is able to support multiple versions of Java in a single jar, but I don't think we are doing this yet.

β€” You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mybatis/mybatis-3/issues/228#issuecomment-355036379, or mute the thread https://github.com/notifications/unsubscribe-auth/AA8Y8ccLMztoSrUupq8InYbfcoUIGpxCks5tG5mBgaJpZM4CGBI5 .

Comment From: christianpoitras

Although it is technically possible, I don't think it is worth the effort.

A much better alternative would be to develop MyBatis 4 with code rewritten for Java 8. Some cool changes were suggested in the dev mailing list!

Comment From: harawata

@christianpoitras ,

I'm sorry for not being clear enough. My patch does not break Java 6 compatibility.

I believe Spring is able to support multiple versions of Java in a single jar, but I don't think we are doing this yet.

I've done this a while ago with the help of @hazendaz . If you could make some time to review it, I'll rebase the patch ASAP.

Many thanks to those who reviewed/tested my patch and sorry for the lack of response. This is a big patch and it has some ugliness [1], so I want/need the other committers to evaluate if it's acceptable or not. Thank you for your patience!

[1] The method signature of SqlSession#selectOptional looks as follows:

<T> T selectOptional(...

whereas it should be:

<T> Optional<T> selectOptional(...

This is a limitation when we keep Java 6, 7 compatibility.

Comment From: christianpoitras

@harawata Can you update your code to the current commit on mybatis-3? There are many merge conflicts that make it difficult to compare. At first glance, most modifications make sense when comparing your version with its previous commit.

Comment From: harawata

@christianpoitras I've just updated the branch. This time the test case is included. https://github.com/harawata/mybatis-3/commits/optional

Comment From: christianpoitras

Everything is fine with me.

The only issue I see is that is doesn't support classes like OptionalInt, but it does support Optional<Integer>. Do you think it should be supported?

Comment From: asafm

In my opinion, the majority of people are using Optional, so it's a very good start as it is.

On Thu, Jan 4, 2018 at 6:53 PM Christian Poitras notifications@github.com wrote:

Everything is fine with me.

The only issue I see is that is doesn't support classes like OptionalInt, but it does support Optional. Do you think it should be supported?

β€” You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mybatis/mybatis-3/issues/228#issuecomment-355335680, or mute the thread https://github.com/notifications/unsubscribe-auth/AA8Y8ZBWjxPDJIKrC9TZPiniFht27SOjks5tHQINgaJpZM4CGBI5 .

Comment From: christianpoitras

I agree. Support for OptionalInt should not prevent merging @harawata's patch since it can be done later.

Comment From: harawata

Thank you, @christianpoitras @asafm !

As OptionalInt is not a parameterized type, adding a new type handler might be sufficient. I'll look into it.

We are a bit too close to 3.4.6 release, so I'll ping the dev list again and set the target to 3.5.0 if no one objects.

Comment From: harawata

Added type handlers for Optional[Int|Long|Double] and it seems to be working.

Constructor auto-mapping won't work if an argument is OptionalInt, but it should not be a big problem (explicit-mapping works, of course).

Comment From: christianpoitras

Thanks @harawata ! That was much more complex to solve than I expected! :)

Comment From: harawata

I have found an issue with lazy loading and it's not easy to overcome 😣 And now I am having second thoughts...

Maybe, we should support Optional only as a return type of a SELECT statement following the developer's suggestion [1]. @kazuki43zoo has already implemented it (#799). I haven't reviewed it, but it should be a good start point. What do you guys think?

[1] http://mail.openjdk.java.net/pipermail/lambda-libs-spec-experts/2013-May/001814.html https://stackoverflow.com/a/24564612/1261766

Comment From: jeffgbutler

+1.

I think optional makes sense as a return for "select one" queries where we return null now. It is not needed for the "select many" queries because we never return null from those queries. I also don't think it is needed for parameters into queries.

I will say that it is easy to go overboard with optional (as described in the openjdk link you reference). I did it for a while myself in my development of mybatis-dynamic-sql. Fortunately I regained my sanity. When I found myself writing signatures like Optional<Stream<Optional<String>>> I knew something had gone very wrong 😈

So I think it is wise to keep it very simple to start. As you have said before, it is hard to remove something once it is in the code base. If we find another good use case we can add it later.

Comment From: kazuki43zoo

I agree with comments of @harawata and @jeffgbutler

Comment From: asafm

From my current needs, it's the ability to update based on a pojo that contains an Optional property, and retrieve a list of this pojo upon simple select. This is supported right?

On Sun, Jan 7, 2018 at 3:36 PM Kazuki Shimizu notifications@github.com wrote:

I agree with comments of @harawata https://github.com/harawata and @jeffgbutler https://github.com/jeffgbutler

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mybatis/mybatis-3/issues/228#issuecomment-355823285, or mute the thread https://github.com/notifications/unsubscribe-auth/AA8Y8aT5DchAJSP83yUbcYvUobLxN46Wks5tIMhhgaJpZM4CGBI5 .

Comment From: jeffgbutler

@asafm this can work right now with no change on our part if you will design your POJOs a little differently:

public class SomePojo {
  private String name;

  public SomePojo(String name) {
    this.name = name;
  }

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

  // traditional getters/setters if you want them , but you don't need them
}

This is in step with the very good idea that you shouldn't store an Optional as an attribute - only use it for return types. The Java designers want us to use Optional this way - that is why they refuse to make Optional implement Serializable.

Personally, I think the "getter" without the "get" in the name makes the code look a little nicer anyway. I've been working with objects like this for a while now.

You could also add a default constructor if you didn't want to use constructor mapping in MyBatis.

Comment From: christianpoitras

As explained in this thread, Optional should not be used for fields that are saved to database. But who am I to judge! :) https://stackoverflow.com/questions/24547673/why-java-util-optional-is-not-serializable-how-to-serialize-the-object-with-suc

The best practices for Optional states that it should be used only as a return type of a method. Not for fields or parameters. Here a link among others. https://zeroturnaround.com/rebellabs/java-8-best-practices-cheat-sheet/

Anyway, I wouldn't mind if it was only possible to return an Optional value from a mapper's method.

Comment From: asafm

Thank you for answers. I had no idea about the JSR group opinion, and how heated this debate is (almost the same as using final everywhere, or using checked exception only).

Practically, we've been using Optional here and there, but not yet as bean properties, mainly to signify to the reader of the code and the user of it that that value may not be there. I personally find that much more readable than guessing that, or using an annotation to mark that. I haven't experienced any downsides from that approach so far, nor my colleagues in my team.

I guess it's a matter of taste and opinion, as the amount of blogs on the topic (pro, con) suggest :) This type of new information usually takes time sink in, and I might end up with a different opinion all together in the future.

Jeff, you suggestion has one downside which is forcing to define constructor mapping, hence more boilerplate. I really like the simplicity in MyBatis of associating the column name to the attribute name easily, this is we still stick to getters and setters, although I am a fan of immutability - this is one of the reasons we consider POC some of our code in Kotlin to see if the language can solve this for us.

Is it o.k. with your help to understand the situation after the patch with this question: Say I do place an Optional as an attribute (e.g. Optional streetAddress2), and have the following method: Optional getStreetAddress2() void setStreetAddress2(Optional streeAddress2)

Would that work?

On Sun, Jan 7, 2018 at 7:26 PM Christian Poitras notifications@github.com wrote:

As explained in this thread, Optional should not be used for fields that are saved to database. But who am I to judge! :)

https://stackoverflow.com/questions/24547673/why-java-util-optional-is-not-serializable-how-to-serialize-the-object-with-suc

The best practices for Optional states that it should be used only as a return type of a method. Not for fields or parameters. Here a link among others. https://zeroturnaround.com/rebellabs/java-8-best-practices-cheat-sheet/

Anyway, I wouldn't mind if it was only possible to return an Optional value from a mapper's method.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mybatis/mybatis-3/issues/228#issuecomment-355838036, or mute the thread https://github.com/notifications/unsubscribe-auth/AA8Y8bI-iMb6fIenGZ30MU96q-wKxcO6ks5tIP4rgaJpZM4CGBI5 .

Comment From: christianpoitras

Let's poke @harawata since the question is one week old.

@asafm To the best of my knowledge, Optional<String> getStreetAddress2() will not work.

Comment From: harawata

Sorry for a late reply! I like @jeffgbutler 's plan (i.e. start simple), of course. As there seemed to be an agreement between committers, I reopened #799 .

Regarding @asafm 's question, @christianpoitras is right. By default...

  • MyBatis cannot set a value via a setter whose argument is Optional.
  • MyBatis cannot get a value from a getter whose return type is Optional.

It may be possible to write a custom type handler, though.

Comment From: christianpoitras

@harawata Can you add a test case for lazy loading so I can take a look at the problem?

Comment From: harawata

@christianpoitras , I've just pushed it to the branch on my fork. I thought I did already, sorry.

Comment From: JanecekPetr

I believe this was implemented in https://github.com/mybatis/mybatis-3/pull/799, wasn't it?

Comment From: harawata

Yes, let's close this. Thanks for the heads up, @JanecekPetr !