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

Replace string id feature #614

Closed
ismaelgomescosta opened this Issue Nov 30, 2017 · 23 comments

Comments

Projects
None yet
2 participants
@ismaelgomescosta
Contributor

ismaelgomescosta commented Nov 30, 2017

I need a feature to override tha method called to create the id of the entity.

I have a composite id with entities embbeded. So I created a new annotation to customize Javers to use a defined method not toString Object.

So before Javers created globalId like = 'dummyEntityWithCompositeEmbeddedId/org.javers.core.model.DummyPoint@5b10,1'

and with this new feature I can customize for = 'dummyEntityWithCompositeEmbeddedId/1,2,1'

Here is my pull request #613. Sorry about my last issues. I should read the CONTRIBUTING.md before

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Nov 30, 2017

Did you tried to implement custom Type Adapter ?
see https://javers.org/documentation/domain-configuration/#ValueType

@ismaelgomescosta

This comment has been minimized.

Contributor

ismaelgomescosta commented Nov 30, 2017

My difficulty isn't the serialization of cdoId. My problem is the serialization of globalId_key field.
And this is part of javers core. I know my feature can be looks like a personal necessity but if you run my tests without the new feature you will see globalId_key is equal 'dummyEntityWithCompositeEmbeddedId/org.javers.core.model.DummyPoint@5b10,1'

My worry is about maintenance of snapshots. I will never can find by globalId_key because it was saved with object reference not with my real values.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Nov 30, 2017

ok , got it, that's resonable

@ismaelgomescosta

This comment has been minimized.

Contributor

ismaelgomescosta commented Nov 30, 2017

Thank you. Please if it's all right can you merge this feature? I need to use that next week in my job.

Thanks for your patience

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Nov 30, 2017

will try , but CR first

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 2, 2017

I have looked at the PR, why another annotation? If you can add annotation to this class why you cant just fix its toString()?

Btw, sometimes you can't modify a class (think about legacy code or libraries code).

I think about more versatile and simpler solution with Function:

API (JaversBuilder)

public <T> void registerToStringFunction(Class<T> clazz, Function<T, String> toString);

usage

javersBuilder.registerToStringFunction(YourClass.class, YourClass::customTtoString);
@ismaelgomescosta

This comment has been minimized.

Contributor

ismaelgomescosta commented Dec 4, 2017

I agree with you, it's a better solution then my. I will adjust the code

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 4, 2017

fine :)

@ismaelgomescosta

This comment has been minimized.

Contributor

ismaelgomescosta commented Dec 4, 2017

I made the changes you suggested. Can you review my code again please?

@ismaelgomescosta

This comment has been minimized.

Contributor

ismaelgomescosta commented Dec 4, 2017

I'm still work in a solution, as soon as possible I will commit it. Tks :)

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 4, 2017

ok, let me know when you will be ready :)

@ismaelgomescosta

This comment has been minimized.

Contributor

ismaelgomescosta commented Dec 5, 2017

Hi, I finished my changes. Can you review for me please?

bartoszwalacik added a commit that referenced this issue Dec 8, 2017

bartoszwalacik added a commit that referenced this issue Dec 8, 2017

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 8, 2017

Guys, I appreciate your effort. I did some changes to the API and impl to make the code more javers-way. Take a look at the tests, is it OK now?

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 8, 2017

Ismael Gomes Costa , your commits are not bounded to your github profile, if you bound them, you will be mentioned as a JaVers contributor. You can do it by adding the email attached to your commits to your github profile

@ismaelgomescosta

This comment has been minimized.

Contributor

ismaelgomescosta commented Dec 8, 2017

Ok, I will do it. Thanks

@ismaelgomescosta

This comment has been minimized.

Contributor

ismaelgomescosta commented Dec 8, 2017

I agree with your changes. But I didn't understand why the PR is not passing. Is this because any of the changes or is just because something wrong with travis? Another thing. I should keep using forks to contributing with Javers right?

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 8, 2017

that's strange, looks like sth is wrong with github-travis integrations, travis builds seems fine https://travis-ci.org/javers/javers/builds

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 8, 2017

@ismaelgomescosta forks are good for occasional contrubitors. If you'd like to contribute more, I can give you access to a branch in javers repository

@ismaelgomescosta

This comment has been minimized.

Contributor

ismaelgomescosta commented Dec 8, 2017

Ok. If I can I will try to contribute more. I like this project. I always had problems with envers and all these rows in the database. Now I use Javers with mongo and I'm happy with this.

bartoszwalacik added a commit that referenced this issue Dec 9, 2017

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 9, 2017

@ismaelgomescosta great! So what's next? (btw. 3.7.6 is on its way to Central)

@ismaelgomescosta

This comment has been minimized.

Contributor

ismaelgomescosta commented Dec 9, 2017

Thanks. At my job we are thinking change mongodb for dynamodb so I would like to contribute with a dynamodb module. I saw you have an issue open for this

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 9, 2017

good idea. if you want we can work together on data model design

@bartoszwalacik bartoszwalacik added the fixed label Dec 9, 2017

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 9, 2017

released in 3.7.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment