release 2.0.8

[baywet]

No description provided.

[MIchaelMainer]

I want to block this release. I have a concern with the null serialization PR. I will add more info in a bit after I review the scenario. I believe there may be an unintended consequence that I'm investigating.

[MIchaelMainer]

We may be fine, but I will feel better if we document this in the Java doc comments as a mitigation.

The side-effect of setting serializeNull on the defaultserializer is that PATCH requests sent with serializeNull == true result in unintended null values being sent for all properties not explicitly set on the PATCH object. Since serializeNull is not the default, we can be fine. We should consider adding a doc comment to the new overload so that it is clear that serializeNull should not be set on the defaultserializer when PATCH requests are sent. This could result in the unintended setting values to null, or the service API returning an exception with the unexpected attempt to set a value to null.

Another scenario to consider and that should be documented on the overload is the POST scenario. We POST an object with serializeNull == true will result in all unset properties being sent as null. Depending on the workload, we may see errors around attempts to set read-only properties. For example, if I perform the following with serializeNull:

User user = new User();
user.displayName = "Al Pacino";

final User result = graphClient
  .users()
  .buildRequest()
  .post(user);

All of the properties defined on the User class besides displayName will be serialized to null, including read-only properties. Depending on the service, it could respond with an exception.

[baywet]

Hey @MIchaelMainer,
Thanks for bringing this up. I put together PR #262 and which should hopefully address the concerns you raised.
As you mentionned, we should be fine because all the default instantiation relies on the overloads without the parameter which defaults to false.
For someone to get in trouble they'd have to:

1. instantiate a new default serializer with the option set to true
2. pass it along to the graph client builder

Have a look at the other PR and let me know if you think I missed anything.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

81.6% Coverage
0.0% Duplication

[MIchaelMainer]

Thank you @baywet. BackingStore feature from Kiota will be so nice for addressing this scenario. Is there a way to selectively serialize null for a property?

[baywet]

@MIchaelMainer are you referring to this? microsoft/kiota#243 Yes the dirty tracking could in theory tell us which properties changed to null, so the serializer can add null property values to the payload, which would fix our PATCH issue.

@ramsessanchez it seems the work you're doing on the pipeline is blocking this PR here (the hooks are registered, but the pipelines definitions are missing from this branch). Do you want to wait for #257 to be finished and use those pipelines to deploy or do you want me to override this, merge as admin, and release with the old pipelines?

[ramsessanchez]

@ramsessanchez it seems the work you're doing on the pipeline is blocking this PR here (the hooks are registered, but the pipelines definitions are missing from this branch). Do you want to wait for #257 to be finished and use those pipelines to deploy or do you want me to override this, merge as admin, and release with the old pipelines?

@baywet I think it would be best to override. I've already gone ahead and paused the new pipelines and re-activated the sandbox ones that we've been using. I think that since this update has been requested for a bit now, it would be best to deploy the work that's been done until now and that we're currently satisfied with.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

81.6% Coverage
0.0% Duplication