Dependency violation not detected when using public folders for privacy boundary
Enceradeira opened this issue · 9 comments
Description
When I enforce privacy for specific constants, a dependency violation to a public constant in that package is correctly detected. As soon as I reconfigure the package and set enforce_privacy: true
and move the public constant to the package's public folder the dependency violation is not detected anymore.
I expect that the way how I declare public/private constants doesn't influence the dependency check/violations.
To Reproduce
- Example 1 reports correctly a dependency violation (on branch working).
git clone git@github.com:Enceradeira/rails_problem_packwerk.git
bundle install
git checkout working
packwerk check
app/models/accounting/accounts.rb:6:8
Dependency violation: ::Debtors::DebtorsService belongs to 'app/models/debtors', but 'app/models/accounting' does not specify a dependency on 'app/models/debtors'.
Are we missing an abstraction?
Is the code making the reference, and the referenced constant, in the right packages?
Inference details: this is a reference to ::Debtors::DebtorsService which seems to be defined in app/models/debtors/debtors_service.rb.
To receive help interpreting or resolving this error message, see: https://github.com/Shopify/packwerk/blob/main/TROUBLESHOOT.md#Troubleshooting-violations
1 offense detected
No stale violations detected
- Example 2 reports wrongly no violation (on branch not_working)
git checkout not_working
packwerk check
No offenses detected
No stale violations detected
- The difference between Example 1 and Example 2 is only the way how I declare the privacy boundary in
app/models/debtors/package.yml
git diff working not_working | cat
diff --git a/app/models/debtors/package.yml b/app/models/debtors/package.yml
index 619e57e..958af2c 100644
--- a/app/models/debtors/package.yml
+++ b/app/models/debtors/package.yml
@@ -1,5 +1,5 @@
-enforce_privacy:
- - "::Debtors::Enquiries"
+enforce_privacy: true
+
enforce_dependencies: true
dependencies:
- app/models/accounting
\ No newline at end of file
diff --git a/app/models/debtors/debtors_service.rb b/app/models/debtors/public/debtors_service.rb
similarity index 100%
rename from app/models/debtors/debtors_service.rb
rename to app/models/debtors/public/debtors_service.rb
Expected Behaviour
Example 2 should produce the same result as example 1. The way how I define the privacy boundary should not influence which dependency violations are detected.
Version Information
- Packwerk: 2.1.1
- Ruby 2.7.6p203
- Zeitwerk: 2.5.4
- Rails: 7.0.2.3
That seems like a bug to me. Are you able to investigate? Thanks.
Thanks for confirming it as a bug. Yes I will investigate it.
Took a quick look - I'm pretty sure your not_working
example doesn't work at all. If the file path is app/models/debtors/public/debtors_service.rb
, both zeitwerk and packwerk expect it to be namespaced under Public
, so Debtors::Public::DebtorsService
.
That code should raise on your reference to Debtors::DebtorsService
.
From a packwerk perspective, the reference is not a dependency violation because the referenced constant can't be found.
If you want to do something like this, you have a couple of options:
- use the public namespace, as shown above
- define
app/models/debtors/public
as a nested load path (can be a little confusing, but nested load paths are not crazy, seeapp/models/concerns
which is usually a nested load path) - restructure your application to use an engine-like layout (maybe use engines):
components/ # or packages, or another name that makes sense to you
debtors/
app/
public/
models/
controllers/
...
accounting/
app/
public/
models/
controllers/
...
That would make public
a load path, so that the word doesn't need to appear in the namespace path.
We use both the second and third option; top level components, then packages with their own load paths nested inside of them. Load paths are set up with Rails Engines. However, the simplest solution is to just use Public
as a namespace.
Not a bug.
Thank's for the thorough analysis. My not_working
example is correctly loaded by zeitwerk but it's using the following configuration for zeitwerk:
# config/initializers/zeitwerk.rb
Rails.autoloaders.main.collapse("#{Rails.root}/app/**/public")
That is why the constants in the public folder are loaded correctly even they are not in the Public
namespace.
@exterm: Is the above configuration of zeitwerk therefore incompatible with packwerk? If yes, is it a missing feature or just not the way how packwerk is indented to work?
collapse
is indeed not currently supported, AFAIR. We should probably document this somewhere. However, maybe #186 would implicitly add support for collapse
too?
collapse
didn't exist when we originally developed packwerk, and we never added support for it - there's no specific reason other than that we didn't need it, and this is the first time it's come up as far as I know.
collapse
public folders is useful for legacy apps. With it you are able to define a folder as a package, make everything in it private and then just move the public constants into the public folder without changing the code where they are referenced. I will see if configuring privacy for specific constants is therefore a better option for now.