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

Aspect around methods from Spring Data repositories #47

Closed
pszymczyk opened this Issue Oct 2, 2014 · 19 comments

Comments

Projects
None yet
3 participants
@pszymczyk
Member

pszymczyk commented Oct 2, 2014

As a Spring Data user i want to have aspect rounding all methods from Spring Data repositories. Aspect should call javers.commit() if arguments from join point have @JaversAuditable annotation.

@pszymczyk pszymczyk changed the title from Spring Framework integration to Aspect around methods from Spring Data repositories Oct 6, 2014

pszymczyk added a commit that referenced this issue Oct 28, 2014

pszymczyk added a commit that referenced this issue Oct 28, 2014

@pszymczyk pszymczyk self-assigned this Nov 5, 2014

bartoszwalacik added a commit that referenced this issue Jan 10, 2015

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 10, 2015

we also need a documentaion for this:

/documentation/spring-integration

@gessnerfl

This comment has been minimized.

Contributor

gessnerfl commented Feb 15, 2015

Is this issue still open? I implemented and aspect around save and delete methods on spring data mongo repositories. The aspect will write changes after save or delete was executed successfully.

If you are interested in this I can try to integrate this into you code line and provide a pull request.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 16, 2015

Hi, issue is implemented and released but not closed due to missing documentation. Would you like to help us in testing this issue and give us some feedback?

@pszymczyk

This comment has been minimized.

Member

pszymczyk commented Feb 16, 2015

Hi, here you have PR in which the change came:
https://github.com/javers/javers/pull/81/files

@gessnerfl

This comment has been minimized.

Contributor

gessnerfl commented Feb 16, 2015

I reviewed the pull request. Basically it seems that the implementation is independent from spring data repositories. If class-based annotation is used it seems that a version commit is made on ever method implemented by the given class.
This behavior is not obvious for me. When I think about standard repository implementations usually methods like get* or find* are not relevant in this case. I would assume that changes are only applied if data is modified which I would not expect from a finder method.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 16, 2015

as far as I remenber there is also a method level annotation, which allow
you to select subset of methods.
Pawel, please correct me if I'm wrong

On Mon, Feb 16, 2015 at 6:58 PM, gessnerfl notifications@github.com wrote:

I reviewed the pull request. Basically it seems that the implementation is
independent from spring data repositories. If class-based annotation is
used it seems that a version commit is made on ever method implemented by
the given class.
This behavior is not obvious for me. When I think about standard
repository implementations usually methods like get* or find* are not
relevant in this case. I would assume that changes are only applied if data
is modified which I would not expect from a finder method.


Reply to this email directly or view it on GitHub
#47 (comment).

@gessnerfl

This comment has been minimized.

Contributor

gessnerfl commented Feb 16, 2015

This is true but do you think that it is obvious for a library consumer that in the following example a findByName would also result in a version change? Keep in mind that if spring data repositories are used you are not able to user the default repositories out of the box as standard methods like findOne or findAll would also result in version changes.

@JaversAuditable
@Repository
public UserRespository implements MongoRepository<ObjectId, User> {
    User findByName(String name);
}

I'm just thinking from a consumer perspective. I personally thought applying the aspect on a lower level like MongoOperations. I also decided to add the aspect to the repository as it gives more flexibility to the user.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 16, 2015

Well, I think we didn't tested this with spring-data, but excluding certain method name pattern (like find*) seems like a good idea.

Not sure about this deep dive to MongoOperations. Javers Spring integration should work well also with other repositories, for example we are working now on JaversSQLRepository. So we would like to stay at higher level (close to the domain/repository layer).

So what we can do about this default support for spring-data? Do you think you can came up with some PR for this?

@pszymczyk

This comment has been minimized.

Member

pszymczyk commented Feb 16, 2015

Hi

  1. Annotations works for custom (non Spring Data) repositories.
  2. You can select which methods Javers should follow by adding annotation over method.

e.g:
Here is custom reposiotory:
https://github.com/javers/javers/blob/master/javers-spring/src/test/java/org/javers/spring/integration/ProjectRepository.java

and here is test using this repo:
https://github.com/javers/javers/blob/master/javers-spring/src/test/groovy/org/javers/spring/integration/JaversCommitAdviceIntegrationTest.groovy#L29

As you can see only save method is annotated, after invove update Javers will not commit change.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 16, 2015

@gessnerfl give us a while, we need some time for internal discusion ...

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 16, 2015

@gessnerfl
ok, our proposed solution is:

We can add spring-data support for JaVers as follows:

New class-level annotation, to be placed on standard spring-data interfaces.
@JaversSpringDataAuditable

This annotation could be aware of spring-data naming convention and
could configure auditing aspect to wrap only data-altering methods:
save(), delete(), update().

Is this solution suitable for you, do you think you could contribute to this?

@gessnerfl

This comment has been minimized.

Contributor

gessnerfl commented Feb 16, 2015

That sounds fine for me. I was just a bit confused because this ticket states support for spring data repositories but would result in a strange behavior.
Your proposed solution would now provide the option for both

  • easy to use spring-data-repositories
  • freedom and openness for custom repositories.
@pszymczyk

This comment has been minimized.

Member

pszymczyk commented Feb 18, 2015

@gessnerfl now we have a lot of things to do around sql repository, maybe you can provide some implementation of this issue?

@gessnerfl

This comment has been minimized.

Contributor

gessnerfl commented Feb 18, 2015

I will try to provide a pull request for this. For my implementation I used AspectJ but I will migrate it to an BeanPostProcessor.
There is only one issue related to object deletion. Spring repositories allow to delete objects by ID. From the javers api it is not 100% clear if commitShallowDelete(String author, Object object) also works if the object is not the object instance but an GlobalIdDTO.
Does this work? I think for deletion it would be quite useful. If not I will have to find an alternative how I can get the latest state before the object is deleted.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 18, 2015

@gessnerfl, great! Give us a notice if you need some support while working on the PR (persmissions, etc). We write tests in Spock and we are crazy about TDD.

Concerning object delete, you are right. Currently you commit deletion only using object instance. I'll implement a method allowing deletion by GlobalId

bartoszwalacik added a commit that referenced this issue Feb 19, 2015

#47
javers.commitShallowDeleteById()
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 19, 2015

PR with javers.commitShallowDeleteById(String author, GlobalIdDTO globalId)
#108

gessnerfl added a commit to gessnerfl/javers that referenced this issue Feb 22, 2015

gessnerfl added a commit to gessnerfl/javers that referenced this issue Feb 23, 2015

gessnerfl added a commit to gessnerfl/javers that referenced this issue Feb 23, 2015

pszymczyk added a commit that referenced this issue Feb 23, 2015

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 27, 2015

@gessnerfl, thanks for contribution, new version of javers-spring-integration is released in v 1.0.7

@gessnerfl

This comment has been minimized.

Contributor

gessnerfl commented Feb 27, 2015

No problem. Was a lot of fun for me and first time using Spock - and I really like it ;)

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