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

Add function overloads with Type as parameter for KiotaSerializer.Deserialize<T> #210

Closed
dansmitt opened this issue Mar 21, 2024 · 5 comments · Fixed by #436
Closed

Add function overloads with Type as parameter for KiotaSerializer.Deserialize<T> #210

dansmitt opened this issue Mar 21, 2024 · 5 comments · Fixed by #436
Assignees
Labels
area:serialization Focused on functional modules of the product enhancement New feature or request help wanted Standard GitHub label WIP

Comments

@dansmitt
Copy link

We (@MichaMican and I) have implemented an TextFormatter extension to use Kiota as thge standard serializer for IParsable objects when working with ASP.NET Controllers.

See: microsoftgraph/msgraph-sdk-dotnet#2343 (comment)

In our TextInputFormatter we have to fallback to using Reflection because we work with an implementation of IParsable object that we have to set as the generic type of KiotaSerializer.Deserialize<T> during run time. As of now the function unfortunately has no overload to specify the Type as a parameter which would be super usefull in our case as we could then imrpove code readability by not using reflection.

Snippet from the TextInputFormatter:

public override Task<InputFormatterResult> ReadRequestBodyAsync(InputFormatterContext context, Encoding encoding)
{
    var httpContext = context.HttpContext;

    if (httpContext.Request.Body.CanRead)
    {
        var targetType = context.ModelType; // <- This is where we get the Type for Deserialize per runtime

        #region Reflection to define Generic type T of KiotaJsonSerializer.Deserialize<T> dynamically
        var methodInfo = typeof(KiotaJsonSerializer).GetMethod("Deserialize", BindingFlags.Public | BindingFlags.Static, [typeof(Stream)]);
        if (methodInfo == null)
        {
            throw new Exception("KiotaJsonSerializer.Deserialize changed. Please update implementation");
        }

        // This line types the KiotaJsonSerializer.Deserialize<T> to T equals to targetType
        var methodWithForcedTargetType = methodInfo.MakeGenericMethod(targetType);

        // obj is null because KiotaJsonSerializer.Deserialize is a static function
        // This function is analog to KiotaJsonSerializer.Deserialize<T>(httpContext.Request.Body) -> Where T = targetType
        var deserializedObject = methodWithForcedTargetType.Invoke(null, [httpContext.Request.Body]);

       // As we used reflection deserializedObject is of type object, hence we have to continue to use Reflection to access the BackingStore/InitializationCompleted property.
        var backingStoreProperty = (deserializedObject?.GetType().GetProperty("BackingStore"));
        var backingStoreValue = backingStoreProperty?.GetValue(deserializedObject);

        backingStoreValue?.GetType().GetProperty("InitializationCompleted")?.SetValue(backingStoreValue, false);

        #endregion

        return Task.FromResult(InputFormatterResult.Success(deserializedObject));
    }

    return Task.FromResult(InputFormatterResult.Failure());

}
@baywet
Copy link
Member

baywet commented Mar 21, 2024

Hi @dansmitt
Thanks for using kiota and for reaching out.
I'm not sure I'm following what's needed here?
Can you share and updated version of the snippet you've shared, removing the reflection and demonstrating the use of the missing method please?

@MichaMican
Copy link

MichaMican commented Mar 22, 2024

Hi @baywet
Basically Kiota currently has a static function:

// from Microsoft.Kiota.Abstractions.dll (1.7.5)
public static T? Deserialize<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] T>(string serializedRepresentation) where T : IParsable```

Note that the generic Type T has to be explicitly defined here as it can't be implicitly derived from the methods parameters.
As @dansmitt mentioned we unfortunately don't have a static type but instead we get it during Runtime from the InputFormatterContext.

Hence if there would be a function where the type can be parsed as a parameter:

public static IParsable? Deserialize(string serializedRepresentation, Type targetTypeToSerializeTo)

we would not have to use reflection to set the generic type T of the KiotaJsonSerializer.Deserialize() function.

I'm not sure if there is a more specific type then IParsable - that this function could return - but that would already help a lot.

For reference System.Text.Json has also a function like that in their Serializer reperteure of JsonSerializer.Deserialize():

// from System.Text.Json.dll (8.0.3)
public static object? Deserialize([StringSyntax(StringSyntaxAttribute.Json)] ReadOnlySpan<char> json, Type returnType, JsonSerializerOptions? options = null)

@dansmitt
Copy link
Author

@baywet what @MichaMican says

@baywet
Copy link
Member

baywet commented Mar 22, 2024

Thanks for the detailed explanation here.
This type is ultimately used to get the factory method of the type
https://github.com/microsoft/kiota-abstractions-dotnet/blob/03026f728ac41c3398600d3f7406f7be123a9e46/src/serialization/KiotaSerializer.Deserialization.cs#L72

So assuming a bit of refactoring, it should be easy to implement an overload that accepts the type directly.

Is this something you'd be interested in submitting a pull request for?

@baywet baywet added enhancement New feature or request Standard GitHub label Issue caused by core project dependency modules or library and removed question Needs: Attention 👋 labels Mar 22, 2024
@andrueastman andrueastman added area:serialization Focused on functional modules of the product help wanted Standard GitHub label and removed Standard GitHub label Issue caused by core project dependency modules or library labels Jul 9, 2024
@DerGuru
Copy link

DerGuru commented Oct 11, 2024

So assuming a bit of refactoring, it should be easy to implement an overload that accepts the type directly.
Is this something you'd be interested in submitting a pull request for?

Just did that.

@baywet baywet linked a pull request Oct 11, 2024 that will close this issue
@github-project-automation github-project-automation bot moved this from Todo 📃 to Done ✔️ in Kiota Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:serialization Focused on functional modules of the product enhancement New feature or request help wanted Standard GitHub label WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants