[Pulp-dev] avoiding circular imports in Pulp 3 plugins

[Michael Hrivnak]

Following an in-person discussion, I wrote up this issue to reflect what we
concluded:

https://pulp.plan.io/issues/3074

Please comment there with questions, thoughts and ideas. I'll be mostly out
of communication the rest of this week, so feel free to improve and groom
it without me.

Thanks!

On Fri, Sep 8, 2017 at 4:42 PM, Michael Hrivnak <mhrivnak at redhat.com> wrote:

> While working with the FileImporter, I discovered an interesting problem
> with circular imports.
>
> An Importer is both a django Model and has a "sync" method, as shown here:
>
> https://github.com/pulp/pulp_file/blob/d684f75e/pulp_file/
> app/models.py#L69
>
> We decided to design it this way for simplicity, so that a plugin writer
> would have a minimum of objects to subclass. It also reduces the number of
> objects for the core to discover. The Importer would have methods for sync,
> copy, upload, etc. This is very similar to Pulp 2. And just like in Pulp 2,
> we know that plugins will often want to implement much of the sync logic in
> one or more separate python modules.
>
> When I tried to move the FileImporter's sync code to a separate module, I
> quickly found a circular import problem. The FileImporter wants to
> instantiate and use a Synchronizer. The Synchronizer of course needs to use
> the data model, both the FileContent model and the corresponding Key
> namedtuple. A plugin with more types would have yet more models.
>
> Thinking of the idea that circular imports often indicate a design
> problem, there is a single and specific design choice throwing a wrench in
> the gears. We have a single class acting as both model and controller: the
> FileImporter. It both defines part of the data model, AND it is expected to
> perform an action (sync) that makes use of several different models
> (classic controller territory, and definitely not in-line with OOP).
>
> We made the choice deliberately to put the plugin API methods (sync,
> upload, etc.) on the Importer to give the plugin writer a simple
> experience. But here we see that is already causing difficulty for plugin
> writing.
>
> Perhaps we should re-consider having the Importer both act as model and
> controller. I'm using those terms loosely, not to imply that we should have
> a strict MVC pattern or anything like that, but just to describe what type
> of role any given portion of code is expected to play.
>
> Options
> ----------
>
> We could recommend something akin to dependency injection. When the sync
> method on an Importer calls some other code, it would need to pass all
> other model classes needed by that code as arguments. It's fairly simple,
> but maybe more awkward of a pattern than we want to impose on plugin
> writers.
>
> We could consider the Importer model subclasses to only be a way to store
> configuration, and keep sync/copy/etc code separate. This would require us
> to provide a way to make those other things discoverable. It's a clean
> pattern that would do a much better job of following OOP, and it aligns
> with best practices of creating django model classes, but we'd need to make
> the discovery aspect easy.
>
> We could make the plugin writer do lazy model loading in their sync code.
> That seems like a pain though.
>
> https://docs.djangoproject.com/en/1.11/ref/applications/#
> django.apps.apps.get_model
>
> Of those three, I like the second the most. It's a familiar programming
> pattern, and I think we could make the discovery very easy.
>
> Have other ideas, or other opinions/variation on the above ideas?
>
> --
>
> Michael Hrivnak
>
> Principal Software Engineer, RHCE
>
> Red Hat
>



-- 

Michael Hrivnak

Principal Software Engineer, RHCE

Red Hat
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20171016/1a93ea51/attachment.htm>