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

Unexpected exception when using KiotaJsonSerializer #397

Closed
svrooij opened this issue Sep 25, 2024 · 7 comments
Closed

Unexpected exception when using KiotaJsonSerializer #397

svrooij opened this issue Sep 25, 2024 · 7 comments
Labels
enhancement New feature or request Status: No Recent Activity status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:enhancement Enhancement request targeting an existing experience

Comments

@svrooij
Copy link
Contributor

svrooij commented Sep 25, 2024

Sample code:

var app = new Microsoft.Graph.Beta.Models.WinGetApp
{
    DisplayName = package?.Name,
    Description = package?.Description,
    ...
};
string json = await KiotaJsonSerializer.SerializeAsStringAsync(app, false, cancellationToken);

Results in:

Exception: System.InvalidOperationException: Content type application/json does not have a factory registered to be parsed
  at Microsoft.Kiota.Abstractions.Serialization.SerializationWriterFactoryRegistry.GetSerializationWriterFactory(String contentType, String& actualContentType)
  at Microsoft.Kiota.Abstractions.Serialization.SerializationWriterFactoryRegistry.GetSerializationWriter(String contentType, Boolean serializeOnlyChangedValues)
  at Microsoft.Kiota.Abstractions.Serialization.KiotaSerializer.GetSerializationWriter(String contentType, Object value, Boolean serializeOnlyChangedValues)
  at Microsoft.Kiota.Abstractions.Serialization.KiotaSerializer.SerializeAsStream[T](String contentType, T value, Boolean serializeOnlyChangedValues)
  at Microsoft.Kiota.Abstractions.Serialization.KiotaSerializer.SerializeAsStringAsync[T](String contentType, T value, Boolean serializeOnlyChangedValues, CancellationToken cancellationToken)
  at Microsoft.Kiota.Abstractions.Serialization.KiotaJsonSerializer.SerializeAsStringAsync[T](T value, Boolean serializeOnlyChangedValues, CancellationToken cancellationToken)

This makes me wonder where and how the factories are registered, and if this is even needed for the extension methods.

@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Sep 25, 2024
@svrooij
Copy link
Contributor Author

svrooij commented Sep 25, 2024

Maybe the new extensions should have used something like:

    internal static async Task<string> SerializeAsJsonString(this IParsable value, CancellationToken cancellationToken)
    {
        using var writer = new JsonSerializationWriter();
        writer.WriteObjectValue(null, value);
        using var stream = writer.GetSerializedContent();
        using var reader = new StreamReader(stream);
        return await reader.ReadToEndAsync(cancellationToken);
    }

@baywet
Copy link
Member

baywet commented Sep 25, 2024

This is currently being registered here.
https://github.com/microsoftgraph/msgraph-sdk-dotnet/blob/817308446f538133e6a7460166cdd8ef0334e964/src/Microsoft.Graph/Generated/BaseGraphServiceClient.cs#L469

And we're in the process of moving it there so the generated code is truely portable.

ApiClientBuilder.RegisterDefaultSerializer<JsonSerializationWriterFactory>();

Which mean that if you use the Json methods (static or extension) before the client, or in the future the request adapter, has been newed up, you'll run into this error.

Now, while the extension methods can register the serialization provider if it's missing because of where they are defined, the static methods can't.
And there's really no reason why one should work but not the other.
We could imagine moving the static KiotaJsonSerializer methods to the Json package. And implement a fallback rather than failing whenever the serialization provider is not present

Thoughts?

@baywet baywet added enhancement New feature or request status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:enhancement Enhancement request targeting an existing experience labels Sep 25, 2024
@svrooij
Copy link
Contributor Author

svrooij commented Sep 25, 2024

What does this means if you would use 2 kiota clients in one application GitHub and Graph for instance? Do we get double registrations? Or will they be overridden? This might lead to issues if you setup custom factories?

I think the the KiotaJsonSerializer and the extension methods need to work indepently of the client. Maybe the registered factories should be set in the client and not in a singleton.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Sep 25, 2024
@baywet
Copy link
Member

baywet commented Sep 26, 2024

Those are the default serialization providers. The app developer can always pass a different set to the request adapter (at least with our implementations) which will be local to this request adapter.
In case the automatic registration is used from the client, first client wins for any given media type.
If we want those methods not to depend on the singleton, they'll need to accept a request adapter.

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Sep 26, 2024
@svrooij
Copy link
Contributor Author

svrooij commented Oct 1, 2024

Would you be ok with me detaching the Iparsable extensions from the registry? To make sure they will work not matter the registry is filled?

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close Status: No Recent Activity labels Oct 1, 2024
@baywet
Copy link
Member

baywet commented Oct 1, 2024

For the Json extension methods defined in the Json package, yes. People won't have access to the method if they have not imported the dependency. So I think it's safe to assume they'll want to use this implementation.
For the format agnostic extension methods, I'm not sure we can do that without adding an additional required parameter.

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Oct 1, 2024

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

@github-project-automation github-project-automation bot moved this from Needs Triage 🔍 to Done ✔️ in Kiota Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Status: No Recent Activity status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:enhancement Enhancement request targeting an existing experience
Projects
Archived in project
Development

No branches or pull requests

2 participants