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

Inject service into JsonConverter #34773

Closed
thomaslevesque opened this issue Apr 9, 2020 · 11 comments
Closed

Inject service into JsonConverter #34773

thomaslevesque opened this issue Apr 9, 2020 · 11 comments
Labels
area-System.Text.Json question Answer questions and provide assistance, not an issue with source code or documentation.
Milestone

Comments

@thomaslevesque
Copy link
Member

thomaslevesque commented Apr 9, 2020

I have a converter that needs to access a service from the DI container. How can I do that?

I know I could create the converter in a IConfigureOptions<JsonOptions> and globally add it to the JsonSerializerOptions, but then it would apply everywhere, which I don't want. I want to apply it with an attribute.

Is there a way to inject a service into a converter?

I guess not, especially since the JsonSerializer API is static, but I can think of one way to achieve the desired result with very little change: add an IServiceProvider property to JsonSerializerOptions. This way, converter can retrieve the services they need.

If you don't want to depend on Microsoft.Extensions.DependencyInjection, expose that property as object, and the user can do whatever they want with it.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Apr 9, 2020
@thomaslevesque
Copy link
Member Author

Or, even better: add a property like this to JsonSerializerOptions:

public Func<Type, object> ServiceProvider { get; set; }

And use it when constructing instances of converter specified with JsonConverterAttribute.

@pranavkm
Copy link
Contributor

pranavkm commented Apr 9, 2020

If the intent is to configure it on a per-callsite basis, couldn't the application code resolve the converter from DI?

@thomaslevesque
Copy link
Member Author

@pranavkm no, it's not per-callsite. For instance if have a type like this:

public class Foo
{
    [JsonConverter(typeof(MyCustomConverter))]
    public int Id { get; set; }
    public string Name { get; set; }
    public int LuckyNumber { get; set; }
}

I can't just add MyCustomConverter to JsonSerializerOptions.Converters, because it would apply to all int properties, which I don't want. So I have to use the attribute, but if I do that I have no way to inject a service into the converter.

@thomaslevesque
Copy link
Member Author

thomaslevesque commented Apr 10, 2020

Current workaround :

        private class ConfigureJsonOptions : IConfigureOptions<JsonOptions>
        {
            private readonly IServiceProvider _services;

            public ConfigureJsonOptions(IServiceProvider services)
            {
                _services = services;
            }

            public void Configure(JsonOptions options)
            {
                options.JsonSerializerOptions.Converters.Add(new ServiceProviderDummyConverter(_services));
            }
        }

    /// <summary>
    /// This isn't a real converter. It only exists as a hack to expose
    /// IServiceProvider on the JsonSerializerOptions.
    /// </summary>
    public class ServiceProviderDummyConverter : JsonConverter<ServiceProviderDummyConverter>, IServiceProvider
    {
        private readonly IServiceProvider _services;

        public ServiceProviderDummyConverter(IServiceProvider services)
        {
            _services = services;
        }

        public object GetService(Type serviceType) => _services.GetService(serviceType);

        public override bool CanConvert(Type typeToConvert) => false;

        public override ServiceProviderDummyConverter Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        {
            throw new NotImplementedException();
        }

        public override void Write(Utf8JsonWriter writer, ServiceProviderDummyConverter value, JsonSerializerOptions options)
        {
            throw new NotImplementedException();
        }
    }

In a real converter, you have access to the options, so you can retrieve the service provider via the converter above:

var serviceProvider = options.Converters.OfType<IServiceProvider>().First();

@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Apr 14, 2020
@layomia layomia added this to the Future milestone Apr 14, 2020
@eiriktsarpalis
Copy link
Member

What is the motivating use case for requiring DI in a JsonConverter? My understanding of JsonConverter is that it's meant to define a pure mapping between JSON and .NET types and vice versa, so a DI requirement in this context sounds like introducing unrelated concerns into the serialization layer.

In any case, it might be possible to achieve what you're looking for using a ConditionalWeakTable on JsonSerializerOptions:

// dynamically attach IServiceProvider instances to JsonSerializerOptions
public static class JsonSerializerOptionsDependencyInjection
{
    private readonly static ConditionalWeakTable<JsonSerializerOptions, IServiceProvider> s_serviceproviders = new();

    public static IServiceProvider? GetServiceProvider(this JsonSerializerOptions options)
        => s_serviceproviders.TryGetValue(options, out var value) ? value : null;

    public static void SetServiceProvider(this JsonSerializerOptions options, IServiceProvider provider)
        => s_serviceproviders.Add(options, provider);
}

public class FooConverter : JsonConverter<int>
{
    public override int Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        IServiceProvider? provider = options.GetServiceProvider();
        ...
    }

    public override void Write(Utf8JsonWriter writer, int value, JsonSerializerOptions options)
        => throw new NotImplementedException();
}

@eiriktsarpalis eiriktsarpalis added the question Answer questions and provide assistance, not an issue with source code or documentation. label Oct 20, 2021
@thomaslevesque
Copy link
Member Author

My understanding of JsonConverter is that it's meant to define a pure mapping between JSON and .NET types and vice versa, so a DI requirement in this context sounds like introducing unrelated concerns into the serialization layer.

@eiriktsarpalis in theory, I agree with you. Unfortunately the real world disagrees, as is often the case...

My initial use case for this was an API that needed to expose obfuscated ids to prevent resource enumeration. The idea was that the application would manipulate "normal" int ids, and the obfuscation would occur at the application's boundaries (i.e. JSON serialization, ASP.NET Core model binding, etc). The obfuscation process uses cryptography, so it needs a key. And that key comes from the application settings, which I can only access through IConfiguration or IOptions<T>. Hence the need to inject services into my JSON converter. (I'm pretty sure I had another use case that came up in another project, but I can't remember what it was.)

Of course, there are workarounds, like the one I posted above, or the one you suggested using ConditionalWeakTable. For the use case I just mentioned, I could have just stored the encryption key in a static somewhere during application startup, but it felt a bit dirty.

So, it's not a major issue, definitely not a show-stopper, but it'd be nice to have a cleaner way to achieve this. Maybe just a general purpose Items property on JsonSerializerOptions that could hold whatever the user wants, like the one found in HttpContext and a few other places:

public IDictionary<string, object> Items { get; }

@eiriktsarpalis
Copy link
Member

For .NET 7 we are looking at extending the converter model (parent issue #36785), and this would likely include a concept similar to StreamingContext. Note however that this would be scoped to individual serialization operations, instead of being associated with the JsonSerializerOptions instance. Would that serve to solve your use case?

@thomaslevesque
Copy link
Member Author

@eiriktsarpalis could you elaborate please? What would it look like? I don't see what you mention in issue #36785

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Oct 21, 2021

We're still in very early phases of planning (in fact we're still grooming our backlog as you can see :-)), so final design is TBD. But fundamentally we'd be looking at exposing new virtual Read&Write methods in JsonConverter<T> that accept a "State" parameter which can also accept user-defined state scoped to the particular serialization operation.

@thomaslevesque
Copy link
Member Author

thomaslevesque commented Oct 21, 2021

@eiriktsarpalis I see, thanks.
I'm not sure it would work for my use case, though; in my scenario, I'm not calling JsonSerializer myself, it's called by ASP.NET Core, so I can't pass state to the serializer. I can only control the JsonSerializerOptions.

@eiriktsarpalis
Copy link
Member

That's good feedback, thanks. We might want to consider ways to tie in the feature to the ASP.NET DI context, but that might require input from that team as well. cc @pranavkm

@ghost ghost locked as resolved and limited conversation to collaborators Nov 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json question Answer questions and provide assistance, not an issue with source code or documentation.
Projects
None yet
Development

No branches or pull requests

5 participants