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

Auto configuration for Spring Data Envers #21370

Closed
scordio opened this issue May 8, 2020 · 10 comments
Closed

Auto configuration for Spring Data Envers #21370

scordio opened this issue May 8, 2020 · 10 comments
Assignees
Labels
status: superseded An issue that has been superseded by another type: enhancement A general enhancement

Comments

@scordio
Copy link
Contributor

scordio commented May 8, 2020

When using Spring Data Envers with Spring Boot, the EnversRevisionRepositoryFactoryBean has to be configured via @EnableJpaRepositories:

@EnableJpaRepositories(repositoryFactoryBeanClass = EnversRevisionRepositoryFactoryBean.class)

Maybe an auto configuration could take care of it. Would it make sense?

In case it does, I would be happy to prepare a pull request.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 8, 2020
@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label May 30, 2020
@philwebb
Copy link
Member

@mp911de Hi Mark, what are your thoughts on this? Is it a sensible thing to auto-configure?

@philwebb philwebb added status: waiting-for-feedback We need additional information before we can continue and removed for: team-attention An issue we'd like other members of the team to review labels Jun 11, 2020
@mp911de
Copy link
Member

mp911de commented Jun 11, 2020

Adding also @schauder to the conversation.

Envers repositories assume that the entity is audited and allows revision retrieval. From what I’ve seen we cannot expect that this assumption holds true for each application. Instead, only some entities are audited.

Since I’m not exactly sure on the impact on non-audited entities, I’d like Jens to chime in.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 11, 2020
@scordio
Copy link
Contributor Author

scordio commented Jun 11, 2020

Each repository would still need to extend from org.springframework.data.repository.history.RevisionRepository in order to get revision retrieval methods, so the application still has to properly declare a repository for each audited entity.

If I am not mistaken, a repository extending RevisionRepository cannot work with Envers if the above configuration is not applied. At the same time, the configuration above should have no impact on repositories not extending from RevisionRepository.

My idea is that when EnversRevisionRepositoryFactoryBean (or any other class coming from spring-data-envers) is in the classpath , most likely the application wants to use Spring Data JPA together with Envers, hence my proposal about the auto configuration.

@philwebb philwebb added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 11, 2020
@schauder
Copy link
Contributor

Pro auto configuration:

  • The impact on non audited entities should be minimal.
    Especially it is well separated from the JPA infrastructure.
    This impression is supported by the fact that I can't remember a single issue where Spring Data Envers triggered a bug in Spring Data JPA.

Contra:

  • Spring Data Envers is rather low usage (~1.5% downloads compared to Spring Data JPA)
  • I expect it to be common, that only some entities are audited. As mentioned above this SHOULD not be an issue.

I'm not sure what the criteria are for inclusion of auto configuration, but my gut reaction is: "Yeah, why not"

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 15, 2020
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Jul 8, 2020
@wilkinsona wilkinsona added this to the 2.x milestone Jul 8, 2020
@wilkinsona
Copy link
Member

Thanks, @schauder and @mp911de.

@scordio It sounds like it makes sense to offer some auto-configuration for this. Are you still interested in contributing a pull request? If so, I think it could be implemented by making some changes to org.springframework.boot.autoconfigure.data.jpa.JpaRepositoriesRegistrar.

@scordio
Copy link
Contributor Author

scordio commented Jul 8, 2020

@wilkinsona yes I am, thanks for the feedback.

I started already hacking around and I was thinking to conditionally register another registrar (extended from JpaRepositoriesRegistrar) when Envers is around.

I will open a draft PR for some brainstorming and make sure I am going in the right direction.

@wilkinsona
Copy link
Member

Excellent. Thank you. Please let us know if you have any questions.

@wilkinsona
Copy link
Member

How's it going, @scordio? Please do let us know if you need anything from us.

@scordio
Copy link
Contributor Author

scordio commented Jul 24, 2020

Hi @wilkinsona, thanks for checking! Unfortunately, in the last weeks I had very limited time to spend on this but I did small advancements.

During the weekend I will put what I have in a good shape and submit the draft PR.

@scordio
Copy link
Contributor Author

scordio commented Jun 14, 2021

Done in fbbfe6a.

@scordio scordio closed this as completed Jun 14, 2021
@snicoll snicoll added the status: superseded An issue that has been superseded by another label Jun 14, 2021
@snicoll snicoll removed this from the 2.x milestone Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants