[almighty] Potential cyclic import issue in core.

[Thomas Mäder]

Hi Pranav,

I think your question touches upon an unresolved problem we have with 
our system layering. We have discussed this before when I wrote the 
first repository interfaces, but haven't really resolved the issue at 
the time. Let's look at what kinds of code we have:

  * Repository interfaces
    They define an interface for persistence in terms of application
    level objects. The repository interfaces are not supposed to be gorm
    specific. The idea is to be able to replace the implementation in
    tests and to avoid coupling consumers to gorm.
  * Repository implementations
    They implement the repository implementations in a gorm-specific
    way. They are free to convert app layer objects to a different form.
    For example, we store work items as JSON in the db, which only has
    very few data types (number, string, bool, etc.). The repository
    implementations convert and validate the work item fields according
    to a given work item type.
  * Gorm support stuff
    Stuff like models.Lifecycle, GormTransactionSupport, etc. This is
    shared infrastructure for implementing repositories.

Now that we have an overview, what problems can we run into and how do 
we solve them?

  * Problem 1: shared repository infrastructure in models package
    Solution: -> move infrastructure to a separate package
  * Problem 2: false cycle
    This problem can occur if we have multiple repositories in the same
    package and have pairwise inverse dependencies: Think of two
    packages foo and bar. We have repository foo.X depend on bar.Y and
    repository bar.M depend on foo.N. Now we have a package cycle, but
    it is not a REAL cycle in the code, we just co-located two
    independent repositories in the same package
    Solution: -> separate repositories into different packages
  * Problem 3: real cycle
    In real life, there ARE cases where cyclic dependencies make sense.
    Imaging an event service and a logging service: the event service
    wants to log things, and the logging service wants to send events.
    You usually need some kind of setup protocol to get things going,
    but it's possible. The problem in our case is that the compiler will
    complain if the two repositories are in separate packages.
    Solution: -> move the two repository interfaces to a common package
    and only depend on the interface of the other repository in the
    implementation.

/Thomas


On 10/08/2016 05:25 AM, Pranav Gore wrote:
> ​Hi Team Core,
>
> *Context/Background:-*
> I am working on #244 
> <https://github.com/almighty/almighty-core/issues/244>. The task is to 
> do `fields["system.creator"]=currentLoggedInUser`.
>
> There, I want to call a method Locate from package "Token" which 
> returns current logged in user.
> While doing so I am facing cyclic import problem.
>
> *Reason:-* Work_item_repo is in package "models", it wants to call 
> Locate method from package "token".
> But "token" pachakge imports "account" package and "account" package 
> imports "models".
> Hence "models" package is unable to import "token" => cyclic dependency.
>
> *Debugging:-* When I was looking into "account" package, what exactly 
> it needs from "models" package is ONLY "models.LifeCycle" and nothing 
> else as of now.
>
> *Solutions:-* One quick possible/feasible solution is to put 
> models.LifeCycle into different new package of its own, refactor and 
> move on (IMO we should do this as first step).
>
> Another is to write a middleware(as mentioned in #244 
> <https://github.com/almighty/almighty-core/issues/244>) for injecting 
> loggedInUser which will be in package "middleware" and avoid cyclic 
> import,
>
> Or redesigning to decouple models package into smaller chunks ?
>
> PFA- I have attached output of `go list -f` on account, models, token 
> packages.
>
> Any input on this will be helpful :)
>
>
> Thanks,
> Pranav

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/almighty-public/attachments/20161010/362114c5/attachment.htm>