-
Notifications
You must be signed in to change notification settings - Fork 695
Handle null params in Spring Data Spanner #2448
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: Just to rename the title of the PR to be more specific (null params for which service and in what context?) Looks good!
...gframework/cloud/gcp/data/spanner/repository/it/SpannerRepositoryInsertIntegrationTests.java
Outdated
Show resolved
Hide resolved
@@ -361,7 +361,8 @@ public static Statement buildStatementFromSqlWithArgs(String sql, List<String> t | |||
private static void bindParameter(ValueBinder<Statement.Builder> bind, | |||
Function<Object, Struct> paramStructConvertFunc, SpannerCustomConverter spannerCustomConverter, | |||
Object originalParam, Parameter paramMetadata) { | |||
if (ConversionUtils.isIterableNonByteArrayType(originalParam.getClass())) { | |||
Class propType = originalParam != null ? originalParam.getClass() : paramMetadata.getType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here about how you need to know the type of the parameter (even if you bind null
) which is why the type information is relevant for null?
Gonna close my PR in favor of this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but let's make sure we don't reduce the test coverage.
sqlSpannerQuery.execute(params); | ||
|
||
verify(this.spannerTemplate, times(1)).executeQuery(any(), any()); | ||
} | ||
|
||
@Test | ||
public void multiplePageableSortTest() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's replacing these 3 tests that you deleted?
…k/cloud/gcp/data/spanner/repository/it/SpannerRepositoryInsertIntegrationTests.java Co-authored-by: Daniel Zou <dzou@users.noreply.github.com>
LGTM - only thing left is I think the build is broken. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs 84.6% Coverage The version of Java (1.8.0_151) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11. |
Codecov Report
@@ Coverage Diff @@
## master #2448 +/- ##
============================================
- Coverage 74.13% 74.09% -0.04%
+ Complexity 2121 2118 -3
============================================
Files 264 264
Lines 7654 7652 -2
Branches 792 790 -2
============================================
- Hits 5674 5670 -4
+ Misses 1617 1614 -3
- Partials 363 368 +5
Continue to review full report at Codecov.
|
Finally! |
@s13o |
fixes #2443