[chore] Remove reflection usage from KiotaSerialization

[andreaTP]

I started picking up #923 again.
Before opening Java to GA I think it makes sense to remove those reflective methods, they look convenient, but I think that a cleaner smaller API is the way to go to start with.

Also, this will prevent accidental usage of reflection on platforms that do not support it completely (like GraalVM native-image).

In case we receive feedback and ask to re-introduce those helpers we can re-evaluate.

[github-actions]

This pull request has conflicting changes, the author must resolve the conflicts before this pull request can be merged.

[github-actions]

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

[baywet]

Thanks for the contribution.
The reflection option was requested by @sebastienlevert in the specification so people don't have to provide the factory method. It's a shorthand for simplicity and since there are overloads accepting the method I don't think it hurts to have those.
They are not used by the generated code.

[andreaTP]

Thanks for the context @baywet !

It's a shorthand for simplicity

If this is the desired outcome I'm convinced they should be removed.

Although at first look they provide a "nice to have" API which is slightly nicer than the plain one, the risk of invoking one of those methods and breaking a GraalVM native-image build is extremely high, especially when the error comes from a transitive dependency.

Debugging and understanding the issues with a native-image build is, at times, extremely challenging and I would give this gun to the users only as a deliberate choice and not masked behind a simple "convenience method".

They are not used by the generated code.

Correct, and it's very very little tested too ...

If, after these motivations, you still feel strongly about keeping the methods, I want to propose, at least, moving them to a location where the usage of reflection is very very clear, for example changing the package name:

com.microsoft.kiota.serialization.KiotaJsonSerialization -> com.microsoft.kiota.serialization.reflective.KiotaJsonSerialization

I'm sorry to push a little hard on this, but I have spent quite enough hours of my life debugging those issues 😢.
I settled on these rules of thumb:

• avoid reflection at all costs
• if you really need to use it, make it very explicit where and how it's used

[baywet]

These are most of the arguments I gave to @sebastienlevert + reliability/performance. I'll let you folks discuss that since I'm already in the camp of "we should not be doing any reflection"

[andreaTP]

Got it 👍
Personally, the only "unusual" detail in the reflection-free API is the name of the method IMHO: createFromDiscriminatorValue which is rather alien terminology for Java developers.

The resulting API looks cleaner with a more "idiomatic" name like:

KiotaSerialization.deserialize(jsonContentType, strValue, TestEntity::deserializer);

[github-actions]

This pull request has conflicting changes, the author must resolve the conflicts before this pull request can be merged.

[sebastienlevert]

In this case @andreaTP if we were to remove the reflection versions of these APIs, would the only thing left on your mind be the deserializer vs. createFromDiscriminatorValue? That even makes me think that this "wording" might be better in other languages but would impact our GA languages. @baywet any opinions on renaming this factory?

[andreaTP]

if we were to remove the reflection versions of these APIs

I would say that no further actions are required around reflection, all the rest falls in the category of "nice to have".

[sebastienlevert]

So current state is OK? Just to make sure I understand correctly?

[andreaTP]

Correct, the current status is OK and Kiota is compatible with native-image already.

To avoid introducing breaking changes after GA, I warmly encourage you to approve this PR, and we can move on from there.

[andreaTP]

Any update? Should I rebase or this is not going to go in?

[sebastienlevert]

This should go in. I agree with your concerns and better to avoid reflection in this case.

@baywet make your magic happen 😁

[baywet]

Just a heads up on that @sebastienlevert : this means that Java will be different from dotnet. But similar to Go/TS. But I'll proceed. Thanks everyone for the discussion.