xmolecules / jmolecules-integrations

Technology integration for jMolecules
Apache License 2.0
82 stars 22 forks source link

Invalid violation: an entity cannot have a reference to an aggregate root #259

Closed xenoterracide closed 1 week ago

xenoterracide commented 1 week ago

I'm confused about this error and the documentation seems wrong

 Field c.x.m.s.IdentityProviderUser.user refers to an aggregate root (c.x.m.s.User). Rather use an identifier reference or Association!

IdentityProviderUser is an Entity and implements that interface, its AggregateRoot is User this is not an external relationship with another aggregate. DDD allows for this design and in fact aggregate roots are expected to composed of many entities and value objects in many circumstances.

Association/Ids are for aggregate to aggregate relationships.


update:

actually I think this is some kind of false positive

@jakarta.persistence.Entity
@org.jmolecules.ddd.annotation.Entity
@Table(name = "identity_provider_users")
public class IdentityProviderUser implements Entity<User, IdentityProviderUser.@NonNull IdentityProviderUserId> {

  private @NonNull IdentityProviderUserId id;
  private @Nullable User user;

  /**
   * For JPA.
   */
  protected IdentityProviderUser() {}

  /**
   * Create a new instance.
   *
   * @param id
   *   the primary key
   */
  IdentityProviderUser(@NonNull IdentityProviderUserId id) {
    this.id = id;
  }

  boolean hasUser() {
    return this.user != null;
  }

  @Identity
  @EmbeddedId
  @NotNull
  @Override
  public @NonNull IdentityProviderUserId getId() {
    return this.id;
  }

  @Initializer
  void setId(@NonNull IdentityProviderUserId id) {
    this.id = id;
  }

  /**
   * Get the user.
   *
   * @return the user
   * @implNote this method can throw {@link NullPointerException} if this object is not properly initialized. Please
   *   look along initialization paths for the real issue.
   */
  @MapsId("userId")
  @NotNull
  @ManyToOne(
    optional = false,
    fetch = FetchType.LAZY,
    cascade = { CascadeType.DETACH, CascadeType.MERGE, CascadeType.PERSIST, CascadeType.REFRESH }
  )
  @JoinColumn(
    nullable = false,
    updatable = false,
    name = "user_id",
    foreignKey = @ForeignKey(ConstraintMode.CONSTRAINT)
  )
  public @NonNull User getUser() {
    return Objects.requireNonNull(this.user);
  }

  @Initializer
  void setUser(@NonNull User user) {
    this.user = user;
  }

but according to your source it shouldn't be doing this because IdentityProviderUser is not an AggregateRoot

     * An {@link ArchRule} that ensures that one {@link AggregateRoot} does not reference another via the remote
     * AggregateRoot type but rather via their identifier type or an explicit {@link Association} type.
     * <p />
     *

it doesn't look like this code is checking that both sides are aggregate roots

https://github.com/xmolecules/jmolecules-integrations/blob/main/jmolecules-archunit/src/main/java/org/jmolecules/archunit/JMoleculesDddRules.java#L150-L158

odrotbohm commented 1 week ago

IdentityProviderUser is an Entity and implements that interface, its AggregateRoot is User this is not an external relationship with another aggregate. DDD allows for this design and in fact aggregate roots are expected to composed of many entities and value objects in many circumstances.

Association/Ids are for aggregate to aggregate relationships.

I disagree. Every entity is part of an aggregate (you declare User as the owning artifact here). This means that your reference to User is and aggregate to aggregate relationship, rightfully rejected as invalid. It does not make sense to reject the relationship from User to User, but not from an entity owned by User towards User. If that was allowed, one could work around the rule, by moving aggregate relationships into nested entities.

xenoterracide commented 1 week ago

How is a parent relationship an aggregate to aggregate?

class Node implements Entity<Root> {
   Root parent;
}
class Root implementats AggregateRoot {
   Node child;
}

you're saying this create an aggregate to aggregate relationship by virtue of having a parent where we've identified that it is the parent type? that doesn't make sense per DDD, there's no good reason a child can't have a reference to its parent. Do you have a reference quote where any of the authors say a child cant have a reference to its parent? should I dig up the converse (will take me a minute)?

I want to say that I think it would be reasonable to add a @Parent annotation to allow this as more explicit while retaining some level of checking.

xenoterracide commented 1 week ago

Also, I think I have to have it due to some nonsense without the Embeddable compound id works in hibernate. Don't quote me 100%, but I recall an issue, I'd have to try removing it and I don't have time right this second. Something to do with needing @MapsId("userId") but maybe the side effect went the other direction. I just recall it being weird and annoying to get the opaque compound id working and it has a few gotchas.

https://github.com/xenoterracide/spring-app-commons/blob/main/security-model/src/main/java/com/xenoterracide/model/security/IdentityProviderUser.java

odrotbohm commented 1 week ago

How is Node not part of the Root aggregate? How is the parent property not a reference to the Root aggregate?

It looks like you conflate aggregate with aggregate root. The former is the entire arrangement including all nested entities and values. References from those towards those must not be full aggregate references as described here.

I guess what you argue is that because the reference targets the same aggregate root, the property should be somehow treated differently. But why? Such an arrangement would still be subject to the same problems that are valid for aggregate references of other types as well.

xenoterracide commented 1 week ago

How is Node not part of the Root aggregate? How is the parent property not a reference to the Root aggregate?

it is, that's kind of my point, and aggregate, can reference itself internally, it is treated externally as 1 thing. So 2 different aggregates should not hold references to each other, but within a single aggregate it shouldn't matter. You aren't checking that an Entity can hold a reference to another Entity, right? that doesn't make sense.

The ONLY time this should be allowed is when an entity references it's OWN aggregate.

OtherRoot and any entities within it should not have a reference to Root or any entities within it.

A Root with Id 1 should not have a reference to a Root with Id 2, and Node with parent Id 1 should not have a reference to a Root with Id 2. Problem with this scenario you can only validate it at runtime. I think this is the problem you're trying to enforce not having.

But why? Such an arrangement would still be subject to the same problems that are valid for aggregate references of other types as well.

what problems do you think those are exactly? do you not think objects

I'll read the link later, I have to leave shortly for an operation (no it's not serious). I think somewhere you're not understanding me. I'm looking through the blue/red books for an example.

xenoterracide commented 1 week ago

I have skimmed the link, but even the title says it all

Rule: Reference Other Aggregates by Identity

Since Aggregates don’t use direct references to other Aggregates but reference by identity, their persistent state can be moved around to reach large scale.

The keyword here is Other, this all happens within the same aggregate, there is no other aggregate here. The entity is part of the same aggregate and is maintaining a reference to its own aggregate root, there's nothing wrong with that.

Root and Node can belong to the same aggregate. IdentityProviderUser is part of the User aggregate.

I see no disagreement with what the intent here is, but the assertion doesn't match the description; nor as per your reference what the DDD restriction or suggestion, is.

Also consider, what happens if the backend uses something like Neo4j where a graph database being chosen for storage probably means considering nodal traversal and probably frequently bidirectional relationships.

odrotbohm commented 1 week ago

I still think you're trying to make an exception for your special case (a nested entity trying to keep a reference to its owning aggregate instance). That said, the type system cannot reasonably understand whether the reference to Root refers to the same instance or a different root (employee-manager-relationship, that would still be a considered a parent-child-one).

The problem is special in a second aspect, as the back-reference is not really needed. State transitions are triggered on the aggregate root anyway, which could then pass the owning instance to the nested entity should any of the operations on that need access to it.

xenoterracide commented 1 week ago

I don't agree because any entity within the aggregate should be able to hold reference to any other entity within the aggregate to do its work. the root is not special in this. I think you're trying to make up rules that no one has ever stated. Your own evidence did not provide such a statement.

xenoterracide commented 4 days ago

BTW Vaughn himself says it's ok to have entities maintain references to their aggregate root. https://x.com/VaughnVernon/status/1838222241341083840

odrotbohm commented 4 days ago

“…, but why?”

xenoterracide commented 3 days ago

you can read the whole thread... stop being selective about things you choose to read.

where I explain the same thing I did here in less words, and some suggested approach's. The why comes around to the other issue I reported elsewhere which is to have the Opaque UserId instead of passing around things like UUID... it's possible there's another way of doing this. This entire "Entity" is basically to represent a foreign aggregate, and so is essentially an association itself. That doesn't really remove hibernate gotcha's.

anyways, I'm still think that the solution should be association if it refers to an external aggregate. I won't argue past this though, I just wanted you to know I reached out and it seems my suspicions are verified. Have a good one!