Sprout for fix #248

[PrimosK]

Sprout for fix which downcasts parametrized nested IJsonBackedObject… objects during deserialization.

fixes #248 (sprout)

As deserialization of (nested) IJsonBackedObject is dependent based on the payload (Odata.type) the only way of doing it was to extend FallbackTypeAdapterFactory accordingly. So if Odata.type is present a new sub type adapter is created/used otherwise default Gson adapter is used.

As I am ATM short on time to look into this further I would highly appreciate if someone with the better overview of the related codebase would take it over from there.

All in all - I can confirm that this change fixes the issue observed in #248.

[ghost]

All CLA requirements met.

[baywet]

thanks for starting to put all this together. I provided some initial feedback.

[baywet]

@PrimosK Also wanted to let you know about the following error from the api linting:

/home/runner/work/msgraph-sdk-java-core/msgraph-sdk-java-core/src/main/java/com/microsoft/graph/serializer/FallbackTypeAdapterFactory.java:133: Error: Access to private field logger of class FallbackTypeAdapterFactory requires synthetic accessor [SyntheticAccessor]
              this.derivedClassIdentifier = new DerivedClassIdentifier(logger);

[PrimosK]

Hi @baywet,

is there any progress with this pull request? Is there any further help needed?

One of the concerns I see (and could be improved) is the fact that adding new Type(Hiearchy)Adapter for IJsonBackedObject base type to (in the future):

https://github.com/PrimosK/msgraph-sdk-java-core/blob/59a3098489194d6446c1830e4c231517bf7aca25/src/main/java/com/microsoft/graph/serializer/GsonFactory.java#L331

... without adding it to:

https://github.com/PrimosK/msgraph-sdk-java-core/blob/59a3098489194d6446c1830e4c231517bf7aca25/src/main/java/com/microsoft/graph/serializer/FallbackTypeAdapterFactory.java#L103

... might result in unexpected behavior as com.microsoft.graph.serializer.FallbackTypeAdapterFactory.ODataTypeParametrizedIJsonBackedObjectAdapter might kick in instead.

With that in mind I wonder whether changing com.microsoft.graph.serializer.FallbackTypeAdapterFactory#create to something like this would be a better solution:

  public <T> TypeAdapter<T> create(@Nonnull final Gson gson, @Nonnull final TypeToken<T> type) {
      Objects.requireNonNull(type, "parameter type cannot be null");
      final Class<T> rawType = (Class<T>) type.getRawType();

      if (rawType.isEnum()) {
          return new EnumTypeAdapter<>(rawType, logger);
      } else if (rawType == Void.class) {
          return (TypeAdapter<T>) voidAdapter;
      } else if (IJsonBackedObject.class.isAssignableFrom(type.getRawType())) {

          final TypeAdapter<IJsonBackedObject> delegatedAdapter = (TypeAdapter<IJsonBackedObject>) gson.getDelegateAdapter(this, type);

          // Avoid overriding custom IJsonBackedObject type adapters defined in GsonFactory
          if (!(delegatedAdapter instanceof ReflectiveTypeAdapterFactory.Adapter)) {
              return null;
          }

          return (TypeAdapter<T>) new ODataTypeParametrizedIJsonBackedObjectAdapter(gson, delegatedAdapter, (TypeToken<IJsonBackedObject>) type, logger);
      }
      else {
          return null;
      }
  }

[baywet]

@PrimosK sorry, I thought you were still working on it since it's still in a draft state. When you are ready, please switch it from draft to open via the "ready to review" button.

The main missing thing I can see at this point are additional unit tests to test for the case we're fixing in this PR.

Having something dynamic that relies on the factory would probably be better indeed if the performance impact is not too high. This would prevent us from potentially shooting ourselves in the foot in the future. Thanks for considering it.

[PrimosK]

BTW - I guess I can't do nothing about static analysis with SonarCloud failing:

Execution failed for task ':sonarqube'.
You're not authorized to run analysis. Please contact the project administrator.

[baywet]

thanks for making changes, a few minor comments left.
And no, sonarqube requires access to a secret, and PRs from forks don't have access to secrets for security reasons, we'll run it once it gets merged.

[PrimosK]

I think that should be it.

In the future some improvements might be made also WRT:

com.microsoft.graph.serializer.IJsonBackedObject#setRawObject
com.microsoft.graph.serializer.IJsonBackedObject#additionalDataManager

.. being called from various places.

I think these could be simply called inside:

com.microsoft.graph.serializer.ODataTypeParametrizedIJsonBackedTypedAdapter#read

... but I am considering this a separate issue to keep the scope of this PR manageable.

[baywet]

LGTM! thanks for making all the adjustments! @ramsessanchez can you have a look as well please? and run a few tests locally?

[PrimosK]

It's nice to see that this fix was merged to 2.0.8. Are there any plans/approximations of when new 2.0.8 release will be out?

[baywet]

We'll probably take advantage of this release to test the migration of our release pipelines (last item in the milestone). The work has started on that, so let's have a tentative date by the end of next week. (ramses here just joined the team, and we need to account for the ramp-up)