Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow a java array object on ArrayTypeHandler#setNonNullParameter #1548

Merged
merged 6 commits into from May 27, 2019

Conversation

@dirk-olmes
Copy link
Contributor

commented May 22, 2019

ArrayTypeHandler unwraps the contents of the java.sql.Array when fetching data. Upon insertion it expects an instance of java.sql.Array as the parameter.

This changeset improves ArrayTypeHandler so that it accepts an Object[] parameter for insert, constructing the java.sql.Array object if necessary. This aproach relies on the jdbcType of the column - it must reflect the array element's type. For setting NULL values on ARRAY columns I had to refactor BaseTypeHandler a bit - but I'm quite sure that this won't break anything.

dirk-olmes added some commits May 22, 2019

Improve ArrayTypeHandler
ArrayTypeHandler unwraps the contents of the java.sql.Array when fetching data. Upon insertion it expects an instance of java.sql.Array as the parameter.

This changeset improves ArrayTypeHandler so that it accepts an Object[] parameter for insert, constructing the java.sql.Array object if necessary. This aproach relies on the jdbcType of the column - it must reflect the array element's type. For setting NULL values on ARRAY columns I had to refactor BaseTypeHandler a bit - but I'm quite sure that this won't break anything.
@harawata
Copy link
Member

left a comment

Hi @dirk-olmes ,
Thank you very much!
It looks pretty good (the null-handling is perfect).
Just one minor request for now.

Regarding the first argument of createArrayOf() (i.e. typeName), I came to think it is better to define/use internal Java-to-SQL type mapping rather than forcing users to specify jdbcType.
Also, technically, the correct jdbcType is ARRAY and specifying different JDBC type is kind of a hack.
Do you (or anyone) have any opinion about this?

throw new TypeException("ArrayType Handler requires SQL array or java array parameter and does not support type " + parameter.getClass());
}
Object[] values = (Object[])parameter;
array = ps.getConnection().createArrayOf(jdbcType.name(), values);

This comment has been minimized.

Copy link
@harawata

harawata May 23, 2019

Member

We should 'free' this array after calling setArray() (the array that is passed as the parameter should be freed by user).

This comment has been minimized.

Copy link
@dirk-olmes

dirk-olmes May 24, 2019

Author Contributor

OK I have modified the PR to free the array.

IMHO the JDBC spec as well as the Array javadoc is confusing. They both talk about the Array being invalid after free(). In this case the Array is used further by the PreparedStatement, though (the values of the Array haven't yet been sent to the database server). This is why I left out the call to free() at first.

This comment has been minimized.

Copy link
@harawata

harawata May 25, 2019

Member

Thank you for the update and I realized that I should have been clearer (sorry!).
If the parameter is an Array, we should let user code free it because it is possible that the same instance is referenced multiple times in the same query.

if (parameter instanceof Array) {
  // no free() in this case
  ps.setArray(i, (Array)parameter);
} else {
  if (!parameter.getClass().isArray()) {
    throw new TypeException("ArrayType Handler requires SQL array or java array parameter and does not support type " + parameter.getClass());
  }
  String typeName = resolveTypeName(parameter.getClass().getComponentType());
  Array array = ps.getConnection().createArrayOf(typeName, (Object[])parameter);
  ps.setArray(i, array);
  array.free();
}

resolveTypeName() method is explained in the other comment.

This comment has been minimized.

Copy link
@dirk-olmes

dirk-olmes May 27, 2019

Author Contributor

I have updated the branch with a type mapping in the form that you described. I have included the defaults for all regular java types but not for interface types like java.sql.Clob etc. Mapping those would require a different approach than a simple map and I think they'd be a very special case anyway.

@dirk-olmes

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Regarding the first argument of createArrayOf() (i.e. typeName), I came to think it is better to define/use internal Java-to-SQL type mapping rather than forcing users to specify jdbcType.
Also, technically, the correct jdbcType is ARRAY and specifying different JDBC type is kind of a hack.
Do you (or anyone) have any opinion about this?

I agree that (ab)using the jdbcType is a hack.
Somehow you need to define the mapping between the type of the array's elements and the JDBC type on the database. This can either be implicit i.e. by using the mapping you describe or must be defined explicitly by the user.

I am fine with an implicit mapping but I am not sure about use cases where users wanting to influence this internal mapping. Then again, the JDBC spec clearly defines which Java type maps to which JDBC type and users should not be mucking with that.

The only thing that will no longer work with this approach is mapping collections - due to the type erasure.

@harawata

This comment has been minimized.

Copy link
Member

commented May 25, 2019

How about defining a protected method?

protected String resolveTypeName(Class<?> componentType) {
    return STANDARD_MAPPING.getOrDefault(componentType, "JAVA_OBJECT");
}

Users can override this method in a subclass if necessary.
STANDARD_MAPPING is a static ConcurrentHashMap.

The only thing that will no longer work with this approach is mapping collections - due to the type erasure.

I would say mapping Collection to ARRAY is out-of-scope of this type handler.
getNullableResult() methods don't return Collection anyway.

dirk-olmes and others added some commits May 27, 2019

Introduce a type mapping
Detect the type of the Array from the component type of the Object[] that's passed in as parameter.

Only regular Java types are mapped - for mapping interface types like java.sql.Clob etc. users would have to override the resolveTypeName() method.

@harawata harawata merged commit f7ce783 into mybatis:master May 27, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@harawata harawata self-assigned this May 27, 2019

@harawata harawata added this to the 3.5.2 milestone May 27, 2019

@harawata

This comment has been minimized.

Copy link
Member

commented May 27, 2019

Hi @dirk-olmes ,
Thanks a lot! I have made a few adjustment and merged it.
If you could, please verify the fix with the latest 3.5.2-SNAPSHOT to make sure everything is alright.

@dirk-olmes dirk-olmes deleted the dirk-olmes:ArrayTypeHandlerImprovements branch May 28, 2019

@dirk-olmes

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

I'm testing my use case with 3.5.2-SNAPSHOT and everything works fine so far. Two more things, though:

  1. Looking at TypeHandlerRegistry I assume that type handlers are effectively singletons per Configuration instance? May it make sense to make ArrayTypeHandler's STANDARD_MAPPING non-static as the entire type handler is singleton? A friend of mine mentioned that the static map may be trouble when unloading the class.

  2. I'm pre-registering the ArrayTypeHandler for some array types I use in my code to avoid excessive config in the mapping statement. Is there any reason that TypeHandlerRegistry's register(Class, JdbcType, TypeHandler) while register(Type, JdbcType, TypeHandler) is not? I don't like the casting work I have to do in my code ...

@harawata

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Thank you for testing, @dirk-olmes !

Static maps are used in various places, so it should not be a problem, I think.
If there is any verifiable issue, please let us know and we'll reconsider.

Regarding 2, I couldn't understand the question, sorry.
Could you please rephrase it for me?

@kazuki43zoo kazuki43zoo changed the title Improve ArrayTypeHandler Allow a java array object on ArrayTypeHandler#setNonNullParameter Jul 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.