1

I have Document and ExternalDocument classes in my system, where ExternalDocument extends Document. The main distinction is that ExternalDocument holds onto externalDocumentId and externalEventId data in order to correlate with the external system.

Documents may be overwrote calling document.overwrite(a, b, c). When overwriting external documents I want to track the externalEventId that triggered the change and this is where the design falls apart.

According to the LSP I shouldn't strengthen preconditions in document.overwrite. I could implement an document.externalOverwrite operation and throw an exception when document.overwrite is called directly, but that stills violates the LSP.

The language I use doesn't support generics so I can't go for Document<T> either where T defines the override contract parameter.

I could solve the problem by not inheriting from Document at all and use composition instead, but it feels weird given ExternalDocument still is a Document specialization.

Any guidance?

EDIT:

Just to give a little more context, local documents can be overwrote by a local/user process. External documents are a reflection of documents existing in an external system. I want to communicate the fact that we do not have authority over external documents. The state of those documents is updated in response to remote system events and I want to be able to correlate every state change with a corresponding externalEventId.

Note that some local document operations remain valid on the external ones though, like assigning the document, etc. I'm also trying to keep the business logic within the model as much as possible to avoid an anemic domain model.

After thinking a little more about it I think I may have conflated both "overwrite" operations as one although overwriting a local document & external document are actually distinct processes. I think we could make a parallel between this and having multiple kinds of locks: they can all be opened, but all in very different ways that would be hard to generalize.

Therefore, so far the most logical route seems to be splitting the current concrete Document into Document (abstract base class) and LocalDocument. The overwrite operation would be implemented on both LocalDocument and ExternalDocument. Both implementations could leverage an internal overwrite implementation living in the Document abstract class for parts of the process that are similar.

Obviously clients would have to know what type of document they are dealing with in order to process an overwrite.

Any new suggestions in light of those precisions?

plalx
  • 389
  • 1
  • 10
  • 4
    Can you clarify: " I want to track the externalEventId that triggered the change" ... "I shouldn't strengthen precondition" What exactly does "track" mean, and how does it constitute a strengthening of the preconditions of overwrite? – Alexander Oct 08 '20 at 19:07
  • 2
    Also worth noting: the fact that in the "real world" an ExternalDocument is a Document is not in itself sufficient justification to make ExternalDocument a subclass of Document. Consider the famous square/rectangle problem. – Alexander Oct 08 '20 at 19:08
  • @Alexander-ReinstateMonica Ok so basically changes to an ExternalDocument are triggered by receiving an event from a remote system in which case I want to track the ID of the event that triggered the change along with the new state. Both overwrite and externalOverwrite are probably distinct operations. However, externalDocument.overwrite shouldn't normally take place as we do not have authority over those documents. – plalx Oct 08 '20 at 19:13
  • I would probably model that by having something like a PurposeForChange (or something like that) interface, which is an input to overwrite. In the general document case, it can be NoReason object (until you have a business requirement to track why those happened, too), and in the ExternalDocument, it can be a different object that contains the id of the external event that triggered the change. Unfortunately without covariant types, you don't have a way of saying that ExternalDocument.overwrite excepts a ExternalDocumentChangeReason instead of a PurposeForChange – Alexander Oct 08 '20 at 19:24
  • What about a Document abstract class with StandardDocument and ExternalDocument. The abtract class wouldn't expose any operation but I could still use it in the repository's contract. However, service classes must now cast to explicit document types, but they kinda need to anyway since we deal with those types differently. – plalx Oct 08 '20 at 19:27
  • By "any operation" I mean the overwrite operation. There are still operations common to both. – plalx Oct 08 '20 at 19:33
  • Let us continue this discussion in chat. – Alexander Oct 08 '20 at 19:36
  • You are going to have a problem in the future with the design you're proposing, very tightly coupling the document and external document together. In the long run, the best approach is to model the external document as a decorator to a document, taking a Document instance as an argument for composition. In your case, you should give up the inheritance altogether. – Andy Oct 08 '20 at 19:37
  • @plalx I posted and then deleted that abstract base class idea. Technically it will work, but there are better solutions like Alexander and Andy are proposing. – Rik D Oct 08 '20 at 19:39
  • @Andy I guess I could give-up on inheritance all-together. It can be a bit annoying though if 99% of other operations should be inherited. The caller needs to know which type of document it's dealing with to carry out the overwrite operation in all cases it seems, so I guess declaring the operation on the concrete specializations without having it in the base class could make sense? – plalx Oct 08 '20 at 19:47
  • @RikD Given that overwrite and externalOverwrite are conceptually two distinct processes I think removing the operation from the base class seems to make the most sense in the end, no? All the solutions require the client to know what process it's fulfilling anyway, whether a reason is used or generic types could be used. – plalx Oct 08 '20 at 19:59
  • Any "is a" is a form of classification and implies a classification criterion. E.g., in ordinary geometry a square is not a circle, but in topology it is (because the criterion there is the way the thing is connected). For us (when it comes to interface/type inheritance), the criterion is: this thing abstractly "behaves as that other thing" in some defined way. So if ExternalDocument doesn't behave as a Document, then it's not a specialization of a Document. Now, it is possible that you've defined the behavior of the Document too narrowly - that's worth considering too. – Filip Milovanović Oct 08 '20 at 20:27
  • @FilipMilovanović So assuming there are 10 other operations which are shared and behave properly does it make sense to inherit and implement overwrite on the specializations only? – plalx Oct 08 '20 at 20:59
  • I don't know enough about your problem domain, but here are a few thoughts. If you cannot really make this work in an elegant way, maybe you could separate the concepts of document and document changes. You mentioned events. One possible route is to reconceptualize so that instead of having an overwrite method, you have something that produces an Overwrite command (which is an object that holds the parameters of the former overwrite method as immutable private fields); a derivative command could similarly store an externalEventId, and all could have a common interface (e.g. execute(). – Filip Milovanović Oct 08 '20 at 21:45
  • P.S. Of course, I can't know if this is feasible whithout being familiar with the codebase, but perhaps this line of reasoning can help you think out of the box and come up with a good idea. – Filip Milovanović Oct 08 '20 at 21:45
  • P.P.S. But this is one of the problems that arise when objects have too many responsibilities, and you need to have an abstraction organized around them - the more things they can do, the harder it is to come up with a generalization that's still useful. So, splitting this class may be a good idea anyway (at lest it's something to consider). – Filip Milovanović Oct 08 '20 at 21:51
  • @plalx I will undelete my abstract base class answer, to allow comments on the idea. – Rik D Oct 08 '20 at 22:01
  • It's very hard to read when you use code highlighting to highlight non-code. Just because you refer to a thing declared or called code doesn't mean you are writing code. – philipxy Oct 09 '20 at 04:32
  • [...] It can be a bit annoying though if 99% of other operations should be inherited. [...] This is why the ExternalDocument contains a Document instance, which is accessible through a getter and on which operations may be executed (alternatively you can introduce proxy methods, something like this: https://pastebin.com/ssn92bxS). – Andy Oct 09 '20 at 06:13
  • @Andy If I have to go all the way to dispatch 99% of the operations or implement a proxy what's the advantage over inheriting AbstractDocument that contains all those operations and implement the 1% on specializations? I'm all for composition over inheritance, but delegating all operations clearly is a smell IMO. – plalx Oct 09 '20 at 11:36
  • LSP says not to strengthen the precondition, but that doesn't mean you can't modify/refactor the original Document class as needed. Further, you don't say how the precondition has been strengthened. If you require two extra id's in the constructor of ExternalDocument, then the information is there to use whenever overwritten. Otherwise there's not enough information here to understand the issue. – Erik Eidt Oct 09 '20 at 12:02
  • @ErikEidt See comments below Rik D's answer. – plalx Oct 09 '20 at 12:05
  • We don't know the workflow around Documents, when the id's are generated relative to the the ExternalDocument, relative to overwrite. We know that overwrite takes (a,b,c), but that is not really informative. One might guess there are two different workflows here (internal vs. external), that the external document workflow might best be handled by an external document update manager class that knows and handles the id's. I still assert that we don't have enough information which is why this question has such varied and speculative answers that do not really seem actionable. – Erik Eidt Oct 09 '20 at 15:06
  • @ErikEidt It's quite simple. Local documents can be overwrote by a local/user process. External documents are a reflection of documents existing in an external system. I want to communicate the fact that we do not have authority over external documents. The state of those documents is updated in response to remote system events and I want to be able to correlate every state change with the externalEventId. Some local document operations remain valid on the external ones though, like adding notes, assigning the document, etc. I'm also trying to keep the logic business logic within the model. – plalx Oct 09 '20 at 15:15
  • @Filip Milovanović Updated the question. – plalx Oct 09 '20 at 16:16
  • @Andy Updated the question. – plalx Oct 09 '20 at 16:17

5 Answers5

1

Indeed, LSP says that everywhere you use a Document you should be able to use an ExternalDocument. This is why you cannot strengthen the precondition. As a consequence, externalDocument.overwrite(a, b, c) should be valid whenever document.overwrite(a, b, c). If overwrite means just not to change the externalDocumentId everything is fine (I could overwrite an external document with the content of an internal document that would replace the content of the external document without changing its identity).

So far so good. But you seem to introduce and additional responsibility to track the origin on the top of external documents. This is no longer about extending the concept of Document: It's adding new responsibilities. Objectively, it seems to me that tracking change events could also make sense for internal documents.

For this reason, your ExternalDocument appears to be only in part an extension. The best way to improve the design is tho ensure separation of concerns between the real extension and the additional responsibilities:

  • limit the ExternalDocument to a Document extension that is LSP compliant;
  • use the decorator pattern to add to a Document or an ExternalDocument the responsibility of tracking change events.
Christophe
  • 77,139
  • Ok, but what if it is prohibited to manually (locally) override an ExternalDocument? If that behavior is inherited and prevented we come back to the LSP violation. – plalx Oct 08 '20 at 19:56
  • @plalx one can override a method. But one does not override a class. The decorator pattern does not override a class: it uses composition over inheritance in conjunction to add responsibilities. So I don't fully get your additional question. If there were requirements missing in your question, please add them there :-) – Christophe Oct 08 '20 at 20:12
  • I am well aware what the decorator pattern is. What I mean is that decorators must extend Document. If the overwrite operation is defined in Document then we have the same problem with a decorator or a sub-class. I think the only sane solution is not to define the overwrite operation on the base class extended by ExternalDocument. An overwrite is not the same business process as an externalOverwrite in the end... – plalx Oct 08 '20 at 21:17
  • @plalx the problem is not the same for the decorator the decorator adds new functionality. So you don't touch the exisitng (extended) logic of the parent (i.e. overwrite without event). And if you don't touch parent, there is no LSP issue. What you'd do, is to add a an externalOverwrite() (or better, a traceableOverwrite() to make it useful on any kind of documents). – Christophe Oct 08 '20 at 22:02
  • @plalx But if you want to make the native overwrite unusable and force the event-based vriant, there there is no need for a decorator, because indeed you'd violate LSP in one way or another. Then just accept it in your design for justified reason, but don't forget that you'll loose a couple of benefits and increase nasty risks – Christophe Oct 08 '20 at 22:03
  • If I understood things well, the core problem (in terms of LSP) is that the Document type assumes that overwrite can be called at any time, but now External Document requires you to set an event ID before you can call overwrite (which can be achieved with a decorator, through its constructor), but there also seems to be a constraint that you only call overwrite in response to an event, and that's a bit trickier - you'd have to re-wrap the (external) doc in a decorator every time. This could be OK, but it could also mess up the semantics of the client code, depending on what it's doing. – Filip Milovanović Oct 08 '20 at 22:26
0

One way to solve this using inheritance is to create an abstract base class BaseDocument that has all the functionality your current Document class has.

Both Document and ExternalDocument can now implement their own overwrite method.

Edit: I deleted this answer because the overwrite method is not part of the base class. Therefor it’s not possible to ‘program to an interface’. I undeleted this answer to allow others to leave comments for the OP why this approach is not ideal.

Rik D
  • 4,801
  • Actually, if the OP can find a way to treat Document and ExternalDocument separately for the purposes of overwriting, and the same for other purposes, then it's fine to have a base class or an IDocument interface without the overwrite method, so this could be a feasible solution. – Filip Milovanović Oct 08 '20 at 22:45
  • @FilipMilovanović I think this is the best solution so far. overwrite and externalOverwrite are two distinct processes which should be modeled as such. If all documents can't be overwrote in the same way then the base class shouldn't implement that operation obviously. If I had to design up-front that's probably what I would have done, but since Document already existed I only thought of problems inheriting Document would cause. Given there are other shared operations inheriting seems to make sense instead of using composition and dispatching most. – plalx Oct 09 '20 at 11:46
  • It's like if we were modeling locks. They may all have common attributes and certain common operations, but they are all opened differently, hence open couldn't be part of a base class. Some would model open(key), others open(combination). The client has to know the type of lock he's dealing with. In that case perhaps composition would make sense like Lock configured with a LockType strategy, but in that case it would have to be generic so that open(T) matches LockType<T> which is not something I can do in my language. – plalx Oct 09 '20 at 11:57
  • We could always model the opening process using a visitor such as lock.open(lockOpenerVisitor), which double-dispatches to lock.openWithKey(key), but ultimately the specific Lock specializations must implement their own open operation or at least a different protocol for opening e.g. lock.insertKey(key); lock.turnKey() -> LockOpened, etc. – plalx Oct 09 '20 at 13:07
0

Are you absolutely sure all local documents can be overwritten?

They might be read-only, be it due to access-control or whatever. In which case, I would expect some error-indication.

Thus, an ExternalDocument objecting when you try would not violate the base contract at all, even if it does so unconditionally.

Deduplicator
  • 9,031
  • 2
    That's what I though, at first - you can just say that the overwrite method can throw an exception (even though the base class never does that), and LSP violation is gone, when it comes to exceptions. But, if I understood the question well, the strengthened precondition really comes from the fact that, with the external document, you can't call overwrite anymore at will (once you have an instance); you have to obtain an event id first, and somehow plug it into the object - and this is what actually messes with clients that need to use Document polymorphically. – Filip Milovanović Oct 08 '20 at 22:34
  • Exactly, processing an external document override requires additionnal inputs such as the externalEventId as a correlation ID. – plalx Oct 08 '20 at 23:30
  • @FilipMilovanović That there is a different way to accomplish some action is independent from the inherited way always being denied. The different way is a pure extension, and thus cannot violate LSP. – Deduplicator Oct 08 '20 at 23:34
  • @Deduplicator - I'm not sure if we understood each other, but the problem is the client; if there's no LSP violation, you don't have to change the client at all (e.g., you may find a way to deal with the event id somewhere else, before the documents are accessed by client code, and it would all just work). However, if the OP cannot find a way to make this work without changing client code, this is precisely what indicates an LSP violation (in the design as it now, against witch the client was written). – Filip Milovanović Oct 09 '20 at 00:19
  • I'm not saying that this cannot be reconceptualized in a way similar to what you're suggesting, or otherwise redesigned. My point was that exception/denial is not the actual problem OP is trying to deal with, it's just something that appeared as a consideration to the OP while trying to work out through the underlying issue (this new document derivative breaking expectations assumed when clients of Document were written; the OP can't find a clean way to fit it into existing structures). – Filip Milovanović Oct 09 '20 at 00:19
  • P.S. For clarity; imagine there's a function that processes a list of documents (polymorphically). It calls overwrite, perhaps multiple times. There's no room there to deal with event id's (unless you change the code and check the type). If you can't arrange for the (external) docs themselves to contain correct event id's before each overwrite call, then, sure, you can make it "work" by just deniyng all external documents. But the OP still needs to handle them elsewhere, somehow. In other words, if this solves the LSP problem, it does so pro forma, and not in a meaningful way. – Filip Milovanović Oct 09 '20 at 00:45
  • @FilipMilovanović I would consider the client deciding to overwrite and the client receiving and dispatching an event causing an overwrite two completely different things. Shoehorning the latter due to partial superficial similarity into the former is causing the OPs current pain. It's just too different to fit properly. – Deduplicator Oct 09 '20 at 01:16
  • I agree with that (that's essentially what I said in my comments to the question itself - it's probably better to rethink some of the aspects of the design and separate this responsibility out in one way or another). – Filip Milovanović Oct 09 '20 at 02:00
0

There are two different situations: One, you use inheritance so that an "ExternalDocument" can be used wherever a "Document" can be used. In that situation it should have compatible behaviour. A caller should have no need to know whether they have a Document or an ExternalDocument.

There are other situations where a caller knows nothing about "Document". They are not interested one bit how ExternalDocument is implemented, including that it is a subclass of "Document". It stands on its own. You will never ever be using an ExternalDocument in a place where a different subclass of Document can be used. That's rare. If that's the case, and you don't ever substitute one for the other, then there are no violations.

gnasher729
  • 44,814
  • 4
  • 64
  • 126
0

If you want to add extra parameters to a method without changing the method signature, inject a context object into the constructor. ie

ExternalDoc
   ExternalDoc(IContext context)

public override Overwrite(a,b,c)
   {
      base.Overwrite(a,b,c)
      this.trackChange(this.context.ExternalEventId, "Overwritten")
   }

Prior to calling overwrite, you set the ExternalEventId in the context, which you keep a reference to after passing it into the ExternalDoc constructor

Ewan
  • 75,506