[almighty] Potential cyclic import issue in core.

[Pranav Gore]

Thanks Konrad and Thomas for your time and inputs. I will create a Github
issue for tracking the changes made regarding layering.

@Thomas, thank you for elaborating the content with problem-solution. When
we say a system-layer., is it a bunch of related packages ? Or a layer is a
package.

>From the content above, what I understand is, we should have three main
system-related packages.
1. Interfaces (can import from infra)
2. Impl (Implementing interfaces from above package, should import from
interfaces)
3. Infra (All infrastructure related stuff e.g> Lifecycle,
GormTransactionSupport, must not import from interfaces/impl. In case, it
needs to import from interface, then we need to think is it really
infrastructure item)

4. Other business related packages e.g> remoteworkitem, account, login etc.

As a first step I will move Lifecycle to infra package (outside models).

@Thomas, is this (#110)
<https://github.com/almighty/almighty-core/issues/110> existing
architecture issue for the same ? If yes, I will update the same.



Thanks,
Pranav.


On Mon, Oct 10, 2016 at 3:03 PM, Thomas Mäder <tmader at redhat.com> wrote:

> 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/20161012/d244ad6b/attachment.htm>