Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid usage of reflection API in Enum deserialization #840

Closed
andreaTP opened this issue Nov 20, 2023 · 4 comments · Fixed by #843
Closed

Avoid usage of reflection API in Enum deserialization #840

andreaTP opened this issue Nov 20, 2023 · 4 comments · Fixed by #843
Assignees
Labels
enhancement New feature or request WIP
Milestone

Comments

@andreaTP
Copy link
Contributor

return (T) targetEnum.getMethod("forValue", String.class).invoke(null, rawValue);

This is extremely unsafe and it works only because there is a "hidden" contract with the generator to emit the specific forValue method on the enum class.
My first reaction would be to make it more similar to a normal "class", and extend the API of ValuedEnum to cover the deserialization explicitly.

Please note that this is going to be a breaking API change for abstractions and I hope we can fix it before GA.

Do you have any prior art on the subject?
Any consideration?

@baywet
Copy link
Member

baywet commented Nov 20, 2023

Thanks for bringing this up.

The reason why we had to resolve to reflections are because it's a static method, so it can't be part of the ValuedEnum interface like getValue, because the return type varies (based on the enum type, but maybe we could get around that with generics).
And this is a static method because we don't have an instance yet.

See this example
https://github.com/microsoft/kiota-samples/blob/ca6f9e6f07fc441d4be167aa55bb1afa75c1c909/msgraph-mail/java/utilities/src/main/java/graphjavav4/utilities/models/BodyType.java#L17

If you have a better solution, let me know, I agree that reflection is costly and risky.

@andreaTP
Copy link
Contributor Author

And this is a static method because we don't have an instance yet.

Right, but this is a design choice, we can make it a class method and have a static way of retrieving an instance ( similar to what createFromDiscriminatorValue does.
Alternatively, we can add an interface at the parent level, but this means that no enum can be serialized without the scope of an object which seems to be a limitation.

@baywet
Copy link
Member

baywet commented Nov 20, 2023

I think the first option would probably work better here. In fact this is what we're doing in Go I think the history here was that I implemented it in dotnet, patched it in Java, later on implemented it in Go, and never went back to update java 🤦‍♂️

Is this something you want to take a look at?

@baywet baywet added the enhancement New feature or request label Nov 20, 2023
@baywet
Copy link
Member

baywet commented Nov 20, 2023

For visibility for everyone, here are the changes we're talking about:

  1. introduce an "ValuedEnumParser" interface (generic, defines a forValue method, functional)
  2. make the getEnumValue, and getCollectionOfEnumValues method accept that as a second parameter
  3. update the generation so it passes a lambda to the method as a second argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants