Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow imports not specified through __all__ #240

Open
charettes opened this issue Oct 14, 2024 · 3 comments
Open

Disallow imports not specified through __all__ #240

charettes opened this issue Oct 14, 2024 · 3 comments

Comments

@charettes
Copy link

👋 there, thanks for that awesome project!

We've been using import-linter to define interfaces between packages in our application but one thing that keeps coming is preventing re-exports from violating our contracts.

For example, say that we allow module foo to only import from bar.interface (and nothing else in bar.*) when we define bar.interface we might imports a few things from bar.* to define the interface.

We'd like for a way to prevent imports from bar.* in bar.interface from being re-exported and we were hoping we could use bar.interface.__all__ for that through some flags at least instead of doing function level imports in bar.interface.

It might be easier to reason about this with code. Today if we want to prevent re-exports of bar.* in bar.interface we have to do

# bar.interface

def update_bar(bar_id: int, updates: dict) -> bool:
    from bar.models import Bar
    return Bar.objects.get(bar_id).update(**updates)

As otherwise doing

# bar.interface
from bar.models import Bar

def update_bar(bar_id: int, updates: dict) -> bool:
    return Bar.objects.get(bar_id).update(**updates)

re-exports Bar as bar.interface.Bar. We'd like to be able to do

# bar.interface
from bar.models import Bar

__all__ = ('update_bar',)

def update_bar(bar_id: int, updates: dict) -> bool:
    return Bar.objects.get(bar_id).update(**updates)

and have import linter prevent bar.interface.Bar imports.

Thank you for your consideration!

@seddonym
Copy link
Owner

Interesting one this, thanks for raising. It's actually something we're also grappling with at work at the moment - broadly speaking I would describe this as wanting to achieve encapsulation - that is, a separation between the public interface and the private internals of a given subpackage. And it looks like you're wanting to make Django models a private concern - again, that's something we're working on.

In terms of your specific feature request, I suspect adding support for this in Import Linter would be a substantial amount of work. Currently it relies on building a directed graph of all the imports between modules. Supporting your use case would require more granularity about the objects exposed in each module.

I wonder if this might be relatively straightforward to lint for without building an import graph - perhaps a regular expression might be enough, as the concern is quite localized. Or you could write a flake8 plugin, or request an addition to Ruff.

Another option would be to import the model with a private alias, like this?

# bar.interface
from bar.models import Bar as _Bar

__all__ = ('update_bar',)

def update_bar(bar_id: int, updates: dict) -> bool:
    return _Bar.objects.get(bar_id).update(**updates)

At the very least, that will give people second thoughts before importing bar.interface._Bar, and you could probably use a linter already available to enforce that, such as this one from ruff.

Incidentally, the convention we currently follow is to prefix private modules with an underscore. So you could move to a structure like this:

bar/
   __init__.py
   interface.py
   _models.py

Of course, that would break Django's autodiscovery of models - because really, models have to be public as far as Django is concerned. But there's probably a way around it, such as running an autodiscovery of _models modules during application bootstrap.

I anticipate that Import Linter will support enforcing the rules based on underscores in the not too distant future.

Another tactic could be to use the Repository Pattern, and structure your application so that the implementation of the repositories is in a layer above the code that interacts with the repository abstraction. That's something Import Linter currently supports (via the layers contract) but I imagine that would be a big rearchitecting job!

Let me know how you get on!

@charettes
Copy link
Author

Thanks for the very detailed answer @seddonym, greatly appreciated!

In terms of your specific feature request, I suspect adding support for this in Import Linter would be a substantial amount of work. Currently it relies on building a directed graph of all the imports between modules. Supporting your use case would require more granularity about the objects exposed in each module.

That unfortunately the conclusion we reached as well. We were hoping there was a way to have the graph built with objects as nodes instead of modules but that's not possible.

Another option would be to import the model with a private alias, like this?

We also considered this pattern and encouraged its usage but alas without enforcement we always ended up with unintended violations. What I mean is that it's not so much the importing of _private that poses a linting challenge but more the enforcement of import something as _something.

I guess there might be a combination of linters out there that might make our life easier so I'll keep looking.

I anticipate that Import Linter will support enforcing the rules based on underscores in the not too distant future.

That's great news!

Another tactic could be to use the Repository Pattern, and structure your application so that the implementation of the repositories is in a layer above the code that interacts with the repository abstraction. That's something Import Linter currently supports (via the layers contract) but I imagine that would be a big rearchitecting job!

We actually implemented a similar concept that we called interfacing contract paired with CODEOWNERS.

Interfacing contracts are defined with three configurations

  • covering a module that constitutes the interface
  • interfaces submodules of covering from which imports are allowed from
  • ignore_imports allowed violations of the interface rules (we try to minimize these as we tidy up our code base)

So something like

[auth]
covering = auth
interfaces =
    auth.interface
    auth.exceptions
    auth.types
ignore_imports =
    foo.bar -> auth.private # known violation

In order to ensure that only code owners of can allow or deny changes to contracts covering verticals they own we also added a way to have the top level .importlinter file include other contracts which has helped teams take control (prevent internal leakages or build interface)

So we basically we have a top level .importlinter file with references to multiple interface.ini files defining contracts owned by the many teams developing in our monolith.

@seddonym
Copy link
Owner

That concept of an interfacing contract is cool! I think something like that might work well as a built in contract for Import Linter (or could be implemented as a separate library, since Import Linter supports pluggable contract types).

we also added a way to have the top level .importlinter file include other contracts

Tell me more! Currently you have to put all the contracts in the same file to use the same built graph - it would be nice to be able to split the contracts up into different files for reasons of maintainability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants