Saving entity containing complex jsonb column with validation and comparision

Question

I've been a .NET Developer for almost 4 years. I'm stronly learning and trying using design patterns and following SOLID principles for 3 months right now (I know - a little late, but better late than never).

I have complex business logic for saving some metadata in database (mainly jsonb object to postgresql database). My code is working right now, but I'm not so happy about my code. I think it is sometimes not clean as it should, and not always "adaptable". And also I think that I break Single Principle Responsibility even if I really try to stick to it. I mean that class should not have too much responsibility, but the last class which combines everything should have multiple injections and just delegate some steps by using interfaces. I think that is correct thinking, but when I see that I inject about 6 interfaces, I have a feeling that this code is not so good.

So I want to get feedback, what I am doing wrong in this code, and what can be written better. Also I think I am still not very well adapting desing patterns with Microsoft Dependency Injection. Hard for me is the factory pattern with generic types and how to properly inject it.

So here is a code for saving "Deliveryset". Note: I use "some" word in some names to hide real business names. This code below is saving data to postgresql database using ef core. Before it is saved it must be validated and also checked in a database that has an existing related entity ("Content") for it. After that I search for the current Deliveryset with the highest version, in order to compare it with the incoming DeliverySet. I compare big jsonb objects using "CompareNETObjects" nuget library. The comparison is needed to know if subtitles, videos, and other metadata changed. If any changes occurred it will create a new row in the ContentDeliverySet table with Version = currentVersion + 1. But it also checks related entities like SubtitleFiles or VideoFiles if they already exist. If it exists and doesn't have changes it should copy it but with a new entity Id. If for example video changed it will not copy and create ContentDeliveryset entity with empty VideoFilesEntity.

The code:

Interface:

public interface IDeliverysetSaverService
{
    Task<bool> SaveDeliveryset(Deliveryset? deliverySet, bool isDeletedFlag = false, CancellationToken cancellationToken = default);
}

Implementation:

using DeliverysetService.Library.Models.ValueObjects;
using DeliverysetService.Library.Persistence;
using DeliverysetService.Library.Persistence.Models;
using DeliverysetService.Library.Persistence.Models.Json;
using DeliverysetService.Library.Services;
using DeliverysetService.Library.Services.ChangesCoordinator;
using DeliverysetService.Library.Services.DataServices;
using FluentValidation;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;

public class DeliverysetSaverService : IDeliverysetSaverService
    {
        private readonly IApplicationDbContext _context;
        private readonly IDeliverysetVersionInfoByIdRetrieve _deliverysetVersionInfoByIdRetrieve;
        private readonly IAssetStatusMapRetriever _assetStatusMapRetriever;
        private readonly ILogger<DeliverysetSaverService> _logger;
        private readonly IDeliverysetService _deliverySetService;
        private readonly IValidator<Deliveryset> _deliverySetValidator;
        private readonly IDeliverysetFileChangesCoordinator _deliverySetFileChangesCoordinator;
    
        public DeliverysetSaverService(IApplicationDbContext context, IDeliverysetVersionInfoByIdRetrieve deliverysetVersionInfoByIdRetrieve, IAssetStatusMapRetriever assetStatusMapRetriever, ILogger<DeliverysetSaverService> logger, IDeliverysetService deliverySetService, IValidator<Deliveryset> deliverySetValidator, IDeliverysetFileChangesCoordinator deliverySetFileChangesCoordinator)
        {
            _context = context;
            _deliverysetVersionInfoByIdRetrieve = deliverysetVersionInfoByIdRetrieve;
            _assetStatusMapRetriever = assetStatusMapRetriever;
            _logger = logger;
            _deliverySetService = deliverySetService;
            _deliverySetValidator = deliverySetValidator;
            _deliverySetFileChangesCoordinator = deliverySetFileChangesCoordinator;
        }
    
        public async Task<bool> SaveDeliveryset(Deliveryset? deliverySet, bool isDeletedFlag = false,
            CancellationToken cancellationToken = default)
        {
            if (deliverySet == null) return false;
            var validationResult = await _deliverySetValidator.ValidateAsync(deliverySet, cancellationToken);
            if (!validationResult.IsValid)
            {
                _logger.LogWarning("Deliveryset validation failed, reason: {reason}", validationResult.ToString());
                return false;
            }
    
    
            var id = new SomeIdValueObject(deliverySet.Tags.MhtId);
    
            var contentId = (await _context.Contents.SingleOrDefaultAsync(content =>
                content.ProductionNr == id.ProductionNr && content.ProgramNr == id.ProgramNr
                                                        && content.Version == id.Version && content.Format == id.Format, cancellationToken: cancellationToken))?.Id;
    
            var (someTrafficId, _) = id;
    
            if (!contentId.HasValue)
            {
                _logger.LogWarning(
                    "Content for id:{id} was not found when saving delivery set", someTrafficId);
                return false;
            }
    
            var deliverySetVersion =
                await _deliverysetVersionInfoByIdRetrieve.GetVersionInfo(contentId.Value, cancellationToken);
    
            var deliverySetChangesInfo = await _deliverySetFileChangesCoordinator.GetDeliverysetChangesInfo(deliverySetVersion, deliverySet, cancellationToken);
            if (!deliverySetChangesInfo.HasChanges)
            {
                return false;
            }
    
            var contentDeliveryset = new ContentDeliveryset
            {
                ContentDeliverysetMetadata = new ContentDeliverysetMetadata()
                {
                    AssetStatusId = _assetStatusMapRetriever.GetAssetStatusMap()["some_code"].Id
                },
                Deliveryset = deliverySet,
                ContentId = contentId,
                Version = deliverySetVersion.NextVersion,
                DeliverysetId = deliverySet.Id,
                SubtitleFiles = deliverySetChangesInfo.SubtitleFiles.ToList(),
                VideoFiles = deliverySetChangesInfo.VideoFiles.ToList(),
                IsDeleted = isDeletedFlag
            };
    
            await _deliverySetService.Save(contentDeliveryset, cancellationToken);
            return true;
        }
    }

IDeliverysetVersionInfoByIdRetrieve interface is used for searching the current delivery set with it's version.

IAssetStatusMapRetriever interface is used to get whole AssetStatus entity and transform to dictionary: <Code, Entity>.

IDeliverysetService interface contains only context and saves passed object to database.

'IDeliverysetFileChangesCoordinator' interface is the most complicated one.

Interface:

public interface IDeliverysetFileChangesCoordinator
{
    Task<DeliverysetChangesInfo> GetDeliverysetChangesInfo(DeliverysetVersionInfo deliverySetVersionInfo,
        Deliveryset newDeliveryset,
        CancellationToken cancellationToken = default); GetFilesToCopy();
}

Implementation:

using DeliverysetService.Library.Enums;
using DeliverysetService.Library.Models;
using DeliverysetService.Library.Persistence;
using DeliverysetService.Library.Persistence.Models;
using DeliverysetService.Library.Persistence.Models.Json;
using DeliverysetService.Library.Services.DeliverysetFileCopyHandlerStrategy;
using DeliverysetService.Library.Services.DeliverysetFileDetectChangesHandlerStrategy;
using Microsoft.EntityFrameworkCore;

public class DeliverysetFileChangesCoordinator : IDeliverysetFileChangesCoordinator
    {
        private readonly IApplicationDbContext _context;
        private readonly IDeliverysetFileCopyHandlerFactory _deliverySetFileCopyHandlerFactory;
        private readonly IDeliverysetFileDetectChangesHandlerFactory _detectChangesHandlerFactory;
        private IEnumerable<VideoFiles> _videoFiles = Array.Empty<VideoFiles>();
        private IEnumerable<SubtitleFiles> _subtitleFiles = Array.Empty<SubtitleFiles>();
        private bool _anyChanges;
    
        public DeliverysetFileChangesCoordinator(IApplicationDbContext context, IDeliverysetFileCopyHandlerFactory deliverySetFileCopyHandlerFactory, IDeliverysetFileDetectChangesHandlerFactory detectChangesHandlerFactory)
        {
            _context = context;
            _deliverySetFileCopyHandlerFactory = deliverySetFileCopyHandlerFactory;
            _detectChangesHandlerFactory = detectChangesHandlerFactory;
        }
        public async Task<DeliverysetChangesInfo> GetDeliverysetChangesInfo(DeliverysetVersionInfo deliverySetVersionInfo, Deliveryset newDeliveryset, CancellationToken cancellationToken = default)
        {
            if (deliverySetVersionInfo.CurrentVersion == null)
            {
                return new DeliverysetChangesInfo(true, Array.Empty<VideoFiles>(), Array.Empty<SubtitleFiles>());
            }
            var currentNewestDeliveryset =
                await _context.ContentDeliveryset.AsNoTracking().Where(d => d.Id == deliverySetVersionInfo.CurrentVersion.Id)
                    .Select(t => new
                    {
                        t.Deliveryset,
                        t.VideoFiles,
                        t.SubtitleFiles,
                        t.Version
                    }).SingleAsync(cancellationToken: cancellationToken);
    
    
    
    
            foreach (var deliverySetChange in Enum.GetValues<DeliverysetChange>())
            {
                var changesHandler = _detectChangesHandlerFactory.GetDeliverysetFileCopyHandler(deliverySetChange);
                var changesResult = changesHandler.GetDeliverysetChangesResult(currentNewestDeliveryset.Deliveryset, newDeliveryset);
    
                if (!changesResult.IsAllAreEqual)
                {
                    _anyChanges = true;
                }
                if (changesResult.IsAllAreEqual && deliverySetChange == DeliverysetChange.Video)
                {
                    var copyHandler = _deliverySetFileCopyHandlerFactory.GetDeliverysetFileCopyHandler<VideoFiles>(DeliverysetFileCopyHandler.Video);
                    _videoFiles = copyHandler.GetFilesToCopy(currentNewestDeliveryset.VideoFiles);
                }
    
                if (deliverySetChange == DeliverysetChange.Subtitle)
                    _subtitleFiles = GetSubtitlesToCopy(changesResult, currentNewestDeliveryset.SubtitleFiles);
            }
    
            return new DeliverysetChangesInfo(_anyChanges, _videoFiles, _subtitleFiles);
        }
    
    
    
        private IEnumerable<SubtitleFiles> GetSubtitlesToCopy(DeliverysetChangesResult changesResult, List<SubtitleFiles> currentDeliverysetSubFiles)
        {
            var copy = _deliverySetFileCopyHandlerFactory.GetDeliverysetFileCopyHandler<SubtitleFiles>(DeliverysetFileCopyHandler.Sub);
    
            bool FilterNotChangedSubtitleTypes(SubtitleFiles t) => changesResult.NotChangedByKeyList.Contains(t.SubtitleType);
    
            var subtitleFilesList = changesResult.IsAllAreEqual
                ? currentDeliverysetSubFiles
                : currentDeliverysetSubFiles.Where(FilterNotChangedSubtitleTypes).ToList();
    
            var subtitleFilesEnumerable = copy.GetFilesToCopy(subtitleFilesList);
            return subtitleFilesEnumerable;
        }
    }

Implementation of DeliverysetFileDetectChangesHandlerFactory

using DeliverysetService.Library.Enums;

    public class DeliverysetFileDetectChangesHandlerFactory : IDeliverysetFileDetectChangesHandlerFactory
    {
        private readonly IDictionary<DeliverysetChange, IDeliverysetFileDetectChangesHandler> _map;
    
        public DeliverysetFileDetectChangesHandlerFactory(IDictionary<DeliverysetChange, IDeliverysetFileDetectChangesHandler> map)
        {
            _map = map;
        }
    
        public IDeliverysetFileDetectChangesHandler GetDeliverysetFileCopyHandler(DeliverysetChange deliverySetChange)
        {
            return _map[deliverySetChange];
        }
    }

Interface IDeliverysetFileDetectChangesHandler:

using DeliverysetService.Library.Models;
using DeliverysetService.Library.Persistence.Models.Json;

    public interface IDeliverysetFileDetectChangesHandler
    {
        DeliverysetChangesResult GetDeliverysetChangesResult(Deliveryset currentDeliveryset, Deliveryset newDeliveryset);
    }

One of implementations:

using DeliverysetService.Library.Models;
using DeliverysetService.Library.Persistence.Models.Json;
using DeliverysetService.Library.Services.Comparers;

    public class DeliverysetAssetsDetectChangesHandler : IDeliverysetFileDetectChangesHandler
    {
        public DeliverysetChangesResult GetDeliverysetChangesResult(Deliveryset currentDeliveryset, Deliveryset newDeliveryset)
        {
            return DeliverysetComparer.Init(newDeliveryset, currentDeliveryset).AssetsChanges();
        }
    }

Implementation of DeliverysetFileCopyHandlerFactory

using DeliverysetService.Library.Enums;
using DeliverysetService.Library.Persistence.Models.Common.Interfaces;

    public class DeliverysetFileCopyHandlerFactory : IDeliverysetFileCopyHandlerFactory
    {
        private readonly IDictionary<DeliverysetFileCopyHandler, IDeliverysetFileCopyHandler> _map;
    
        public DeliverysetFileCopyHandlerFactory(IDictionary<DeliverysetFileCopyHandler, IDeliverysetFileCopyHandler> map)
        {
            _map = map;
        }
    
        public IDeliverysetFileCopyHandler<T> GetDeliverysetFileCopyHandler<T>(DeliverysetFileCopyHandler copyMethod) where T : IFileEntity
        {
            var deliverySetFileCopyHandler = _map[copyMethod];
            return (IDeliverysetFileCopyHandler<T>)deliverySetFileCopyHandler;
        }
    }

Interface IDeliverysetFileCopyHandler:

using DeliverysetService.Library.Persistence.Models.Common.Interfaces;

    public interface IDeliverysetFileCopyHandler
    {
    }
    public interface IDeliverysetFileCopyHandler<T> : IDeliverysetFileCopyHandler where T : IFileEntity
    {
        IEnumerable<T> GetFilesToCopy(List<T> entities);
    }

One of implementations:

using DeliverysetService.Library.Persistence.Models;

    public class DeliverysetSubtitleFilesCopyHandler : IDeliverysetFileCopyHandler<SubtitleFiles>
    {
        public IEnumerable<SubtitleFiles> GetFilesToCopy(List<SubtitleFiles>? entities)
        {
            if (entities == null) yield break;
    
            foreach (var entity in entities)
            {
                yield return new SubtitleFiles()
                {
                    FileId = entity.FileId,
                    Status = entity.Status,
                    SubtitleType = entity.SubtitleType
                };
            }
        }
    }

And last is dependency injection:

using DeliverysetService.Library.Enums;
using DeliverysetService.Library.Persistence.Models.Json;
using DeliverysetService.Library.Validators;
using FluentValidation;
using Microsoft.Extensions.DependencyInjection;
using System.Reflection;
using DeliverysetService.Library.Persistence;
using DeliverysetService.Library.Services.DataServices;
using Microsoft.EntityFrameworkCore;
using DeliverysetService.Library.Services.DeliverysetFileCopyHandlerStrategy;
using DeliverysetService.Library.Services.DeliverysetFileDetectChangesHandlerStrategy;
using DeliverysetService.Library.Services.ChangesCoordinator;
using DeliverysetService.Library.Services;

     public static class ConfigureServices
        {
            public static IServiceCollection AddLibraryServices(this IServiceCollection services)
            {
                var dummyConnString = "Server=127.0.0.1;Port=5432;Database=myDataBase;User Id=myUsername;Password=myPassword";
    
                services.AddDbContext<ApplicationDbContext>(options =>
                    options.UseNpgsql(dummyConnString));
                services.AddValidatorsFromAssembly(Assembly.GetExecutingAssembly());
                services.AddScoped<IApplicationDbContext>(provider => provider.GetRequiredService<ApplicationDbContext>());
                services.AddScoped<AuditableEntitySaveChangesInterceptor>();
    
                services.AddDeliverysetServices();
    
    
                return services;
            }
            public static IServiceCollection AddDeliverysetServices(this IServiceCollection services)
            {
                services.AddScoped<IValidator<Deliveryset>, DeliverysetValidator>();
                services.AddScoped<IDeliverysetSaverService, DeliverysetSaverService>();
                services.AddScoped<IAssetStatusMapRetriever, AssetStatusMapRetriever>();
                services.AddScoped<IDeliverysetVersionInfoByIdRetrieve, DeliverysetVersionInfoByIdRetrieve>();
                services.AddScoped<IDeliverysetService, Services.DataServices.DeliverysetService>();
                services.AddScoped<IDeliverysetFileChangesCoordinator, DeliverysetFileChangesCoordinator>();
                services.AddSingleton<IDeliverysetFileCopyHandlerFactory, DeliverysetFileCopyHandlerFactory>(_ =>
                {
                    var deliverySetFileCopyHandlers =
                        new Dictionary<DeliverysetFileCopyHandler, IDeliverysetFileCopyHandler>
                        {
                            { DeliverysetFileCopyHandler.Video, new DeliverysetVideoFilesCopyHandler() },
                            { DeliverysetFileCopyHandler.Sub, new DeliverysetSubtitleFilesCopyHandler() }
                        };
                    return new DeliverysetFileCopyHandlerFactory(deliverySetFileCopyHandlers);
                });
                services.AddSingleton<IDeliverysetFileDetectChangesHandlerFactory, DeliverysetFileDetectChangesHandlerFactory>(_ =>
                {
                    var deliverySetFileDetectChangesHandlers =
                        new Dictionary<DeliverysetChange, IDeliverysetFileDetectChangesHandler>
                        {
                            { DeliverysetChange.Video, new DeliverysetVideoFilesDetectChangesHandler() },
                            { DeliverysetChange.Asset, new DeliverysetAssetsDetectChangesHandler() },
                            { DeliverysetChange.Subtitle, new DeliverysetSubtitleFilesDetectChangesHandler() }
                        };
                    return new DeliverysetFileDetectChangesHandlerFactory(deliverySetFileDetectChangesHandlers);
                });
                return services;
            }
        }
    }

I put the whole fully compiling code in github. Don't watch repo structured it was copied from bigger solution for code review purposes.

link to repo

I know that this is a lot of classes and interfaces. But if it was putted in single method in single class it won't be so big. This also my concern if I am not split to much the code.

Ps. I'm not asking for an in-depth review of the code and pointing out fixes in each class. Small tips on what to pay attention to and what I can improve in total approach will also very helpful.

EDIT: Answer for: BCdotWEB

 - About the using statements, my bad that I copied it from the temporary repo for code review. In my real solution I have more dlls and they have other namespaces  - About splitting by "type". If I understood correctly, you mean entities, enums, events. I've taken it from this repo CleanArchitecture, but I probably misunderstood something.  - "SaveDeliveryset" name for method is redundant I agree, it was previous in bigger interface and I forgot refactor it  - For getVersionInfo it is probably very bad naming, but as you see I have a problem with that for the whole project  - About "IsAllAreEqual" same I forgot to refactor it, because previously I looked for all changes not for every change. Thanks for pointing that  - do not call something a "xxxxxxxList". I know that is a bad thing, I will name a plural car, cars. But it is my mistake that I named the entity with plural.

About your last paragraph:

As you said I use DI and interfaces, because I have some unit tests for it. I am probably doing it so much, because of the Dependency Inversion definition. That I should not instantiate the implementation because it will probably break the Open/Close principle. I really try to stick with rules, but probably I understand them well in some small examples, but totally get lost in real big business scenario which means that I don't understand them :D

Answer 1

Some general remarks:

• What is the point in shoving everything in a namespace called "Library"? All of your usings now are eight characters longer for no reason.

• Same for the "Persistence" and the "Services" namespaces, which themselves seem to mainly serve as namespaces which contains other namespaces ("Models", "DataServices",...)

• Your entire structure seems to be splitting up code by "type". Looking at what you present here, I wonder if most of those folders contain more than a handful of files. To me this feels outdated, an old way of working which creates much pointless overhead.

  These days we tend to group code by "topic", e.g. if your business logic deals with applications and environments, all the code related to one of these gets grouped under a namespace called "Applications" or "Environment", and inside that namespace you can split the files by function (e.g. "Creating" or "Deleting") if the list becomes unwieldy.

• A "DeliverysetSaverService" which has a "SaveDeliveryset" method feels redundant to me. Call the method "Handle" or "Execute".

• Same with "_deliverysetVersionInfoByIdRetrieve" and "GetVersionInfo". Why does "deliverysetVersionInfoByIdRetrieve" even need to start with "deliveryset"? Isn't all the code in DeliverysetService about deliverysets? And what is a "Retrieve"?

• Same for "_deliverySetFileChangesCoordinator" and "GetDeliverysetChangesInfo".

• "IsAllAreEqual" is very confusingly named.

• WRT "subtitleFilesList": do not call something a "xxxxxxxList". A collection of cars should be named "cars", not "carsList".

• Why do you have a class with a plural name: "SubtitleFiles"?

---

Honestly, I get completely lost in the maze that is your code. It feels overly complex to me, very focused on its implementation instead of its function. At several places I notice that if I want to find out what happens in a specific method, I need to go looking at how it it implemented from an interface, only to find a class that itself calls other methods which are also implemented from an interface, etc.

I get that you feel you need this for DI and perhaps testability, but at some point it becomes so complex that it is simply not maintainable. I really wonder if some of your code couldn't be significantly simplified by just calling a static class.