sasa1977/boundary

Subdomains unexpected boundary violation

aspett opened this issue ยท 13 comments

aspett commented

๐Ÿ‘‹ Howdy. I'm experiencing an issue with subdomains.

Below is a contrived example - I am actually having this issue in a larger codebase and wanted to reproduce it on a smaller scale.

If I have these boundaries:

  • Blah (deps: [], exports: :all)
    • Blah.Context (deps: [], exports: :all)
  • BlahWeb (deps: [Blah], exports: :all)

Then I have a file Blah.Context.Subcontext.Module with a call/0

Compiling gives a forbidden reference error. This is unexpected to me, since Blah exports :all and Blah.Context exports all.

What I've tried

  • โŒ Using exports: [{Subcontext, []}] syntax does not help.
  • โœ… If I specify in Blah.Context the exact modules to export (exports: [Subcontext.Module]), the violation disappears.
  • โŒ Adding a boundary on Blah.Context.Subcontext and exporting all does not help.
    • โŒ If I export Module in Subcontext, the violation is still present which surprises me since exporting Subcontext.Module from Context did work.

I've demonstrated this in this repo where if you compile on main, you'll see the violation.

warning: forbidden reference to Blah.Context.Subcontext.Module
  (references from BlahWeb to Blah.Context are not allowed)
  lib/blah_web/controllers/page_html/home.html.heex:1

Any thoughts / help would be greatly appreciated ๐Ÿ™. I'm not sure if I've just missed how this case should be idiomatically handled, or if this is a bug, or not a feature at all.

Thank you

aspett commented

Looks like this duplicates #55

aspett commented

In this particular case, I am adding Boundary to an existing application where we'll iterate away some dirty xrefs. Unfortunately it means I can't promote this particular subboundary to top level because that'll introduce a circular dependency - so seems with the current boundary implementation, it's necessary to explicitly export these modules for aunt/uncle dependents.

Thanks for the report. Currently, exports: :all doesn't include sub-boundaries. I don't think supporting this would be very hard, but I'm not sure if this is something we want to do.

TBH, exports; :all should IMO mostly be avoided. It was originally introduced for the purposes of Ecto schemas, which are the modules you usually want to export, and there's typically a lot of them, so listing them all explicitly is tedious. But otherwise I'd mostly avoid this approach because it effectively removes the private modules features. Everything in the boundary becomes public.

If you have a mixture of public and private modules (which I presume is the case, given that you also have sub-boundaries), then I think explicit exports are a better approach.

So my first question is what's wrong with explicitly listing exports? Are there too many of them?

aspett commented

In this use case, there are many subcontexts, and also many modules in this particular Subcontext which make sense to be exported as part of a migration towards cleaner boundaries. I had actually discovered this issue when trying to export like,

use Boundary,
  deps: [...],
  export: [
    # ...,
    {Subcontext, []}
  ]

There are quite a few modules, but not so many that it's unfeasible to list them explicitly ๐Ÿ˜

I agree that exports: :all is ideally avoided; this is a struggle of migrating an existing codebase only.

Perhaps I could look into implementing this as an option, but not default?

Adding our use case here as we also have hit this limitation in the past.
In our case, we have a main boundary in a module that defines the main API.
This module then exposes 2 other modules that define the interfaces (inputs/outputs) for this API.
So we end up with something like:

defmodule API do
    use Boundary
end

defmodule API.Inputs do
  alias API.Inputs.<some sub module>
  ...
end

defmodule API.Outputs do
  alias API.Outputs.<some sub module>
  ....
end

To export Outputs/Inputs, we tried to use mass exports in the top API boundary but that didn't work so we have to either:

a) explicitly list each of the Output/Input's sub-modules in the main boundary
b) make Output/Input into top-level boundaries

Both a) and b) work, but for a) we lose the benefit of having a top-level module name that "encapsulates" the public interfaces and, for b), the module that depends on this boundary needs to explicitly depend on these sub-boundaries.

I had actually discovered this issue when trying to export like,

use Boundary,
  deps: [...],
  export: [
    # ...,
    {Subcontext, []}
  ]

@aspett What didn't work with this? This is supposed to allow you to bubble up all the exports of a sub-boundary.

I agree that exports: :all is ideally avoided; this is a struggle of migrating an existing codebase only.

That's true. I actually use it myself in one place in a large codebase I'm moving to boundary.

Perhaps I could look into implementing this as an option, but not default?

I mean I'm open to this, clearly there's a need. But I'm also worried that if we make an ad hoc choice now, we'll regret it soon. So I'm still trying to grasp the problem, and I'd like us to devise a solid solution.

I understand that you're working on an existing codebase, and so you need to cut some corners. But this begs the question if introducing nested boundaries is a good choice at this point? Maybe instead just configure level 1 boundaries and make that work, and then refine deeper gradually? I feel I don't understand enough about your boundary strategy to make a choice here.

@AfonsoTsukamoto I don't quite understand this. Are Input/Output sub-boundaries? What is it you're trying to export that doesn't work?

aspett commented

I had actually discovered this issue when trying to export like,

use Boundary,
  deps: [...],
  export: [
    # ...,
    {Subcontext, []}
  ]

@aspett What didn't work with this? This is supposed to allow you to bubble up all the exports of a sub-boundary.

The same scenario as the issue described; if Blah exports [{Context, []}] and Blah.Context exports [{Subcontext, []}] and BlahWeb depends on Blah, then I'd hoped that it would not violate boundaries by calling Blah.Context.Subcontext.Module.call/0 from BlahWeb. (This is not the case, it does result in a violation)

So I'm still trying to grasp the problem, and I'd like us to devise a solid solution.

I understand that you're working on an existing codebase, and so you need to cut some corners. But this begs the question if introducing nested boundaries is a good choice at this point? Maybe instead just configure level 1 boundaries and make that work, and then refine deeper gradually? I feel I don't understand enough about your boundary strategy to make a choice here.

There are two components here that I think will help convey the strategy. The details are fudged, but same idea:

  1. We want contexts to talk to each other via clearly documented APIs. Currently contexts own several standardised namespaces - e.g. imagine commands, queries, workers, etc. Those currently can be called from anywhere, despite many of those modules being "internal". The idea for migration is that each context will instead begin to expose it's public interface on the context module (Application.Context1), but there will also be some additional modules (domain records) that will be public, e.g.

    • Application.Context1
      • Application.Context1.Public
        • Application.Context1.Public.DomainObject1
  2. Fully migrating to a model like that will be a slow burn, so the idea is to set up each context to export the namespaces we actually want (i.e. Public) and then list everything else as dirty_xrefs and that list can be driven down over time.

Hopefully that makes sense ๐Ÿ™ˆ. So my contrived example in the original comment actually reflects trying to expose the Public namespace of contexts so that DomainObject can be depended upon between contexts, but also in other top level boundaries (and in their children).

We can do this by explicitly listing modules, but more ideal is to use notation like exports: [{Public, []}]

@AfonsoTsukamoto I don't quite understand this. Are Input/Output sub-boundaries? What is it you're trying to export that doesn't work?

Yes, they are sub-boundaries - I think it is easier to see the issue when scoped in the app so let me update the example.
So, taking the same example but with the top level fixed:

defmodule MyApp do
    # expectation: MyApp mass exports API
   use Boundary, top_level?: true, exports: [{API, {}}]
end

defmodule MyApp.API do
  # expectation: API mass exports Inputs/Outputs
   use Boundary, exports: [{Inputs, []}, {Outputs, []}]
   
   def some_fn, do: :some_fn
end

defmodule MyApp.API.Inputs do 
   def some_fn, do: :some_fn
end

defmodule MyApp.API.Outputs do
   def some_fn, do: :some_fn
end

If I then add a dependent module, I get:

defmodule MyAppWeb do
   use Boundary, top_level?: true, deps: [MyApp]
   
   alias MyApp.API
   
   def test do
      API.some_fn
      API.Inputs.some_fn # forbidden reference to MyApp.API.Inputs
   end
end

In this case, looking from the top level down, I would expect the boundary to:

  • create a boundary on MyApp level that also exports MyApp.API and all its sub-modules
  • MyApp.API redefines the boundary include only MyApp.API.Inputs/Outpus and their sub-modules as sub-modules/boundaries

making the final MyApp boundary something like:
{MyApp, MyApp.API, MyApp.API.Inputs, MyApp.API.Outputs, MyApp.API.Inputs.*, MyApp.API.Outputs.*}

but the final result, at least from a dependent module call, is {MyApp, MyApp.API}

Thank you both. This is definitely contrasting the documentation (https://hexdocs.pm/boundary/Boundary.html#module-exporting-from-sub-boundaries).

On one hand, I'm not a fan of exporting something from a sub-sub-boundary. Not gonna lie, this seem smelly to me. But I also see how the current behaviour seems weird. So I'm also reconsidering caving in, and including sub-boundary exports into :all. The advantage here is that the model is simple to explain. We can basically say that sub-boundary public modules are also treated as the members of the parent boundary. This is already the case for the root module, so now we'd just expand this definition.

Another choice is to fix the mass export, and/or maybe even add the special option, so you can say exports: [{Foo, as: :boundary}].

I need to think about this a bit. I probably won't find much time this week though, as I'm away on a conference.

Thank you for taking the time to look into this @sasa1977 ๐Ÿ™

I agree that it might come as smelly but also that the model becomes easier to explain - in our case, we assumed this is how it worked already.

I'm at the beginning of my Elixir journey but would be happy to help with the implementation of this once you have a decision ๐Ÿ‘