Introducing a new subclass to an established system using inheritance which violates the Liskov Subs-CodePudding

Question: are there any design principles other than the Liskov Substitution Principle to consider when introducing a subclass with a subset of existing base functionality to an established inheritance system?

Context: We have an established system that models dozens of different types of an entity with shared base functionality. As such we have base database tables, base aggregates, base helpers, base validators etc. and then entity type specific subclasses which inherit from the base classes.

We have been asked to introduce a new type of entity which implements a subset of this base functionality only. From what I understand it would violate the Liskov Substitution Principle to leave the base functionality in place and override the affected properties and methods in subclasses for the new entity only. However, it seems counterintuitive to completely overhaul the system to:

move the database fields required by all entity types except the new one to a related table
change the base aggregate, helper and validator implementation to only load / get / update / validate the subset of base data and override this in all entity type specific aggregates (even if they use a common adaptor or helper)
etc...

just for one new entity type, knowing that all others will need the previously shared database fields and base implementation.

In a case like this is it acceptable to violate the LSP? Are there any other design principles that apply to this situation?

Detailed example: Consider an Assignment management system:

There are many types of Assignment but all have a Person assigned.
They all have AssignmentNotes, which allows for zero to many Notes to be added to the Assignment.
The AssignmentNotes also allow for the Assignment to be marked as a group Assignment with an optional comment, and the Persons involved in the Assignment to be identified.

There are Aggregate, Helper and Validator classes for each Assignment type, but because of the shared functionality they inherit from a BaseAssignmentAggregate, BaseAssignmentHelper and BaseAssignmentValidator respectively. The BaseAssignmentAggregate takes care of loading and updating shared data, for example:

public virtual async Task Load(int id)
{
    Model = await Context.Assignment.SingleAsync(a => a.Id == id);
    
    await Context.AssignmentNotes.Where(a => a.AssignmentId == id)
        .Include(an => an.AssignmentNotesNote)
        .Include(an => an.AssignmentNotesGroupAssignmentIncludedPerson)
        .LoadAsync();    
}

public async Task SaveAssignmentNotes(IReadOnlyCollection<int> noteIds, bool isGroupAssignment, string groupAssignmentComments, IReadOnlyCollection<int> groupAssignmentIncludedPersonIds)
{
    AssertIsLoaded();
    if (noteIds == null) throw new ArgumentNullException(nameof(noteIds));  
    ...

    // synchronise notes
    ...
    
    Model.AssignmentNotes.IsGroupAssignment = groupAssignmentComments;
    Model.AssignmentNotes.GroupAssignmentComments = groupAssignmentComments;
    
    // synchronise group assignment people
    ...

    // save
    await SaveChanges();
}

Relevant entity types from a DbContext could be as follows:

public partial class Assignment
{
    public int Id { get; set; }    
    public byte AssignmentTypeId { get; set; }    
    public int AssignedPersonId { get; set; }
        
    public virtual Person AssignedPerson { get; set; }    
    public virtual AssignmentNotes AssignmentNotes { get; set; }    
    public virtual AssignmentType AssignmentType { get; set; }        
    public virtual AssignmentTypeADetails AssignmentTypeADetails { get; set; }     
    public virtual AssignmentTypeBDetails AssignmentTypeBDetails { get; set; }     
    public virtual AssignmentTypeCDetails AssignmentTypeCDetails { get; set; }     
    public virtual AssignmentTypeDDetails AssignmentTypeDDetails { get; set; }     
    ...
}

public partial class Person
{
    public Person()
    {            
        Assignment = new HashSet<Assignment>();            
        AssignmentNotesGroupAssignmentIncludedPerson = new HashSet<AssignmentNotesGroupAssignmentIncludedPerson>();            
    }

    public int PersonId { get; set; }        
    public string Name { get; set; }
            
    public virtual ICollection<Assignment> Assignment { get; set; }        
    public virtual ICollection<AssignmentNotesGroupAssignmentIncludedPerson> AssignmentNotesGroupAssignmentIncludedPerson { get; set; }        
}

public partial class AssignmentNotes
{
    public AssignmentNotes()
    {            
        AssignmentNotesNote = new HashSet<AssignmentNotesNote>();
        AssignmentNotesGroupAssignmentIncludedPerson = new HashSet<AssignmentNotesGroupAssignmentIncludedPerson>();  
    }

    public int AssignmentId { get; set; }
    public bool IsGroupAssignment { get; set; }
    public string GroupAssignmentComments { get; set; }
    
    public virtual Assignment Assignment { get; set; }        
    public virtual ICollection<AssignmentNotesNote> AssignmentNotesNote { get; set; }
    public virtual ICollection<AssignmentNotesGroupAssignmentIncludedPerson> AssignmentNotesGroupAssignmentIncludedPerson { get; set; }
}

public partial class AssignmentNotesNote
{
    public int Id { get; set; }
    public int AssignmentId { get; set; }
    public int NoteId { get; set; }

    public virtual Note Note { get; set; }
    public virtual AssignmentNotes Assignment { get; set; }    
}

public partial class AssignmentNotesGroupAssignmentIncludedPerson
{
    public int Id { get; set; }
    public int AssignmentId { get; set; }
    public int PersonId { get; set; }

    public virtual Person Person { get; set; }
    public virtual AssignmentNotes Assignment { get; set; }
}

Now say we are asked to introduce a new AssignmentTypeZ which can never be a group Assignment i.e. this AssignmentType can still have Notes, but it can never be IsGroupAssignment, it can never have GroupAssignmentComments and it will never have any AssignmentNotesGroupAssignmentIncludedPerson.

Is it now invalid for these properties to exist on the AssignmentNotes table and all related base classes? Is there another design principle involved or am I now obliged to

Shift these properties to say a new AssignmentNotesGroupAssignment database table?
Change the BaseAssignmentAggregate to not Load this table or implement the properties on it, and override all other AssignmentTypeAggregates to do so?
Change the BaseAssigmentAggregate to only update Notes, and override all other AssignmentTypeAggregates to update AssignmentNotesGroupAssignment details?
etc...

just for one new entity type?

CodePudding user response：

Is it now invalid for these properties to exist on the AssignmentNotes table and all related base classes?

I think it's important to draw a stark line here between objects and data. The most common software application design is probably "database-driven-design" where the database schemas are designed first, and every "object" is simply a representation of a database table. However that is a terrible way to do OOP, because database tables persist data, not objects. Tables do not persist inheritance or polymorphism or encapsulation or any of the business logic that is the heart of an object in OOP.

OO application design should be independent of persistence, in the sense that the same application could run on top of NoSQL or RDBMS or flat files or JSON data pulled from a network. It doesn't matter.

That's a long-winded rant, to say the LSP doesn't care about the AssignmentNotes table.

When it comes to the AssignmentNotes class, the LSP has no problem with a subclass that always returns false for IsGroupAssignment and never populates GroupAssignmentComments or AssignmentNotesGroupAssignmentIncludedPerson. After all, another subclass would be valid in the same state, even if it does allow other states as well.

The LSP problem stems from this method signature: SaveAssignmentNotes(IReadOnlyCollection<int> noteIds, bool isGroupAssignment, string groupAssignmentComments, IReadOnlyCollection<int> groupAssignmentIncludedPersonIds).

Even before adding the proposed subclass, that method seems problematic, because the isGroupAssignment parameter dictates whether the last two parameters can be populated or not, meaning the method can already be invoked with an invalid combination of arguments.

I would split the method in two.

SaveAssignmentNotes(IReadOnlyCollection<int> noteIds)
SaveAssignmentNotes(IReadOnlyCollection<int> noteIds, string groupAssignmentComments, IReadOnlyCollection<int> groupAssignmentIncludedPersonIds)

Ideally, these two methods would be in separate base classes: one which supports group assignments and one which does not. If you keep them in the same class, document the second one as optional, for AssignmentNotes that support groups.

Either way, the solution for Liskov is always to clearly document preconditions, postconditions, and invariants. The LSP is a syntactic and semantic principle, meaning an API contract is not just a method signature but also its documentation. A base class can define optional behavior (in this case for saving "unsupported" fields) but it must also define expected behavior for subclasses that don't implement the optional behavior, so clients are not surprised by that subclass.