SendPrimitiveAsync throws InvalidOperationException for enums

[MartinM85]

Closes microsoft/kiota-dotnet#274

[baywet]

Thanks for this great PR! A couple of remarks to push it to completion

[baywet]

We're going to need the AOT annotation on the generic type so people don't get errors when compiling with trimming.
https://github.com/microsoft/kiota-serialization-json-dotnet/blob/d0454daecb7ec2327f3e55d642fa510cf617e8b5/src/JsonParseNode.cs#L579

[baywet]

The AOT attribute will also be required in the public method declaration I believe.

[MartinM85]

Do you mean something like this?

public async Task<ModelType?> SendPrimitiveAsync<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)] ModelType>(RequestInformation requestInfo, Dictionary<string, ParsableFactory<IParsable>>? errorMapping = default, CancellationToken cancellationToken = default)

[baywet]

correct

[MartinM85]

@andrueastman @baywet
It's probably a breaking change which requires changes in Microsoft.Kiota.Abstractions

error IL2095: 'DynamicallyAccessedMemberTypes' in 'DynamicallyAccessedMembersAttribute' on the generic parameter 'ModelType' of 'Microsoft.Kiota.Http.HttpClientLibrary.HttpClientRequestAdapter.SendPrimitiveAsync(RequestInformation, Dictionary<String, ParsableFactory>, CancellationToken)' don't match overridden generic parameter 'ModelType' of 'Microsoft.Kiota.Abstractions.IRequestAdapter.SendPrimitiveAsync(RequestInformation, Dictionary<String, ParsableFactory>, CancellationToken)'. All overridden members must have the same 'DynamicallyAccessedMembersAttribute' usage.

Is DynamicallyAccessedMembersAttribute necessary for SendPrimitiveAsync? Reflection is used on the underlying type, not directly on ModelType.

[baywet]

I don't know the internals of AOT/trimming/the type system well enough to have a definitive answer answer to that question.
For changes impacting parse node and others, we did update the original interfaces, and that didn't result in a breaking change for anyone besides maybe people already doing AOT, but they were already broken.
I guess a first step here could be to look at the implementation/annotations on Nullable.GetUnderlyingType(modelType) and see what it requires (any dynamic attribute?) to know whether we need to propagate things here.

[MartinM85]

At first sight, no dynamic attribute is used in the implementation

[andrueastman]

I think if we move the functionality to the abstractions side, things will become clearer/different. So, let's make that change first...

[MartinM85]

Do you think that similar fix for SendPrimitiveCollectionAsync should go there or to https://github.com/microsoft/kiota-serialization-json-dotnet/blob/main/src/JsonParseNode.cs#L310 ?

[baywet]

Do you think that similar fix for SendPrimitiveCollectionAsync should go there or to https://github.com/microsoft/kiota-serialization-json-dotnet/blob/main/src/JsonParseNode.cs#L310 ?

I think the similar fix should be implemented in the json parse node, but in that case, I think it'd be beneficial to move all the code getting the enum value in the abstractions so we don't duplicate it. Thoughts @andrueastman ?

[andrueastman]

Do you think that similar fix for SendPrimitiveCollectionAsync should go there or to https://github.com/microsoft/kiota-serialization-json-dotnet/blob/main/src/JsonParseNode.cs#L310 ?

That's right. We will need a similar fix in the parsenode as well.

I think it'd be beneficial to move all the code getting the enum value in the abstractions so we don't duplicate it

I agree with this too. Let's definitely do this if we can.

[MartinM85]

So, what's the plan?
Merge this PR and prepare some helpers in kiota-abstractions-dotnet for parsing enums (as part of another ticket)?
Then update kiota-http-dotnet and kiota-serialization-json-dotnet to use enum helpers from kiota-abstractions-dotnet?
It would be much easier once microsoft/kiota-dotnet#238 is resolved.

[baywet]

I agree that a mono-repo would make things simpler here. However, I don't want you to be blocked because of that change, we don't have timelines for that change yet even though it's a high-priority item for us.
So it's up to you to whether you want to wait for the mono-repo migration or not.
Without the mono repo, I think the plan would be to:

1. Open the PR in abstractions with the new helper.
2. Update this PR to take a dependency on this change (you can link projects locally), we know the CI will fail, but at least it'll give us an idea of what things look like.
3. Open the similar PR in JSON serialization, same rationale.

How does that work for you?

[MartinM85]

I agree that a mono-repo would make things simpler here. However, I don't want you to be blocked because of that change, we don't have timelines for that change yet even though it's a high-priority item for us. So it's up to you to whether you want to wait for the mono-repo migration or not. Without the mono repo, I think the plan would be to:

1. Open the PR in abstractions with the new helper.
2. Update this PR to take a dependency on this change (you can link projects locally), we know the CI will fail, but at least it'll give us an idea of what things look like.
3. Open the similar PR in JSON serialization, same rationale.

How does that work for you?

I will try to update repos one by one.

[andrueastman]

Thanks for the contribution @MartinM85.

We'll close this one for now as we have moved the sources.
Please re-open the PR at the mono-repository at https://github.com/microsoft/kiota-dotnet