Submitted a PR to ShardingSphere, I don't know if I dislike me

I'm ashamed to say, I've been a programmer for 10 years, but I haven't made any contribution to open source. I used to only know Gaga's writing, and I had to change it when there was a problem. I never thought of submitting a PR to fix it. I encountered a problem recently and found that It's very simple, just bring a PR in the past.

question

The problem is quite simple, that is, when using mybatis and ShardingSphere, someone used this time type in the model class OffsetDateTime and found that an error will be reported.

 Caused by: java.lang.ClassCastException: class java.sql.Timestamp cannot be cast to class java.time.OffsetDateTime (java.sql.Timestamp is in module java.sql of loader 'platform'; java.time.OffsetDateTime is in module java.base of loader 'bootstrap')
    at org.apache.ibatis.type.OffsetDateTimeTypeHandler.getNullableResult(OffsetDateTimeTypeHandler.java:38)
    at org.apache.ibatis.type.OffsetDateTimeTypeHandler.getNullableResult(OffsetDateTimeTypeHandler.java:28)
    at org.apache.ibatis.type.BaseTypeHandler.getResult(BaseTypeHandler.java:85)
    ... 99 more

This is a simple type conversion exception, so I followed the source code and saw first BaseTypeHandler#getResult This method actually returns the query result according to the column name.

According to the calling relationship, I found the OffsetDateTimeTypeHandler implementation class.

It is found that the method rs.getObject() will be called eventually. In fact, this method will eventually go to the getObject method implemented by ShardingSphere.

When I saw this, I actually understood why the error was reported. Shardingsphere only judged a few types LocalDateTime and other types. This special time type is not processed, and will eventually be converted to Timestamp, and then forced Reported wrong.

Finally, call the ResultSetUtil#convertTimestampValue method, and you can see that this is indeed the case.

Then if you modify the source code, it is actually very simple, getObject to judge and add one more, convertTimestampValue to add one more, it is very simple.

 @Override
    public <T> T getObject(final int columnIndex, final Class<T> type) throws SQLException {
        if (BigInteger.class.equals(type)) {
            return (T) BigInteger.valueOf(getLong(columnIndex));
        } else if (Blob.class.equals(type)) {
            return (T) getBlob(columnIndex);
        } else if (Clob.class.equals(type)) {
            return (T) getClob(columnIndex);
        } else if (LocalDateTime.class.equals(type) || LocalDate.class.equals(type) || LocalTime.class.equals(type) || OffsetDateTime.class.equals(type)) {
            return (T) ResultSetUtil.convertValue(mergeResultSet.getValue(columnIndex, Timestamp.class), type);
        } else {
            return (T) ResultSetUtil.convertValue(mergeResultSet.getValue(columnIndex, type), type);
        }
    }
    
private static Object convertTimestampValue(final Object value, final Class<?> convertType) {
        Timestamp timestamp = (Timestamp) value;
        if (LocalDateTime.class.equals(convertType)) {
            return timestamp.toInstant().atZone(ZoneId.systemDefault()).toLocalDateTime();
        }
        if (LocalDate.class.equals(convertType)) {
            return timestamp.toInstant().atZone(ZoneId.systemDefault()).toLocalDate();
        }
        if (LocalTime.class.equals(convertType)) {
            return timestamp.toInstant().atZone(ZoneId.systemDefault()).toLocalTime();
        }
        if (OffsetDateTime.class.equals(convertType)) {
            return timestamp.toInstant().atZone(ZoneId.systemDefault()).toOffsetDateTime();
        }
        return value;
}

repair

At first, I didn't actually want to change the source code. I was thinking of other implementation solutions. After searching, I found that it could be solved by introducing a package, which is the JSR310 specification of mybatis.

 <dependency>
  <groupId>org.mybatis</groupId>
  <artifactId>mybatis-typehandlers-jsr310</artifactId>
  <version>1.0.1</version>
</dependency>

Why can he solve this problem? I looked at the code in his package. Isn't that just adding a TypeHandler to handle it myself.

I looked again at the implementation of OffsetDateTimeTypeHandler , in fact, it solved it by itself, and returned it directly to OffsetDateTime , it will not go into the logic of ShardingSphere at all, that is, he can solve it the cause of this problem.

Of course, if you don't want to be so troublesome to introduce a package, you can also take it out and specify it yourself. This is very simple, so I won't say more.

mention PR

So I thought, this thing is so simple, I might as well mention a PR to the official, here I teach you how to mention PR.

Because it's not our project, we can't push the code, so enter the project, and then fork , after the fork is done, just clone the project and execute the command.

 git remote add upstream https://github.com/apache/shardingsphere.git

Through the command, we can see that it is successful, so it is OK, and then pull the branch normally and write the code.

After writing, go to our project interface to submit PR as normal, and then you can.

trouble

Of course, the process was not so smooth, although it was only a very simple modification.

First of all, this check gave me an error, the first point told me not to use the * number to quote.

This is actually the pot of IDEA . If there are too many classes in the same package, it will automatically turn it into an asterisk for us. We can find it in Editor-Code Style-Java and then find the one under Imports. For these two options, just change them to 99 to prevent him from automatically changing us to an asterisk.

There are some others, such as if there is no space after it, this is because I forgot to format it!

Then the big guy replied that he couldn't stand it anymore, this code is too disgusting, and said whether we can use java.time.temporal.TemporalAccessor to judge, otherwise, there are so many types of time, what a woolen thread.

Then I translated a paragraph of English. I don’t know if the big guy understood it. I told him that this is not good. Look at this interface. Many messy classes implement it. In fact, I think we cover the commonly used Some of them are fine, and other special time types can be handled by TypeHandler themselves.

The boss said, um, of course, there is no way to judge this interface, so we have nothing to do, I said that it can't be.

In fact, there are still many time types that he will report errors. The best way is to abstract this out and use the implementation class separately with Mybatis, but then it will be a lot of work. I'm too lazy, that's all.

. . .