Unit test to verify multi-level additional data setter #114

dadheech115 · 2018-08-16T11:55:12Z

Added a unit test case to verify multi-level additional data setter and added a comment to explain the IllegalArgumentException

Fixes #113

Changes proposed in this pull request

-Have Added a test function in AddtionalDataTests.java class.
-Have also added a comment in DefaultSerializer which explains the reason for not throwing IllegalArgumentException

…ded a comment to explain the IllegalArgumentException

davidmoten · 2018-08-17T00:20:08Z

src/main/java/com/microsoft/graph/serializer/DefaultSerializer.java

                }
            } catch (IllegalArgumentException | IllegalAccessException e) {
-                logger.logError("Unable to access child fields of " + serializedObject.getClass().getSimpleName(), e);
+                //Not throwing the IllegalArgumentException as the Serialized Object would still be usable even if the additional data is not set.


Not enough of an argument for me. Describe the situation where this error would happen (and unit test it) and and justify why you would not throw.

You need more unit test coverage of this method. Use a coverage tool till you have 100% coverage of every line and branch. What IDE are you using (Emma is good for Eclipse)?

I would argue that just because the Java reflection engine hands us a field that it won't let us access, isn't a good enough reason to throw an exception at the user. Especially when the only reason we are reflecting over it is because we are hunting for extra properties that might be there so that we can fill the additionalProperties collection.

I do agree that the try/catch is a bit too wide of a net and we should try and limit the lines we are actually swallowing exceptions for.

However, this brings me to my main concern here. If I understand this code, we are reflecting over every single property of every object that is returned from the Graph. This seems like an expensive solution to a problem that by definition is an exception case. The .Net library has the luxury of doing this because the deserializer library builds the list of unknown properties while it is parsing the JSON.

I really don't like the idea of running this chunk of reflection code on the retrieval of every single object. That just sounds like asking for trouble.

I would argue that just because the Java reflection engine hands us a field that it won't let us access, isn't a good enough reason to throw an exception at the user.

That may be the case, just want it documented and reasoned. All of the objects being serialized/deserialized are objects defined in the SDK though right? So we should know what the variation in access is across the model objects is and can test these cases.

I'm only brushing the surface with my review because I'm too busy to study this AdditionalData stuff properly but it does seem fair enough that if JSON from API has a field that is not represented in the object then it is ok to ignore that field.

However, if the Java object has a field that the JSON doesn't have then we will throw probably with a NullPointerException (NPE). That is ugly and should be fixed if only to throw a more meaningful exception but perhaps just to ignore (log). Needs consideration and needs unit testing!

msgraph-sdk-java/src/main/java/com/microsoft/graph/serializer/DefaultSerializer.java

Line 137 in b4ecc09

childAdditionalDataManager.setAdditionalData(rawJson.get(field.getName()).getAsJsonObject());

@davidmoten ,I agree that 100% code coverage is the ideal test case result that we should strive for. And we will further try to enhance this test case to cover most of the branching in the code.
As for the exceptions caught at the end of setting up additional data, these exceptions are thrown by field.get(object) method of java.lang.reflect.Field class. So, we are just catching it and not throwing it to break the flow.

No, you are catching IllegalArgumentException and IllegalAccessException. A NullPointerException will be thrown not caught. This is why unit tests are important (in fact thinking about all scenarios is important because full line and branch test coverage would not find this necessarily).

davidmoten · 2018-08-17T03:59:37Z

I notice also that the setChildAdditionalData is recursive. This sets us up for an infinite recursion if object A has a reference to B and B has a reference to A as a simple example. Do we need protection for this and if not why not?

davidmoten · 2018-08-17T05:10:52Z

I'm also concerned by

is it even appropriate to recursively set the additional data because we are not traversing the JSON as we traverse the object structure

Again, forgive the superficial review!

dadheech115 · 2018-08-17T09:28:12Z

@davidmoten , as for your concern about infinite recursion. As we are recursively traversing the JSON object returned from Graph server, there is no chance of one JSON object being referred from a sub-element. Hence there will not be any infinite recursion scenario.
And if I understand it correctly, you want to know why are we not traversing the json at the time we are deserializing other properties of a Model. So, the reason for that is we are using GSON library for object parsing. So instead of modifying the code of that library to include our logic of setting additional data, we thought it would be wise to once deserialize the object with all the properties available using this library [so not to reinvent the wheel] and then traverse and see if there are any additional values that have been left out from this parser.

davidmoten · 2018-08-21T07:45:33Z

@davidmoten , as for your concern about infinite recursion. As we are recursively traversing the JSON object returned from Graph server, there is no chance of one JSON object being referred from a sub-element. Hence there will not be any infinite recursion scenario.

Ok, thanks.

And if I understand it correctly, you want to know why are we not traversing the json at the time we are deserializing other properties of a Model. So, the reason for that is we are using GSON library for object parsing. So instead of modifying the code of that library to include our logic of setting additional data, we thought it would be wise to once deserialize the object with all the properties available using this library [so not to reinvent the wheel] and then traverse and see if there are any additional values that have been left out from this parser.

Ok, so I want to check this scenario. If

the object graph A -> B is deserialized (we end up with an object A that points to B)
and the json fragment for A has an unmapped property x = y (additional data)
and the json fragment for B has an unmapped property x = z (additional data)

Will the additional data for deserialized B end up with x = y instead of x = z?

Another scenario is

the object graph A -> B is deserialized (we end up with an object A that points to B)
and the json fragment for A has an unmapped property x = y (additional data)
and the json fragment for B has no unmapped properties

Will the additional data for deserialized B end up with x = y?

If these are valid scenarios I'd like the desired outcomes to be unit tested.

davidmoten · 2018-10-02T00:41:05Z

Until the scenarios I mention above have been addressed I think the original PR should be backed out of dev branch (and master if there).

deepak2016 · 2018-10-02T04:06:20Z

@davidmoten thanks for bringing this up, we will get working on fixing this issue.

github-actions · 2020-09-09T18:52:33Z

Conflicts have been resolved. A maintainer will take a look shortly.

baywet · 2020-09-09T20:03:04Z

Hey @davidmoten
I just did some light refactoring to improve readability and robustness.
I don't want to touch too much this section of the code as I'm still getting familiar with the code base at this point.
I believe most if not all the comments you had have been addressed in previous PRs.
Let me know if I missed anything.

src/main/java/com/microsoft/graph/serializer/DefaultSerializer.java

MIchaelMainer · 2020-09-17T18:29:51Z

src/main/java/com/microsoft/graph/serializer/DefaultSerializer.java

-                                final JsonElement elementRawJson = rawJsonArray.get(i);
-                                setChildAdditionalData((IJsonBackedObject) element, elementRawJson.getAsJsonObject());
+                        // If the object is a list of Graph objects, iterate through elements
+                        else if (fieldObject instanceof List) {


Add a test for fieldObject instanceof HashMap using serializer.deserializeObject(sourcestring, class);

One test for a homogenous list of objects, and one for a hetergenous list of objects.

added a unit test. We currently don't have a case where hashmaps have heterogeneous values and I didn't want to add too much fake code in the test setup.

MIchaelMainer · 2020-09-17T18:32:43Z

src/main/java/com/microsoft/graph/serializer/DefaultSerializer.java

-                            	childAdditionalDataManager.setAdditionalData(rawJson.get(field.getName()).getAsJsonObject().get(pair.getKey()).getAsJsonObject());
-                            	setChildAdditionalData((IJsonBackedObject) child,rawJson.get(field.getName()).getAsJsonObject().get(pair.getKey()).getAsJsonObject());
+                                // If the item is a valid Graph object, set its additional data
+                                if (child instanceof IJsonBackedObject) {


I'd like to see a test in case where the child is a OData primitive, do we add non-schematized primitive fields to additionalData?

tweaked the last unit test I added so it tests for that as well.

Added a test case to verify multi-level additional data setter and ad…

b4ecc09

…ded a comment to explain the IllegalArgumentException

darrelmiller added the in progress label Aug 16, 2018

dadheech115 requested a review from deepak2016 August 16, 2018 11:56

This was referenced Aug 16, 2018

Deserializing additional data throughout the object hierarchy #104

Merged

PR #104 work incomplete #113

Closed

dadheech115 requested a review from davidmoten August 16, 2018 12:00

davidmoten suggested changes Aug 17, 2018

View reviewed changes

baywet self-assigned this Aug 17, 2020

github-actions bot added the conflicting label Aug 18, 2020

This comment has been minimized.

Sign in to view

baywet added this to the 2.1.1 milestone Sep 4, 2020

Merge branch 'dev' into vidadhee/AdditionalDataManagerTest

c75b10d

github-actions bot removed the conflicting label Sep 9, 2020

baywet added 2 commits September 9, 2020 15:48

- removes duplicate test and adjusts faulty unit test

59fd088

- light refactoring for clarity and defensive programming

0059f66

baywet requested review from davidmoten and removed request for deepak2016 September 9, 2020 20:03

Merge branch 'dev' into vidadhee/AdditionalDataManagerTest

fcfdca7

baywet requested review from MIchaelMainer and zengin September 14, 2020 16:52

baywet added 2 commits September 15, 2020 07:57

Merge branch 'dev' into vidadhee/AdditionalDataManagerTest

d4f2c0f

Merge branch 'dev' into vidadhee/AdditionalDataManagerTest

d41b575

MIchaelMainer reviewed Sep 17, 2020

View reviewed changes

baywet added 6 commits September 17, 2020 15:16

- adds logging for edge cases to help with potential troubleshooting

cd27f49

Merge branch 'dev' into vidadhee/AdditionalDataManagerTest

e006914

Merge branch 'dev' into vidadhee/AdditionalDataManagerTest

0d4d6b2

- adds a unit test for hashmap properties

4831f58

- updates unit test to check for additional properties in child elements

4150c14

- add suppress warnings

d6881da

baywet requested a review from MIchaelMainer September 21, 2020 17:19

MIchaelMainer approved these changes Sep 21, 2020

View reviewed changes

baywet merged commit 54355c8 into dev Sep 21, 2020

baywet deleted the vidadhee/AdditionalDataManagerTest branch September 21, 2020 17:37

Unit test to verify multi-level additional data setter #114

Unit test to verify multi-level additional data setter #114

Uh oh!

Conversation

dadheech115 commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this pull request

Uh oh!

davidmoten Aug 17, 2018

Choose a reason for hiding this comment

Uh oh!

darrelmiller Aug 17, 2018

Choose a reason for hiding this comment

Uh oh!

davidmoten Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dadheech115 Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidmoten Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidmoten commented Aug 17, 2018

Uh oh!

davidmoten commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dadheech115 commented Aug 17, 2018

Uh oh!

davidmoten commented Aug 21, 2018

Uh oh!

davidmoten commented Oct 2, 2018

Uh oh!

deepak2016 commented Oct 2, 2018

Uh oh!

This comment has been minimized.

github-actions bot commented Sep 9, 2020

Uh oh!

baywet commented Sep 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MIchaelMainer Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

baywet Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

MIchaelMainer Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baywet Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dadheech115 commented Aug 16, 2018 •

edited

Loading

davidmoten Aug 17, 2018 •

edited

Loading

dadheech115 Aug 17, 2018 •

edited

Loading

davidmoten Aug 20, 2018 •

edited

Loading

davidmoten commented Aug 17, 2018 •

edited

Loading

MIchaelMainer Sep 17, 2020 •

edited

Loading