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

[Michael Hrivnak]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20170908/0c0f603e/attachment.htm>