DATAJDBC-327 - Adds support for conversion to JdbcTypeAware and storing byte[] as binary. #123

schauder · 2019-03-07T12:59:12Z

Some JDBC drivers depend on correct explicit type information in order to pass on parameters to the database.
So far CustomConversion had no way to provide that information.
With this change one can use @WritingConverter that converts to JdbcTypeAware in order to provide that information.

byte[] now also get stored as BINARY.

See also: DATAJDBC-239.

mp911de

I left a few comments. Looks pretty decent and I think we should add some more Javadoc along with some assertions.

mp911de · 2019-03-11T10:04:59Z

...ng-data-jdbc/src/main/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategy.java

@@ -292,15 +288,18 @@ public long count(Class<?> domainType) {
 		return result;
 	}

-	private <S, T> MapSqlParameterSource getPropertyMap(S instance, RelationalPersistentEntity<S> persistentEntity,
-			String prefix) {
+	private <S, T> MapSqlParameterSource getParameterSource(S instance, RelationalPersistentEntity<S> persistentEntity,


Nit: How about Predicate< RelationalPersistentProperty> instead of skipId?

mp911de · 2019-03-11T10:06:55Z

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/JdbcTypeAware.java

+ * @since 1.1
+ */
+@Value(staticConstructor = "of")
+public class JdbcTypeAware {


How about naming this type JdbcValue?

mp911de · 2019-03-11T10:07:24Z

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/JdbcTypeFactory.java

+	 * needs to be provided anyway, mostly for providing backward compatibility. Calling it will result in an exception.
+	 * The features normally supported by a {@link JdbcTypeFactory} will not work.
+	 */
+	JdbcTypeFactory DUMMY_JDBC_TYPE_FACTORY = value -> {


How about naming this one SIMPLE? From a design perspective, we could turn the field into a static interface method to align with our API style.

I don't like SIMPLE because it is kind of less than simple: Not really working. How about UNSUPPORTED?

mp911de · 2019-03-11T10:09:06Z

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/JdbcTypeFactory.java

+	 * @param value the value to be converted. Must not be {@literal null}.
+	 * @return an {@link Array}. Guaranteed to be not {@literal null}.
+	 */
+	Array createArray(Object[] value);


This is very nice that we do not declare SQLException 👍

mp911de · 2019-03-11T10:10:03Z

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/support/package-info.java

@@ -0,0 +1,7 @@
+/**


Nit: Package declarations do not get author tags.

mp911de · 2019-03-11T10:10:44Z

...org/springframework/data/jdbc/repository/JdbcRepositoryCustomConversionIntegrationTests.java

+	/**
+	 * In PostrgreSQL this fails if a simple converter like the following is used.
+	 *
+	 * <pre>


Nit: How about <pre class="code">? This makes formatting a bit simpler.

mp911de · 2019-03-11T10:14:15Z

.../main/java/org/springframework/data/relational/core/conversion/BasicRelationalConverter.java

@@ -193,6 +193,30 @@ public Object writeValue(@Nullable Object value, TypeInformation<?> type) {
 		return conversionService.convert(value, rawType);
 	}

+
+	@Override
+	public boolean canWriteValue(Object value, TypeInformation<?> type) {


Let's keep this method on JDBC level. The underlying assumption is that a module using the converter is able to persist entities as values.

From how the method is used, I would even suggest to keep it private and consider public exposure at a later time.

The main implementation is in the BasicRelationalConverter based on the conversions service.

If we don't want to expose it there we could drop it completely and rely on exception handling, possibly even caching failures.

Not sure that is a good way to do it though.
I'm also not sure I understood your comment about the underlying assumption.

mp911de · 2019-03-11T10:20:11Z

...g-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/BasicJdbcConverter.java

+		}
+
+		if (componentType == Byte.class) {
+			Byte[] boxedBytes = (Byte[]) convertedValue;


Let's extract the code into a package-private utility (ArrayUtil.toPrimitive(…)) or something like that.

mp911de · 2019-03-11T10:22:19Z

...ta-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/DefaultJdbcTypeFactory.java

+
+	private final JdbcOperations operations;
+
+	public DefaultJdbcTypeFactory(JdbcOperations operations) {


Nit: Assertion for Non-null.

…ng byte[] as binary. Some JDBC drivers depend on correct explicit type information in order to pass on parameters to the database. So far CustomConversion had no way to provide that information. With this change one can use @WritingConverter that converts to JdbcTypeAware in order to provide that information. byte[] now also get stored as BINARY. See also: DATAJDBC-239.

Removed author tag for package-info.java

schauder · 2019-03-11T14:12:18Z

I fixed all the mentioned issues.

…[] as binary. Some JDBC drivers depend on correct explicit type information in order to pass on parameters to the database. So far CustomConversion had no way to provide that information. With this change one can use @WritingConverter that converts to JdbcTypeAware in order to provide that information. byte[] now also get stored as BINARY. Original pull request: #123.

Removed author tag for package-info.java Original pull request: #123.

Extend Javadoc. Reorder constructors in the order of their parameter number. Add missing assertions. Introduce utility to create predicates. Remove unused fields. Add since tags. Reformat. Original pull request: #123.

mp911de · 2019-03-12T11:34:19Z

That's merged and polished now.

…[] as binary. Some JDBC drivers depend on correct explicit type information in order to pass on parameters to the database. So far CustomConversion had no way to provide that information. With this change one can use @WritingConverter that converts to JdbcTypeAware in order to provide that information. byte[] now also get stored as BINARY. Original pull request: spring-projects#123.

Removed author tag for package-info.java Original pull request: spring-projects#123.

Extend Javadoc. Reorder constructors in the order of their parameter number. Add missing assertions. Introduce utility to create predicates. Remove unused fields. Add since tags. Reformat. Original pull request: spring-projects#123.

Original pull request: #123.

Add extension for UpdateTableSpec.table. Add tests. Formatting. Original pull request: #123.

mp911de requested changes Mar 11, 2019

View reviewed changes

schauder added 2 commits March 11, 2019 12:25

DATAJDBC-327 - Prepare branch.

48df427

schauder force-pushed the issue/DATAJDBC-327 branch from 4afac46 to 9054265 Compare March 11, 2019 11:27

schauder added 2 commits March 11, 2019 15:09

DATAJDBC-327 - Address review comments.

373d2d3

DATAJDBC-327 - Polishing.

c6871ca

Removed author tag for package-info.java

schauder force-pushed the issue/DATAJDBC-327 branch from 9054265 to c6871ca Compare March 11, 2019 14:10

mp911de pushed a commit that referenced this pull request Mar 12, 2019

DATAJDBC-327 - Polishing.

35d9bb6

Removed author tag for package-info.java Original pull request: #123.

mp911de closed this Mar 12, 2019

mp911de deleted the issue/DATAJDBC-327 branch March 12, 2019 13:07

THD-Thomas-Lang pushed a commit to THD-Thomas-Lang/spring-data-jdbc that referenced this pull request Apr 12, 2019

DATAJDBC-327 - Polishing.

5db0fe2

Removed author tag for package-info.java Original pull request: spring-projects#123.

spring-projects-issues mentioned this pull request Dec 31, 2020

Add support for conversion to JdbcValue and store byte[] as binary [DATAJDBC-327] #549

Closed

mp911de pushed a commit that referenced this pull request Feb 21, 2022

#122 - Improve Kotlin extensions for CriteriaStep and DatabaseClient.

9bcaa24

Original pull request: #123.

mp911de added a commit that referenced this pull request Feb 21, 2022

#122 - Polishing.

972bc09

Add extension for UpdateTableSpec.table. Add tests. Formatting. Original pull request: #123.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

DATAJDBC-327 - Adds support for conversion to JdbcTypeAware and storing byte[] as binary. #123

DATAJDBC-327 - Adds support for conversion to JdbcTypeAware and storing byte[] as binary. #123

Uh oh!

schauder commented Mar 7, 2019

Uh oh!

mp911de left a comment

Uh oh!

mp911de Mar 11, 2019

Uh oh!

mp911de Mar 11, 2019

Uh oh!

mp911de Mar 11, 2019

Uh oh!

schauder Mar 11, 2019

Uh oh!

mp911de Mar 11, 2019

Uh oh!

mp911de Mar 11, 2019

Uh oh!

mp911de Mar 11, 2019

Uh oh!

mp911de Mar 11, 2019

Uh oh!

schauder Mar 11, 2019

Uh oh!

mp911de Mar 11, 2019

Uh oh!

mp911de Mar 11, 2019

Uh oh!

schauder commented Mar 11, 2019

Uh oh!

mp911de commented Mar 12, 2019

Uh oh!

Uh oh!


		private final JdbcOperations operations;

		public DefaultJdbcTypeFactory(JdbcOperations operations) {

DATAJDBC-327 - Adds support for conversion to JdbcTypeAware and storing byte[] as binary. #123

DATAJDBC-327 - Adds support for conversion to JdbcTypeAware and storing byte[] as binary. #123

Uh oh!

Conversation

schauder commented Mar 7, 2019

Uh oh!

mp911de left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schauder commented Mar 11, 2019

Uh oh!

mp911de commented Mar 12, 2019

Uh oh!

Uh oh!