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

feat: add support for ARRAY<JSON> type in Spring Data Spanner #1157

Merged
merged 16 commits into from
Aug 22, 2022
Merged

Conversation

zhumin8
Copy link
Contributor

@zhumin8 zhumin8 commented Jun 3, 2022

support ARRAY spanner type.

  • read: StructAccessor.java and StructPropertyValueProvider.java
  • write: ConverterAwareMappingSpannerEntityWriter.java
  • sql query when return type is ARRAY field.
    • SqlSpannerQuery.java and SpannerPersistentEntityImpl.java
  • test and sample

closes #651.

@zhumin8 zhumin8 changed the title Add support for ARRA<JSON> type in Spring Data Spanner Add support for ARRAY<JSON> type in Spring Data Spanner Jun 6, 2022
@zhumin8 zhumin8 changed the title Add support for ARRAY<JSON> type in Spring Data Spanner feat: Add support for ARRAY<JSON> type in Spring Data Spanner Jun 6, 2022
@zhumin8 zhumin8 changed the title feat: Add support for ARRAY<JSON> type in Spring Data Spanner feat: add support for ARRAY<JSON> type in Spring Data Spanner Jun 6, 2022
@zhumin8 zhumin8 marked this pull request as ready for review June 6, 2022 19:12
Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks amazing. I only have a few optional nits.

Comment on lines 176 to 185
private <T> List<T> getListJsonValue(int colIndex, Class<T> colType) {
if (this.struct.getColumnType(colIndex).getCode() != Code.ARRAY) {
throw new SpannerDataException(EXCEPTION_NARRATIVE_COL_NOT_ARRAY + colIndex);
}
List<String> jsonStringList = this.struct.getJsonList(colIndex);
List<T> result = new ArrayList<>();
jsonStringList.forEach(item ->
result.add(gson.fromJson(item, colType)));
return result;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be able to reuse the same code for the two getListJsonValue definitions by passing struct -> struct.getJsonList(colIndex); and struct->struct.getJsonList(colName) as lambdas.

@Query(
"SELECT additionalDetails from :com.google.cloud.spring.data.spanner.test.domain.Trade:"
+ " where trader_id = @trader_id")
List<List<Details>> getAdditionalDetailsById(@Param("trader_id") String traderId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool that this works.

@sonarcloud
Copy link

sonarcloud bot commented Jun 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

93.3% 93.3% Coverage
0.0% 0.0% Duplication

Copy link

@ansh0l ansh0l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

QQ> I couldn't see a test for the behaviour around a malformed array of Json. Would there be any behavioural difference in what was happening prior to this PR (say it used to throw something like an UnimplementedException) vs after this PR (say it throws something like InvalidDataException)?

@@ -184,7 +215,8 @@ <T> T getSingleJsonValue(String colName, Class<T> colType) {
return gson.fromJson(jsonString, colType);
}

public <T> T getSingleJsonValue(int colIndex, Class<T> colType) {
//TODO: change this to private in next major release

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it because users will be using getJsonValue directly for single json value and array of json, instead of using separate methods getSingleJsonValue and getListJsonValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, getJsonValue is public and getSingleJsonValue and getListJsonValue does not need to be exposed. In fact, this method is mostly for internal usage. Users don’t need to interact with neither of these methods, as shown in the samples, user only need to add annotation of the dedicated field and use save()/find() as usual. But this method needs to be public because it is used outside of its package.

@zhumin8
Copy link
Contributor Author

zhumin8 commented Aug 16, 2022

@ansh0l To your question:
Before this implementation: there is no specific logic around ARRAY<JSON> to throw an UnimplementedException , it would fall through logic to process regular ARRAY and fail on conversion because it does not know what to convert to. For example on write, it would fail with Caused by: java.lang.IllegalArgumentException: Target type to convert to cannot be null

About malformed array of Json: User is expected to interact with entities (data model), with ARRAY<JSON> field as a member of List<some object class>. Between write and read, conversion is done by Spring Data registered converters where this value is parsed by Gson to and from json format. This step is where malformed Json may occur, and it is expected to be captured by Gson with exception like JsonParseException.

@zhumin8 zhumin8 merged commit 77437d0 into main Aug 22, 2022
@zhumin8 zhumin8 deleted the array-json branch August 22, 2022 14:04
kateryna216 added a commit to kateryna216/spring-cloud-gcp that referenced this pull request Oct 20, 2022
…CloudPlatform#1157)

support ARRAY spanner type.

read: StructAccessor.java and StructPropertyValueProvider.java
write: ConverterAwareMappingSpannerEntityWriter.java
sql query when return type is ARRAY field.
SqlSpannerQuery.java and SpannerPersistentEntityImpl.java
test and sample
closes GoogleCloudPlatform#651.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support for ARRAY<JSON> in spring data spanner
4 participants