Introduce IProductService as a consistently-named replacement for IItemService

[j-mok]

The CRUD/search service pairs in catalog follow an intuitive pattern:

• ICatalogService/ICatalogSearchService
• ICategoryService/ICategorySearchService
• IVideoService/IVideoSearchService
• etc...

And then there's this:

• IItemService/IProductSearchService

I know this may be due to historical reasons, but it's very confusing, especially for newcomers to VC.

This PR introduces IProductService to replace IItemService for new client code to use (implementations are renamed too). At the same time IItemService and ItemService are still available and registered in DI to provide backward compatibility, but they're marked obsolete and could be removed in the future.

The JavaScript and entity layers remain unchanged and keep using the term item.

[vc-ci]

Reveiw task created: https://virtocommerce.atlassian.net/browse/VCST-1195

[artem-dudarev]

These changes will break custom modules that override services with IItemService argument in the constructor. ProductDocumentBuilder for example. If custom module overrides the IItemService itself, it will also break, because IItemService is not being used anymore.

[j-mok]

Thanks @artem-dudarev for sharing your concerns. Can you please explain in more detail how that update would be backward-incompatible? Maybe I'm missing something, but I intentionally left IItemService and ItemService types within their original namespaces, but now they are marked obsolete to facilitate and signal the phase-out, also made them extend/inherit from IProductService and ProductService respectively and pushed all their members up the hierarchy. You can leave them in place indefinitely if you want and they should be 100% binary-compatible with code that has consumed them before. I hoped the only noticeable change for dependent code would be CS0618 during builds.

[artem-dudarev]

1. Overriden services with IItemService argument in the constructor

public class MyProductDocumentBuilder : ProductDocumentBuilder
{
    public MyProductDocumentBuilder(..., IItemService itemService, ...)
    : base(... itemService, ...)
}

serviceCollection.AddTransient<ProductDocumentBuilder, MyProductDocumentBuilder>();

After updating the catalog module there will be an error: System.MissingMethodException: Method not found: 'Void VirtoCommerce.CatalogModule.Data.Search.Indexing.ProductDocumentBuilder..ctor(VirtoCommerce.Platform.Core.Settings.ISettingsManager, VirtoCommerce.CatalogModule.Core.Search.IPropertySearchService, VirtoCommerce.CatalogModule.Core.Services.IItemService, VirtoCommerce.CatalogModule.Core.Search.IProductSearchService)' because the signature of the constructor has been changed.

2. Overriden IItemService

public class MyItemService : ItemService {...}

serviceCollection.AddTransient<IItemService, MyItemService>();

Since IItemService is not being used anymore, this overriden service will not be used as well. There will be no error, but the functionallity will be broken.

[j-mok]

Thanks, Artem, now I see these problems. The first one is relatively easy to fix - I just brought back (see the latest commit) all the constructors that originally had IItemService in their signature, but now they are just overloads marked as obsolete, encouraging consumers to use the IProductService overloads. This should be enough to keep existing code that extends or constructs those types happy.

The other one is much more tricky, we have to take into account the possibility that either IItemService or IProductService, or both are overridden in DI registration somewhere. It's not that easy to make that work, but I have a couple of ideas, stay tuned.

[OlegoO]

@j-mok thank you for the update, we will check the code, if all is ok, we will merge, if not I will plan to applying the breaking changes on end of September after one of the next stable release because indeed it simplify dev experience.

[j-mok]

I've added another commit that deals with backward compatibility issue with custom IItemService DI registrations. I've identified four scenarios that needed to be handled:

1. There are no custom implementations of either IItemService or IProductService anywhere in the platform instance and the DI container has only standard registration for these services from the Catalog Module. In that case both IItemService and IProductService resolve to ItemService.
2. There's a custom implementation of IItemService registered somewhere by an existing module. Both IItemService and IProductService resolve to that implementation, meaning that backward compatibility is kept and legacy custom IItemService is resolved by both new and old code.
3. There's a custom implementation of IProductService registered somewhere by an existing module (this will be happing more often as IProductService gets adopted widely). Both IItemService and IProductService resolve to that implementation making old modules that are not aware of IProductService, still work with new custom implementations of IProductService. If that implementation does not implement IItemService, it is automatically adapted.
4. Both IItemService and IProductService have custom implementations registered somewhere - in that case either one or the other are resolved, depending on which service was requested. That makes both registrations independent. I've figured out that such a situation is problematic even when there are multiple custom implementations of just one of those services, so I haven't attempted to make all resolutions use the same type - in this scenario it is up to the platform maintainer to make sure there's just one custom registration used.

Let me know if that'll work, I think this should be 100% backward compatible now, but if not, I'll give it another try.

[artem-dudarev]

Did you try to run this code? It doesn't work for me:

System.InvalidOperationException: Unable to activate type 'VirtoCommerce.CatalogModule.Data.Search.ProductSearchService'. The following constructors are ambiguous:
Void .ctor(System.Func`1[VirtoCommerce.CatalogModule.Data.Repositories.ICatalogRepository], VirtoCommerce.Platform.Core.Caching.IPlatformMemoryCache, VirtoCommerce.CatalogModule.Core.Services.IProductService, Microsoft.Extensions.Options.IOptions`1[VirtoCommerce.Platform.Core.GenericCrud.CrudOptions])
Void .ctor(System.Func`1[VirtoCommerce.CatalogModule.Data.Repositories.ICatalogRepository], VirtoCommerce.Platform.Core.Caching.IPlatformMemoryCache, VirtoCommerce.CatalogModule.Core.Services.IItemService, Microsoft.Extensions.Options.IOptions`1[VirtoCommerce.Platform.Core.GenericCrud.CrudOptions])

Also, it doesn't compile because of an error in the ClassCtorTests, but this is a small error with quick fix.

[j-mok]

@artem-dudarev you're right, it turned out .NET DI has some peculiar constructor resolution rules that favor constructors with the most DI-resolvable parameters, and if there is no such single constructor, the DI fails.

Following Microsoft's advice, I've added another constructor overload that accepts both the obsolete and the new product CRUD service parameters, thus making it the single constructor with the most DI-resolvable parameters. This solves the problem at the cost of increased complexity and having weird-looking constructors that discard one parameter, but unfortunately there's no other simple solution, like attribute-based constructor ambiguity resolutions mechanism.

So, to sum it all up, classes that originally had a constructor that accepted IItemService as a parameter, now have 3 constructors:

• accepting only IProductService - the only non-obsolete one, intended for direct (non-DI) instantiation from new client code and for DI in the future,
• accepting only IItemService - the original one, now obsolete and intended for backward-compatible direct instantiation from existing client code,
• accepting both IProductService and IItemService - obsolete, intended for DI container-based instantiation as it has the most DI-resolvable parameters. It discards IItemService argument and calls the first constructor.

I've run the platform locally after introducing the 3rd constructor and it seems to start up without problems. I must admit though a small change in naming proved to be much more nuanced than I anticipated, but that's how things are when we want to keep backward compatibility. On the bright side, after transition period, all that code can be simplified back to the original level.