fix: add support to deserialize to custom Lists and Maps#337
fix: add support to deserialize to custom Lists and Maps#337schmidt-sebastian merged 3 commits intogoogleapis:masterfrom
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
schmidt-sebastian
left a comment
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
|
@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? |
|
@schmidt-sebastian ,Thanks i will update this ASAP. |
|
a nit:
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: |
0b29cd8 to
57e18a9
Compare
25e3880 to
a4e8e47
Compare
| throw deserializeError( | ||
| context.errorPath, | ||
| String.format( | ||
| "Unable to deserialize to the %s type: %s", |
There was a problem hiding this comment.
| "Unable to deserialize to the %s type: %s", | |
| "Unable to deserialize to %s: %s", |
| throw deserializeError( | ||
| context.errorPath, | ||
| String.format( | ||
| "Unable to deserialize to the %s type: %s", rawType.getSimpleName(), e.toString())); |
There was a problem hiding this comment.
| "Unable to deserialize to the %s type: %s", rawType.getSimpleName(), e.toString())); | |
| "Unable to deserialize to %s: %s", |
ad6d398 to
8843b5a
Compare
🤖 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).
Fixes #318