KEMBAR78
fix: add support to deserialize to custom Lists and Maps by suraj-qlogic · Pull Request #337 · googleapis/java-firestore · GitHub
Skip to content

Conversation

@suraj-qlogic
Copy link
Contributor

Fixes #318

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 17, 2020
@suraj-qlogic suraj-qlogic self-assigned this Aug 17, 2020
@codecov
Copy link

codecov bot commented Aug 17, 2020

Codecov Report

Merging #337 into master will decrease coverage by 0.08%.
The diff coverage is 55.55%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #337      +/-   ##
============================================
- Coverage     72.76%   72.68%   -0.09%     
- Complexity     1045     1046       +1     
============================================
  Files            64       64              
  Lines          5512     5528      +16     
  Branches        681      685       +4     
============================================
+ Hits           4011     4018       +7     
- Misses         1289     1297       +8     
- Partials        212      213       +1     
Impacted Files Coverage Δ Complexity Δ
.../com/google/cloud/firestore/CustomClassMapper.java 76.84% <55.55%> (-0.73%) 111.00 <0.00> (+2.00) ⬇️
...in/java/com/google/cloud/firestore/BulkWriter.java 71.02% <0.00%> (-0.47%) 28.00% <0.00%> (-1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44c806c...8843b5a. Read the comment docs.

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

I think this looks good, but it seems like (despite what is mentioned in #318) the same problem exists in Map (we always return a HashMap). Should we fix both instances?

| IllegalAccessException
| NoSuchMethodException
| InvocationTargetException e) {
throw new RuntimeException(e);
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian Aug 17, 2020

Choose a reason for hiding this comment

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

Just FYI: While uncommon, I think this method would now throw if called on an type that represents an subinterface of List, e.g.:

interace MyList extends List {
}

class Pojo {
  MyList data;
}

I am however not 100% certain that is the case, but based on my reading of https://stackoverflow.com/questions/36564041/getdeclaredconstructor-on-an-interface, this would throw an NoSuchMethodException, whereas before it would just create an ArrayList (albeit of the wrong type). I don't know if there is a solution that doesn't involve creating a list of the wrong type, and if there isn't, we can probably leave this logic as is.

@suraj-qlogic
Copy link
Contributor Author

@schmidt-sebastian ,Yes you are right.Whenever I try to create an instance of interface it's produce NoSuchMethodException. Using reflection api won't provide support for creating an instance of interface.So alternative i think to produce deserialization error.

try {
	  result =(rawType == List.class)
		  ? new ArrayList<>(list.size())
		  : (List<Object>) rawType.getDeclaredConstructor().newInstance();
     }catch (InstantiationException
	    | IllegalAccessException
	    | NoSuchMethodException
	    | InvocationTargetException e) {
	  throw deserializeError(context.errorPath,String.format("Deserializing values to %s interface 
          is not supported : %s", rawType.getSimpleName(),e.toString()));
    }

@schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian ,Yes you are right.Whenever I try to create an instance of interface it's produce NoSuchMethodException. Using reflection api won't provide support for creating an instance of interface.So alternative i think to produce deserialization error.

That error message LGTM. Do you want to update the PR and also add Map support?

@suraj-qlogic
Copy link
Contributor Author

@schmidt-sebastian ,Thanks i will update this ASAP.

@dmitry-fa
Copy link
Contributor

a nit:

"Deserializing values to %s interface is not supported : %s"

rawType is not necessary to be an interface to cause a deserialization problem, it could be an abstract class, a class without the default constructor, or with the private default constructor.

I would rephrase: "Unable to deserialize to the %s type: %s"

@suraj-qlogic suraj-qlogic added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 21, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 21, 2020
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Aug 21, 2020
throw deserializeError(
context.errorPath,
String.format(
"Unable to deserialize to the %s type: %s",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Unable to deserialize to the %s type: %s",
"Unable to deserialize to %s: %s",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

throw deserializeError(
context.errorPath,
String.format(
"Unable to deserialize to the %s type: %s", rawType.getSimpleName(), e.toString()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Unable to deserialize to the %s type: %s", rawType.getSimpleName(), e.toString()));
"Unable to deserialize to %s: %s",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@schmidt-sebastian schmidt-sebastian changed the title fix: add support for deserialize a class extending ArrayList fix: add support for deserialize to custom Lists and Maps Aug 27, 2020
@schmidt-sebastian schmidt-sebastian changed the title fix: add support for deserialize to custom Lists and Maps fix: add support to deserialize to custom Lists and Maps Aug 27, 2020
@schmidt-sebastian schmidt-sebastian merged commit dc897e0 into googleapis:master Aug 27, 2020
@suraj-qlogic suraj-qlogic deleted the firestore-318 branch August 31, 2020 04:07
gcf-merge-on-green bot pushed a commit that referenced this pull request Sep 15, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
## [2.1.0](https://www.github.com/googleapis/java-firestore/compare/v2.0.0...v2.1.0) (2020-09-10)


### Features

* add method to set emulator host programmatically ([#319](https://www.github.com/googleapis/java-firestore/issues/319)) ([#336](https://www.github.com/googleapis/java-firestore/issues/336)) ([97037f4](https://www.github.com/googleapis/java-firestore/commit/97037f42f76e9df3ae458d4e2b04336e64b834c3)), closes [#210](https://www.github.com/googleapis/java-firestore/issues/210) [#190](https://www.github.com/googleapis/java-firestore/issues/190)
* add opencensus tracing support ([#360](https://www.github.com/googleapis/java-firestore/issues/360)) ([edaa539](https://www.github.com/googleapis/java-firestore/commit/edaa5395be0353fb261d954429c624623bc4e346))
* add support for != and NOT_IN queries ([#350](https://www.github.com/googleapis/java-firestore/issues/350)) ([68aff5b](https://www.github.com/googleapis/java-firestore/commit/68aff5b406fb2732951750f3d5f9768df6ee12b5))
* generate protos to add NOT_EQUAL, NOT_IN, IS_NOT_NAN, IS_NOT_NULL query operators ([#343](https://www.github.com/googleapis/java-firestore/issues/343)) ([3fb1b63](https://www.github.com/googleapis/java-firestore/commit/3fb1b631f8dd087f0f3e1c43363e9642f497993a))


### Bug Fixes

* **samples:** re-add maven exec config for Quickstart sample ([#347](https://www.github.com/googleapis/java-firestore/issues/347)) ([4c2329b](https://www.github.com/googleapis/java-firestore/commit/4c2329bf89ffab4bd3060e16e1cf231b7caf4653))
* add support to deserialize to custom Lists and Maps ([#337](https://www.github.com/googleapis/java-firestore/issues/337)) ([dc897e0](https://www.github.com/googleapis/java-firestore/commit/dc897e00a85e745f57f615460b29d17b7dd247c6))


### Dependencies

* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.9.0 ([#352](https://www.github.com/googleapis/java-firestore/issues/352)) ([783d41e](https://www.github.com/googleapis/java-firestore/commit/783d41e167c7c79957faeeebd7a76ab72b5b157d))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the googleapis/java-firestore API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't deserialize a class extending ArrayList

4 participants