Add a protected contract type #266

abeaufays · 2025-06-12T22:16:21Z

Add a new contract type to protect modules or packages from un-allowed imports

abeaufays · 2025-06-25T21:17:10Z

Looking at #231, it could make sense to make external packages protected as well to solve this specific part of the issue. Maybe not the first version ?
I want the app module to be the only module that can import "app" dependencies (i.e. fastapi), my cli module should be the only one capable of importing "cli" dependencies (i.e. click).

seddonym · 2025-07-10T15:08:44Z

Looking at #231, it could make sense to make external packages protected as well

It's possible this might just work with external packages as-is. Worth adding a test case to see!

seddonym

This is looking great so far.

I think it is worth improving how we report failed contracts, other than that hopefully most of my other comments are small tweaks.

Happy to discuss!

src/importlinter/contracts/protected.py

seddonym · 2025-07-10T14:47:11Z

src/importlinter/contracts/protected.py

+        illegal_imports = check.metadata["illegal_imports"]
+
+        output.print_error(
+            "Following imports do not respect the protected policy:",


I think we should improve the broken contract message. Taking inspiration from the layers contract, it might look something like this:

mypackage.green is not allowed to import mypackage.blue directly: - mypackage.green.one -> mypackage.blue.two (l.6) - mypackage.green.three -> mypackage.blue.four (l.12, 34) - mypackage.green.five -> mypackage.blue.six (l. 4) mypackage.yellow is not allowed to import mypackage.blue directly: - mypackage.yellow.one -> mypackage.blue.two (l.16)

This organisation makes sense with as_package=True only ?
Because I believe overlaps should be valid, I think it will be difficult to group reports by lines that triggered it 🤔, but it would be useful still.

This organisation makes sense with as_package=True only ?

It's true that it has less value when as_packages is False, but maybe for consistency it's still worth presenting like that. The principle is that in the summary line ("x is not allowed to import y") we refer to the items listed in the contract, and then below we list the specific imports.

I tried something in this commit

It would display error like this:

Following imports do not respect the protected policy: mypackage.foo.protected rule is imported by unallowed modules: - mypackage.bar.other_package.one -> mypackage.foo.protected.models (l.7)

Thanks for taking a look. For me, the wording a bit too verbose - also I think it's worth making the reporting style more consistent with other contracts, including whitespace. Could we output the error in this form?

mypackage.green is not allowed to import mypackage.blue directly: - mypackage.green.one -> mypackage.blue.two (l.6) - mypackage.green.three -> mypackage.blue.four (l.12, 34) - mypackage.green.five -> mypackage.blue.six (l. 4) mypackage.yellow is not allowed to import mypackage.blue directly: - mypackage.yellow.one -> mypackage.blue.two (l.16)

(Also let's drop the "Following imports do not respect the protected policy:" line, that should be obvious from the context.)

As agreed, we will go with this version of the broken contract rendering:

[importlinter:contract:models-can-only-be-imported-by-colors] name = Models can only be imported by colors direct descendant type = protected protected_modules = mypackage.**.models mypackage.**.data mypackage.brown allowed_importers = mypackage.colors.* as_packages = True Models can only be imported by colors direct descendant ------------------------------------------------------- Illegal imports of protected package mypackage.blue.models (via mypackage.**.models expression): - mypackage.green.one -> mypackage.blue.models (l.6) - mypackage.green.three -> mypackage.blue.models (l.12, 34) - mypackage.green.five -> mypackage.blue.models.alpha (l. 4) Illegal imports of protected package mypackage.orange.models (via mypackage.**.models expression): - mypackage.yellow.one -> mypackage.orange.models (l.16) Illegal imports of protected package mypackage.orange.data (via mypackage.**.data expression): - mypackage.yellow.one -> mypackage.orange.data (l.16) Illegal imports of protected package mypackage.brown: - mypackage.yellow.one -> mypackage.brown (l.16) - mypackage.yellow.one -> mypackage.brown.alpha (l.16)

src/importlinter/contracts/protected.py

docs/contract_types.rst

seddonym

Thanks for the changes!

It's nearly there but I do think we should improve the broken contract message before merging.

src/importlinter/contracts/protected.py

seddonym · 2025-07-14T11:18:01Z

tests/unit/contracts/test_protected.py

+        )
+
+        contract_check = contract.check(graph=graph, verbose=False)
+        assert not contract_check.kept


I think it would be worth asserting on the value of check.metadata["illegal_imports"] here, otherwise we don't have test coverage for that.

I agree but I think we should review the way to write these tests then 🤔
Since we only add one import to the graph we're almost sure it will be the one, phrased differently we could make these tests pass with some logic that puts all the imports in "illegal_import" if the contract is not kept.
Maybe if I add some valid imports to the build graph + a test to check that the base imports let keep the contract ? 🤔

Either way sounds fine - I think the main thing is that the unit test for rendering assumes that data is going to be stored in the metadata in a certain way, so we should also have a unit test that checks that the metadata is populated in that way. Does that make sense?

docs/contract_types.rst

abeaufays · 2025-07-15T13:46:29Z

Looking at #231, it could make sense to make external packages protected as well

It's possible this might just work with external packages as-is. Worth adding a test case to see!

@seddonym
I added a test case but it doesn't work with as_squashed=True, I suspect this is how external dependencies are handled in a real run no ?
(See 44093f0 )
(Next thing on my list is the rendering of broken contract 🙏)

seddonym · 2025-07-15T14:29:09Z

I added a test case but it doesn't work with as_squashed=True, I suspect this is how external dependencies are handled in a real run no ?

Yes that's correct (if you're interested, the logic is in Grimp here).

I think it we probably need to external packages too, or at least document what isn't supported about them. I'm surprised they don't work though. Maybe a good way forward is to include an xfailing test in this PR, so I can see what you feel isn't working.

seddonym · 2025-09-12T15:34:13Z

src/importlinter/contracts/protected.py

+                )
+
+                protected_modules = {top_level_protected_module}
+                if self.as_packages:


This should be if self.as_packages and not graph.is_module_squashed(top_level_protected_module):.

That's because external packages will be squashed, so they only appear as a single node in the graph.

That will sort the tests out!

seddonym · 2025-09-12T15:40:31Z

tests/unit/contracts/test_protected.py

+                True,
+                "Allowed module can import external protected module",
+            ),
+            (


This test case should be removed, as django.core would never appear in the graph as an external package. (Just django.)

seddonym

This is looking great! I've already tried it on a real life code base and it's going to be really helpful.

I see what the problem is with the external packages, it's a quick fix. There's a few other tweaks I'd like to make, so in the interests of not too much more back and forth, I'm going to merge into a branch and I'll make the tweaks there.

Thanks for your contribution, this is a really exciting one.

abeaufays force-pushed the add-protected-contract-type branch 5 times, most recently from bf35329 to 39125b4 Compare June 18, 2025 19:51

abeaufays marked this pull request as ready for review June 18, 2025 19:54

abeaufays force-pushed the add-protected-contract-type branch from 39125b4 to 46312ca Compare June 18, 2025 19:58

seddonym mentioned this pull request Jun 19, 2025

Implement module expression optional fragments python-grimp/grimp#228

Closed

abeaufays force-pushed the add-protected-contract-type branch 4 times, most recently from 60b1861 to 2aec196 Compare June 26, 2025 21:25

seddonym self-requested a review July 10, 2025 15:09

seddonym requested changes Jul 10, 2025

View reviewed changes

abeaufays mentioned this pull request Jul 11, 2025

Make forbidden contract docstring consistent with other contracts #271

Merged

abeaufays force-pushed the add-protected-contract-type branch 2 times, most recently from aef3d96 to efd1d13 Compare July 12, 2025 11:54

abeaufays mentioned this pull request Jul 12, 2025

Make module expressions to modules package aware #273

Merged

seddonym requested changes Jul 14, 2025

View reviewed changes

abeaufays force-pushed the add-protected-contract-type branch from dd0a6b3 to 5529e37 Compare July 15, 2025 13:11

abeaufays force-pushed the add-protected-contract-type branch 3 times, most recently from 24d05e8 to 100161c Compare July 16, 2025 21:43

abeaufays force-pushed the add-protected-contract-type branch 3 times, most recently from f01cf5e to 16d2257 Compare September 11, 2025 17:53

abeaufays force-pushed the add-protected-contract-type branch from 16d2257 to 99dee67 Compare September 11, 2025 17:56

abeaufays and others added 4 commits September 11, 2025 20:01

Add a protected contract type

d69dca0

Add documentation for the protected contract type

1d46224

Fill CHANGELOG.rst and AUTHORS.rst

9c7398a

Add a test to show that external_packages cannot be protected yet

b05aa7c

abeaufays force-pushed the add-protected-contract-type branch from 99dee67 to b05aa7c Compare September 11, 2025 18:01

abeaufays requested a review from seddonym September 11, 2025 18:02

seddonym changed the base branch from main to protected-contract September 12, 2025 15:09

seddonym reviewed Sep 12, 2025

View reviewed changes

seddonym approved these changes Sep 12, 2025

View reviewed changes

seddonym merged commit 3d25f61 into seddonym:protected-contract Sep 12, 2025
5 checks passed

seddonym mentioned this pull request Sep 12, 2025

Protected contract #285

Merged

Add a protected contract type #266

Add a protected contract type #266

Uh oh!

Conversation

abeaufays commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abeaufays commented Jun 25, 2025

Uh oh!

seddonym commented Jul 10, 2025

Uh oh!

seddonym left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seddonym Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seddonym Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seddonym left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abeaufays commented Jul 15, 2025

Uh oh!

seddonym commented Jul 15, 2025

Uh oh!

seddonym Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seddonym left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

abeaufays commented Jun 12, 2025 •

edited

Loading

seddonym Jul 10, 2025 •

edited

Loading

seddonym Jul 17, 2025 •

edited

Loading

seddonym Sep 12, 2025 •

edited

Loading