Skip to content

Pyreverse: composition / aggregation arrow strange behavior (and field annotation bug) #9045

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

Open
nickdrozd opened this issue Sep 18, 2023 · 7 comments
Labels
Bug 🪲 Discussion 🤔 pyreverse Related to pyreverse component

Comments

@nickdrozd
Copy link
Contributor

nickdrozd commented Sep 18, 2023

I came across a diagram that seemed to have too many arrows and too many kinds of arrows. The code is something like this:

class P:
    pass

class A:
    x: P

class B:
    def __init__(self, x: P):
        self.x = x

class C:
    x: P

    def __init__(self, x: P):
        self.x = x

Classes A, B, and C each have the same relationship to P, namely that they each have a field of that type. So it seems to me that they should all look similar in the class diagram.

But instead, they look like this:

classes

Class A has a "composition" arrow (black diamond), class B has an "aggregation" arrow (white diamond), and class C has both.

Is this correct from a UML perspective? To me this output is unexpected and undesirable. I've tried reading about it, but everything I can find about "aggregation vs composition" turns into word soup and I can't make sense of it. Plus all the examples are from Java, with fine distinctions that don't seem to apply in Python. (This SO page is the best discussion I've been able to find.)

On top of all that, the x field is unannotated in all of A, B, and C, when it should say x: P.

### Tasks
### Tasks
- [ ] Multiple arrows
- [ ] Aggregation vs composition arrows
- [ ] Missing field annotations
@Pierre-Sassoulas
Copy link
Member

Nice catch ! I think we should display only the strongest of the two relations for C. In this case I would argue it's composition (i.e. you can't instantiate a C without the contained P, which is stronger than association). But I'm far from an UML expert.

@DudeNr33
Copy link
Collaborator

Not a UML expert either, but as I understand it the example is not composition, so in all of these cases it should show just the aggregation arrow:

Composition implies a relationship where the child cannot exist independent of the parent. Example: House (parent) and Room (child). Rooms don't exist separate to a House.

(from this article)

Composition arrows should only be used if the parent (holding) class instantiates the P itself, which is not the case here.

That being said I agree with Pierre that if there are multiple arrows (for the same attribute as it is denoted on the arrow) we should only show the strongest relation.

@nickdrozd
Copy link
Contributor Author

So the logic is: A -> B is composition if A actually instantiates B, and aggregation otherwise? In examples:

class P:
    pass

class A:
    x: P  # can't tell, so default to aggregation

class B:
    def __init__(self, x: P):
        self.x = x  # not instantiated, so aggregation

class C:
    x: P

    def __init__(self, x: P):
        self.x = x  # not instantiated, so aggregation

class D:
    x: P

    def __init__(self):
        self.x = P()  # instantiated, so composition

class E:
    def __init__(self):
        self.x = P()  # instantiated, so composition

@DudeNr33
Copy link
Collaborator

At least that is my understanding from the article linked above, yes.

@Julfried
Copy link
Contributor

Julfried commented May 6, 2025

@Pierre-Sassoulas I think this can be closed because it was already solved in #9055. What do you think?

@Pierre-Sassoulas
Copy link
Member

Nice catch thank you. Fixed in 3.0.0

@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.0.0 milestone May 6, 2025
@Julfried
Copy link
Contributor

Julfried commented May 7, 2025

@Pierre-Sassoulas I think I missed something here. Actually the differentiation between aggregation and composition is not correct in the tests files according to the article mentioned. Maybe this should be reopened (in a seperate issue)?:

So the logic is: A -> B is composition if A actually instantiates B, and aggregation otherwise? In examples:

class P:
pass

class A:
x: P # can't tell, so default to aggregation

class B:
def init(self, x: P):
self.x = x # not instantiated, so aggregation

class C:
x: P

def __init__(self, x: P):
    self.x = x  # not instantiated, so aggregation

class D:
x: P

def __init__(self):
    self.x = P()  # instantiated, so composition

class E:
def init(self):
self.x = P() # instantiated, so composition

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 3.0.0 milestone May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 Discussion 🤔 pyreverse Related to pyreverse component
Projects
None yet
Development

No branches or pull requests

4 participants