How to manage files that have behaviours and defer to an implementation at the same time?

Background

I have an umbrella project that has several apps. One of these apps, is called manager and it has a Public API that is used by other apps.

The file where all of this happens does two things at the same time:

  • Specifies the behaviour of the Public API
  • Defers to the implementation of the Public API

Code

Here is an example of such a file:

defmodule Manager do
 alias Manager.{Interpreter, PriceAnalyst}
  
  @callback activate(String.t, String.t) :: any
  @callback valid_strategy?(String.t) :: boolean

  ##### Implementation ##### 

  @behaviour __MODULE__

  @impl __MODULE__
  defdelegate valid_strategy?(strategy), to: PriceAnalyst

  @impl __MODULE__
  defdelegate activate(syndicate, strategy), to: Interpreter
end

Problem

Now, the problem with this is that it doesn’t really work. Specifically, this code generates the error:

got “@impl Manager” for function valid_strategy?/1 but this behaviour does not specify such callback. There are no known callbacks, please specify the proper @behaviour and make sure it defines callbacks

Which is crazy to me, because I am defining the callbacks.

Alternative Code

Another way of doing the same thing that avoids the error would be:

defmodule Manager do
  alias Manager.{Interpreter, PriceAnalyst}

  @callback valid_strategy?(String.t) :: boolean
  @spec valid_strategy?(String.t) :: boolean
  defdelegate valid_strategy?(strategy), to: PriceAnalyst

  @callback activate(String.t, String.t) :: any
  @spec activate(String.t, String.t) :: any
  defdelegate activate(syndicate, strategy), to:  Interpreter
end

However this way I run the risk of deviating the defdelegate from the callback, since this is not implementing any behaviour. This is not a risk I want to incur into.

Questions

So, by now I am sure some of you may be asking:

  • If this file does 2 things, why not have 2 files?

And it is a fair question. The problem here is:

  • What would I name such files?
  • Where would I put them? (what would be the folder structure in this project?)

Both questions have no clear answer to me.

So, a couple of final questions remain:

  • How can I avoid the behaviour error?
  • How would you solve this issue?

A module cannot define a behaviour and implement it as well. Therefore you need 2 modules as you seem to have gathered already. But that’s not what I’d gather you should fix/change.

What you seem to be missing is the following: Usually the implementation implements a behaviour not the delegating abstraction. So within :manager the implementation seems to be PriceAnalyst. It needs to implement a behaviour. The behaviour might be Manager.PriceAnalyzer.

Now you’ll have (iirc) :cli, which depends on :manager. If you want to mock out that dependency in :cli you’d limit the implementation calling functions on Manager to a single module, say: Cli.Backend.Manager and this one implements a behaviour as well. But this time it’s a behaviour defined within :cli (e.g. Cli.Backend), which caters to the needs of what exactly :cli needs of any backend implementation.

The public interface of the Manager module doesn’t really need a behaviour. It’s the module as is.

When testing :manager you can mock Manager.PriceAnalyzer, when testing :cli you can mock Cli.Backend. This way whenever :manager adds new features (like new callbacks), :cli doesn’t need to change at all – at least unless it as well needs new features based on those added to :manager.

Edit:

If you don’t need to mock anything within :manager you can surely skip that step. The important part is that :cli should not mock behaviours of :manager.

This example is actually a simplification of the actual issue. The real Manager module does not only delegate to PriceAnalyst, it also delegates to other modules. Following is a more complete version (though not the full thing, for the sake of brevity) of the file:

defmodule Manager do
  alias Manager.{Interpreter, PriceAnalyst}

  @callback activate(String.t, String.t) :: any
  @spec activate(String.t, String.t) :: any
  def activate(syndicate, strategy), do: Interpreter.activate(syndicate, strategy)

  @callback valid_strategy?(String.t) :: boolean
  @spec valid_strategy?(String.t) :: boolean
  defdelegate valid_strategy?(strategy), to: PriceAnalyst
end

I thought that by removing complexity and simplifying my example I would make life easier for the people trying to understand the issue, but now I see my simplification removed too much and has led you astray.

Apologies for that. I hope now the issue is more clear.


EDIT

I have updated the Description of the issue to better reflect the problem at hand. It should be clearer now.

To be honest, this doesn’t change anything to my answer: Manager doesn’t need to implement a behaviour. You might have behaviours in :manager, but they’re implemented by the modules actually doing the work not by Manager. Anything depending on :manager should create its own behaviour, which is then implemented by a module, which calls functions on Manager. Those dependant apps should only mock their own behaviours.

You might be interested to hear about knigge which is a library I wrote to literally address this exact issue.

It’s opinionated in that it assumes that you have a single behaviour and delegate to one implementation of this behaviour but it certainly goes into the direction you’re suggesting here.

The thing is, if I do it your way, I will have a Cli.Backend.Manager module that will be in practice a copy of the Public API of Manager. This way, changes on Manager will always mean I have to change Cli.Backend.Manager as well. If I forget to do that, Cli.Backend.Manager becomes an ad-hock mock, i.e., a mock that doesn’t represent the real thing (that real thing being Manager's Public API).

This was the issue I tried tackling in the first discussion, and the solution I found was to just make Cli depend on Manager's Public API. This way, changes in Manager's Public API create compile errors in the Cli, which is good imo, because it forces me to have both apps in sync.

I am unsure of this solution. I understand the “Don’t mock what you don’t own” principle, but I am not really sure it applies here, as I own all the apps. Even so, adding a Facade just because of that doesn’t seem to grant me any benefits (or at least I don’t see them yet).

That’s imho a big fallacy. This might for sure be true right now as you’re essentially developing one functionality in two applications forcefully trying to add separation. But imagine besides :cli there’s also a :http dependant on :manager. The manager adds some kind of session handling because the http app needs that. The :cli app doesn’t and therefore shouldn’t be bothered by the change in :manager. That’s also the point where you’re no longer dealing with 1:1 copies. The :cli behaviour will become different to the :http behaviour and both will be different to the functions on Manager. It also automatically means :cli can at any point have another implementation of its behaviour, which doesn’t even depend on :manager anymore. That independence is actually what ideas like DDD or “Don’t mock what you don’t own” want to point people to. It’s also the reason people usually ditch those principles for smaller scale, simple projects. The independence only becomes truely useful when things grow.

If you treat all your apps as “your own” then there is no separation. I still think you don’t need a behaviour for Manager though, but only for the implementations within :manager. Generally you can ask the question of: How would it work if everything would be in one application and take the answer.

This option however raises some questions:

  1. Will the Cli app ever depend on something else that is not Manager?
  2. Will a change in the Public API of Manager somehow not affect Cli.Backend.Manager?

My answer to both would be:

  1. No. I don’t see a scenario where this would ever be possible (right now). If somehow it happens one day, I will have to do major changes to Cli anyway even if I have a Facade.
  2. Again, I don’t see how changes in one would not mean changes in the other. Furthermore, this much separation means I will have several layers of indirection in my app. Changes in Manager's Public API will eventually mean changes in Cli.Backend.Manager and then changes across Cli in a very classic Shotgun Surgery code smell: https://en.wikipedia.org/wiki/Shotgun_surgery

I must admit I am terrified of point 2, having to work in a project that suffers from this issue I can say it doesn’t feel nice.

But, going with your solution I have one more question:

  • What would be the contents of Cli.Backend?

I understand that Cli.Backend.Manager would be a Facade of Manager, so if that is the case, what would be the contents of the file Cli.Backend? Would it be an empty file?

If the cli mocks a manager behaviour it means it needs to “change” with any change to the behaviour. You just don’t notice it because mox automatically implements all callbacks.

In your current situation I feel you could just move the behaviour you have in :manager into :cli, have a super dump implementation with Cli.Backend.Manager, which basically delegates everything to Manager. Currently :cli is driving the interface and Manager just does need to provide the implementation.

But what does this give you:

Again, another client to :manager requires some function to be split up into two separate functions – say fun_old/2 becomes fun_a(a) |> fun_b(b). So the public API of :manager changed. Instead of needing to hunt down each fun_old/2 call in :cli you can just leave fun_old/2 be the interface of Cli.Backend, but change only the implementation:

defmodule Cli.Backend.Manager do
  @impl Cli.Backend
  def fun_old(a, b) do
    a |> Manager.fun_a() |> Manager.fun_b(b)
  end

  # If things match 1:1
  @impl Cli.Backend
  defdelegate other_fun(a), to: Manager
end

The core of :cli didn’t change at all. It still depends on fun_old/2 to be available like before. The implementation with manager is the only place concerned with the change in Manager.

This again is something you benefit the most once things actually diverge and no longer fit 1:1. As long as Manager is a perfect implementation for the interface :cli depends on this will look like useless indirection as you said.

2 Likes

I messed with an imperfect metaprogramming solution to this a few years ago: https://gist.github.com/christhekeele/917c430a57bc6eccc3227f90928f396c

A better pattern is to define a Thing.Behaviour module and a separate Thing.Behaviour.Impl module for other people to use. They can even pass the latter to defdelegate, but I don’t recall if that lets them flag things as @impl.

Playing around with this architecture something struck me: How do you then test Cli.Backend.Manager?
You would need a mock for Manager and you would eventually find the same issue I have here - you would need to use the behaviour directly from Manager or you would be forced to do an integration test to make sure you are calling it with the correct parameters.

I don’t unit test something like that. Generally when your testing implementations for otherwise mocked behaviour they’re integration tests and in your case would call Manager proper. There should be hardly any business logic in there besides mapping the behaviours api <-> the implementing libraries api.

Edit:

Which isn’t to say you configure :manager to use its own mocks for those integration tests. But it clearly is a integration test, as you’re integrating with :manager for the test. :cli alone can’t test this.

1 Like

Although a discussion regarding “How to approach testing” is out of the scope of this post, I would say I am more on the side of J. B. (Joe) Rainsberger and I usually avoid Integration tests. This is not to say I have 0, it’s just I prefer not having to deal with them as they are quite hard to maintain.

I am usually on the side of “If I know I am sending the correct information to Manager, then I don’t care what happens next because it is not my problem”. This is why I have the Manager behaviour in the first place. technically I don’t need it, but since I am using Hammox then I must have a behaviour so I can have my unit tests. Thus, I place the Public API in the same file of the behaviour.

This allows me to test that I am “sending messages with the correct format” to Manager (because Hammox will blow up at compile time if I am not) and the test itself makes sure I am sending the “messages with the correct content”.

For these reasons I am leaning on going with the alternate code of my question. I was wondering if there was another solution I was missing, but after reading (and trying) the options everyone provided me here I am still not convinced that all the extra work will be worth it.

A very interesting discussion and I thank everyone for their participation (special kudos to @LostKobrakai !)

So as mentioned prev. you could treat your umbrella like a single application and all my points become irrelevant. But if you don’t do that then even "sending the correct information to Manager" is something :cli can’t know.

1 Like

Regarding your Impl suggestion, I believe you could benefit from reading this article:

1 Like

Tbh I’ve just skimmed the but there are a few thoughs. There is not just one implementation if you’re mocking the implementation. There’s always at least the implementation you use in prod and the mock. That’s 2.

The other part is having the interface and the implementation in one package. That’s a valid concern. You could have a third application, which depends on both :cli and :manager to hold the implemenation, then :cli wouldn’t even depends on :manager at all. You can see the same e.g. in :phoenix <- :phoenix_ecto -> :ecto.

Agreed my suggestion is poor naming. This case differs slightly from the article, as the module I describe is not “concrete implementation” here so much as “default implementation”, for concrete-land to fall back to. “Default” probably works better though I still don’t love it.

1 Like