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

Use ISerializerService in Dependency Injection Setup #221

Closed
MoritzDa opened this issue May 8, 2023 · 5 comments · Fixed by #224
Closed

Use ISerializerService in Dependency Injection Setup #221

MoritzDa opened this issue May 8, 2023 · 5 comments · Fixed by #224
Labels
enhancement New feature or request

Comments

@MoritzDa
Copy link

MoritzDa commented May 8, 2023

Is your feature request related to a problem? Please describe.
I want to be able to use the interface ISerializerService in constructor injection instead of SerializerService, when setting up the SerializerService with Dependency Injection via services.UseCommercetoolsApiSerialization();. Then it would also be better for testing and mocking.

Describe the solution you'd like
I would like to change the service registration in the DI setup like this:

- services.AddSingleton<SerializerService>();
+ services.AddSingleton<ISerializerService, SerializerService>();

Describe alternatives you've considered
Right now I have the current workaround in place to be able to use the interface:

service.UseCommercetoolsApiSerialization();
service.AddSingleton<ISerializerService, SerializerService>();

Additional context
Acutally the expected behaviour was introduced within #23 in commit (6c04123) but, also removed in the same PR with commit (0ded78e)

The DI should be the same for: commercetools.Sdk.Api/DependencyInjectionSetup.cs, commercetools.Sdk.HistoryApi/DependencyInjectionSetup.cs, commercetools.Sdk.ImportApi/DependencyInjectionSetup.cs and commercetools.Sdk.MLApi/DependencyInjectionSetup.cs

@MoritzDa MoritzDa added the enhancement New feature or request label May 8, 2023
@jenschude
Copy link
Contributor

We would more likely have to create a marker interface for each of the SerializerService implementations, as they hold specific configurations for the supported API.

But overall yes we are open for supporting a interface instead of a specific implementation

@MoritzDa
Copy link
Author

MoritzDa commented May 9, 2023

Okay I must say that I don't fully understand the purpose of a marker interface and how and where it is used. But I trust, that you guys know the potential benefits of using it.
So do you think my sugestet idea is any good? Or do you think it should be implemented differently, with the mentioned marker interface? Or not implemented at all?

@jenschude
Copy link
Contributor

jenschude commented May 10, 2023

Just opened a PR which adds the API specific marker interfaces. For backwards compatibility the registration of the SerializerService classes are still there, but internally it will be only retrieved by it's interface.

So it should be possible to mock or replace them e.g. in tests.

We need the marker interfaces as the serialization configuration can be a little bit different and even conflicting between the APIs.

@MoritzDa
Copy link
Author

Hey, perfect! Thank you for the fast implementation. Really appreciate it.

I now understand better what you meant by the marker interfaces. After seeing your changes and implementation.

Just for my understanding:
I now can just use in the DI setup: (when the new version is released)

...
services.UseCommercetoolsApiSerialization();
...

And the use the marker interface in the constructor for example like this:

    private readonly IApiSerializerService _serializerService;

    public ExampleService(IApiSerializerService serializerService)
    {
        _serializerService = serializerService;
    }

Thank you a lot again :)

@jenschude
Copy link
Contributor

Yes. That's the idea behind it. And for testing purposes you could inject whatever implementation you mark with the interface ;)

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

Successfully merging a pull request may close this issue.

2 participants