Skip to content
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

Composite Id #625

Open
umk8yw opened this issue Jan 12, 2018 · 21 comments

Comments

Projects
None yet
5 participants
@umk8yw
Copy link

commented Jan 12, 2018

I would be glad to contribute at implementation of a solution for defining multiple fields as an identifier for a business model.

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Jan 12, 2018

Just to summarize, this issue is about making this mapping possible:

class Person {
    @Id String name
    @Id String surname
    @Id LocalDate dob
    ...
}

and id should be equiv to this mapping, which is currently supported

class Person {
   @Id PersonId personId
    ...
}

class PersonId {
 String name
 String surname
 LocalDate dob
}
@umk8yw

This comment has been minimized.

Copy link
Author

commented Jan 15, 2018

I was thinking to change to EntityType with a Set<JaversProperty> instead of the JaversProperty idProperty field and adapt the rest of the functionality.

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Jan 15, 2018

bare Set is not ok in terms of OO modeling but we can create a dedicated class fot that, lets say EntityId and hide this set

@umk8yw

This comment has been minimized.

Copy link
Author

commented Jan 15, 2018

ok, this was my approach into EntityDefinitionBuilder, I've created a dedicated class UniqueKey to store Set<String> propertyNames and I've created method:

public EntityDefinitionBuilder withCompositeId(String... compositeIdPropertyNames) {
    	Validate.argumentsAreNotNull(compositeIdPropertyNames);
    	this.uniqueKey = Optional.of(new UniqueKey(compositeIdPropertyNames));
    	return this;
    }

so when Javers is built to have the possibility to add multiple fields as id

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Jan 16, 2018

OK, now when you have access to javers repo, could you come up with a failing E2E test and push it to a new branch? This test should be a specification for our new Composite Id feature.

umk8yw pushed a commit to umk8yw/javers that referenced this issue Jan 17, 2018

@umk8yw

This comment has been minimized.

Copy link
Author

commented Jan 17, 2018

when I tried to run again the my test, I found another problem at some code added on 8 December:

 private String localIdAsString(Object localId) {
        PrimitiveOrValueType idPropertyType = getIdProperty().getType();
        return idPropertyType.smartToString(localId);
    }

because my id is a ComplexEntity this method fails.
I've done a pull request so you can see exactly what I am talking about, its title is Composite Id #625 - found problem with new method "localIdAsString"

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Jan 17, 2018

so, javers clashes because you have forced JaversType of your Id Property to EntityType:

.registerEntity(entityDefinition(ComplexEntity.class).withIdPropertyName("code").build())
registerEntity(entityDefinition(TenderLocation.class).withIdPropertyName("complexEntity").build()

It's not possible for now, JaversType of an Id Property can be only Primitive or Value.

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Jan 17, 2018

btw, i'm not sure why this kind of example,
I thought that we want to achieve sth like this:

class Person {
    @Id String name
    @Id String surname
    @Id LocalDate dob
    ...
}
@umk8yw

This comment has been minimized.

Copy link
Author

commented Jan 17, 2018

yes, I want to be able to define multiple fields as an id, but I want to be able to do this at javers build, and some of the fields defined as id will not be Primitive or Value.

in the pull request is exactly the example of the way I've imagined it.

Javers javers = JaversBuilder.javers().registerEntity(EntityDefinitionBuilder.entityDefinition(ComplexEntity.class)
                                                .withIdPropertyName("code").build())
				.registerEntity(EntityDefinitionBuilder.entityDefinition(TenderLocation.class)
						.withCompositeId("location", "complexEntity").build())
				.registerEntity(EntityDefinitionBuilder.entityDefinition(TenderAirline.class)
						.withCompositeId("code", "tenderLocation").build())
				.build();
@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Jan 18, 2018

You are talking about 2 different issues, one issue is composite Id, so possibility to point more than one Id property for an Entity (with both ways, annotations and javersBuilder). That's what we agreed on.
Second issue is possibility to use Entity as Id property, and that's what I don't want in javers because it brakes the rule that Id property can be only Primitive or Value (see https://javers.org/documentation/domain-configuration/#entity-id). I don't want to change this rule, moreover, I think that this is not good idea in terms of domain model. Why to put one Entity as an Id of another Entity?

@umk8yw

This comment has been minimized.

Copy link
Author

commented Jan 19, 2018

ok, then what solution do you see for this kind of situation?
I've chosen this approach because I have 3 entities which all have the equals method overriden and I have to specify their id because all of then have multiple fields defined as id.
more, there is a hierarchical relation between entities.

ComplexEntity
    |__ List<Location>
             |__ List<Airline>

and Location has in its unique key ComplexEntity and Airline also has in its unique key a Location.

my first try was to make all of them ValueObject but the result was not the one expected.

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Jan 20, 2018

@umk8yw i don't see Airline and Location classes, could you express your issue in more general way? What feature or features you want to add to JaVers finally? Would this features be useful for other Javers users?

@ismaelgomescosta

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2018

@bartoszwalacik I opened a solution for this issue. and its a simple change needed to make this case possible.

#644

In my case I have an entity which is the Id of another entity.

Like
Person
|___ PersonDocumentation
Where

Documentation
|___
@id Person

I can't change this structure because Person saves personDocumentation by cascade and the Id of my documentation is the Id of my People. And i can`t change this because I work in a legacy project.

Please help me with that.

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Mar 1, 2018

This issue is about making this mapping possible

class Person {
    @Id String name
    @Id String surname
    @Id LocalDate dob
    ...
}

usually, this is called composite Id

@singhjoga

This comment has been minimized.

Copy link

commented Sep 21, 2018

Hi,
I wanted to use Javers in my project where many entities have multiple Id fields and entity itself is annotated with IdClass.
Is there any further progress on this issue? If not, maybe with your guidance, I can try to implement it.

Best Regards,

Joga

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Sep 22, 2018

@singhjoga nobody is working on this issue now, your contribution is welcome. I can guide you by writing first failing test case.

@singhjoga

This comment has been minimized.

Copy link

commented Sep 24, 2018

Hi @bartoszwalacik,
Yes please, send me the information to get started. I have already forked the git repo from master.

Joga

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

OK, so you can start from adding support for composite Id to the diff algorithm (javers.compare()) and then to the persistence layer (javers.commit()). New tests for comparing should be placed close to JaversDiffE2ETest.

In your test, write some Class, for example:

Person class {
    @Id private String firstName;
    @Id private String lastName; 
}

and try to compare some objects.

bartoszwalacik added a commit that referenced this issue Oct 19, 2018

#625
failing test
@shiruba

This comment has been minimized.

Copy link

commented Feb 7, 2019

I would like to contribute. I already extended the tests in CaseWithCompositeId, which are failing as expected. Now I am trying to understand what I should do to fix these tests.

My guess is that I need to change the EntityType so that it can hold more than one idProperty. This also involves the EntityTypeFactory, which creates the EntityType.

What should I do after that?

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Then GlobalIdFactory, the place where InstanceId is created.
InstanceId holds object's Id as Object. If we don't want to change this structure, we can put here a List of all values grabbed from fields marked as @Id

 private final Object cdoId;

bartoszwalacik added a commit that referenced this issue May 4, 2019

bartoszwalacik added a commit that referenced this issue May 4, 2019

#625
unmodifiableMap

bartoszwalacik added a commit that referenced this issue May 5, 2019

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented May 11, 2019

released in 5.4.0, see https://javers.org/release-notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.