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

Add auto-configuration for Spring Data Envers #22610

Conversation

scordio
Copy link
Contributor

@scordio scordio commented Jul 28, 2020

This adds the auto-configuration for Spring Data Envers.

Fixes #21370.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 28, 2020
@scordio
Copy link
Contributor Author

scordio commented Jul 28, 2020

I am not sure how to address testing properly in JpaRepositoriesAutoConfigurationTests. My preference would have been something like:

assertThat(context).getBean(CityRepository.class)
	.extracting(AopProxyUtils::getSingletonTarget)
	.isInstanceOf(EnversRevisionRepositoryImpl.class); // fails as it's a SimpleJpaRepository.class

but it doesn't work as the bean is always a SimpleJpaRepository.class although with the debugger I can see EnversRevisionRepositoryImpl being used. Any suggestion for a proper assertion?

Otherwise, I could add a CityRevisionRepository and perform verification on that, for both cases when Spring Data Envers is in the classpath or it is not.

@schauder may I ask your input on this?

@scordio scordio force-pushed the spring-data-envers_auto_configuration branch from c5120a2 to bec4dac Compare July 28, 2020 08:42
@wilkinsona wilkinsona added for: team-attention An issue we'd like other members of the team to review and removed for: team-attention An issue we'd like other members of the team to review labels Jul 28, 2020
@scordio scordio force-pushed the spring-data-envers_auto_configuration branch from bec4dac to b15a6cb Compare August 4, 2020 05:50
@scordio scordio marked this pull request as ready for review August 4, 2020 06:38
@scordio
Copy link
Contributor Author

scordio commented Aug 4, 2020

@wilkinsona sorry for the slow feedback, the PR should now be ready for review. I am wondering if some more test cases could be added, I will continue thinking about that.

I see the build is failing but I guess not because of my changes. I will keep an eye on master to see if a rebase if needed.

@scordio scordio requested a review from wilkinsona August 4, 2020 06:45
@scordio scordio force-pushed the spring-data-envers_auto_configuration branch 5 times, most recently from 69a91ad to ad4987a Compare August 5, 2020 20:18
@scordio
Copy link
Contributor Author

scordio commented Aug 5, 2020

I finished to refactor the tests the way I had in mind and I don't plan to have any other change. Please let me know if there is something to be adjusted.

(the build is failing but it doesn't seem related to my changes)

@scordio scordio force-pushed the spring-data-envers_auto_configuration branch 2 times, most recently from 7b4937a to 7186fd8 Compare August 10, 2020 19:02
@scordio
Copy link
Contributor Author

scordio commented Sep 28, 2020

Hi everyone, is there anything else I could do to move this one forward?

@snicoll
Copy link
Member

snicoll commented Sep 29, 2020

@scordio thank you for your patience and sorry we didn't have the time to get back to you yet. We're busy handling the scheduled backlog for 2.4.x so it may take a bit more time before we can spend time on this one.

@scordio
Copy link
Contributor Author

scordio commented Sep 29, 2020

@snicoll thanks for the feedback and no problem, it was just a friendly reminder 🙂

@scordio scordio force-pushed the spring-data-envers_auto_configuration branch from 7186fd8 to eaa3981 Compare December 16, 2020 00:42
@scordio
Copy link
Contributor Author

scordio commented Jan 20, 2021

Hi @snicoll, just wanted to check if this is blocked for any reason and if I can help with it.

@snicoll
Copy link
Member

snicoll commented Jan 20, 2021

Hello @scordio thank you for your patience. I did review the proposal during the holidays and wanted to experiment how we could avoid having to subclass JpaRepositoriesRegistrar.

In retrospect, I should have written that down to get some feedback from the team. I've added that to my todo.

@scordio
Copy link
Contributor Author

scordio commented Jan 20, 2021

I deleted my previous comment, I mixed up different things. Thanks for looking into it!

@scordio
Copy link
Contributor Author

scordio commented Jan 20, 2021

Out of curiosity, what concerns do you have about subclassing? I had the assumption that Jpa is a prerequisite to use spring-data-envers, therefore any configuration applied by the JpaRepositoriesRegistrar should be also applied with the new one.

@wilkinsona
Copy link
Member

wilkinsona commented Feb 1, 2021

I have just discovered that there's no reference documentation for Spring Data Envers. Unfortunately we can't afford the support burden of an undocumented project so I'm marking this one as blocked.

@wilkinsona wilkinsona added the status: blocked An issue that's blocked on an external project change label Feb 1, 2021
@scordio
Copy link
Contributor Author

scordio commented Feb 1, 2021

Got it, thanks for looking into it. I'll get in touch with the project to understand if they are willing to improve the docs and accept some help.

@scordio
Copy link
Contributor Author

scordio commented Feb 25, 2021

Hello team, @schauder has taken care of the Spring Data Envers reference documentation (see spring-projects/spring-data-envers#279). Is that enough to unblock this PR?

@wilkinsona wilkinsona removed the status: blocked An issue that's blocked on an external project change label Feb 26, 2021
@scordio
Copy link
Contributor Author

scordio commented Mar 29, 2021

spring-projects/spring-data-envers#289 might influence this PR.

@philwebb philwebb added for: team-meeting An issue we'd like to discuss as a team to make progress type: enhancement A general enhancement and removed for: team-meeting An issue we'd like to discuss as a team to make progress status: waiting-for-triage An issue we've not yet triaged labels Apr 6, 2021
@philwebb philwebb added this to the 2.6.x milestone Apr 7, 2021
@scordio scordio force-pushed the spring-data-envers_auto_configuration branch from bc84a46 to 19e634b Compare May 10, 2021 10:18
@scordio
Copy link
Contributor Author

scordio commented May 10, 2021

I updated the PR to use the composed annotation introduced with spring-projects/spring-data-envers#290.

In case you have any suggestions on how to avoid the subclass, I'm happy to work in that direction.

@scordio scordio force-pushed the spring-data-envers_auto_configuration branch from 19e634b to a87fbd7 Compare May 10, 2021 13:11
@snicoll snicoll self-assigned this Jun 2, 2021
snicoll pushed a commit to snicoll/spring-boot that referenced this pull request Jun 3, 2021
snicoll added a commit to snicoll/spring-boot that referenced this pull request Jun 3, 2021
@snicoll snicoll modified the milestones: 2.6.x, 2.6.0-M1 Jun 3, 2021
snicoll pushed a commit that referenced this pull request Jun 14, 2021
@snicoll snicoll closed this in fbbfe6a Jun 14, 2021
@snicoll
Copy link
Member

snicoll commented Jun 14, 2021

@scordio thank you for making your first contribution to Spring Boot.

@scordio scordio deleted the spring-data-envers_auto_configuration branch June 14, 2021 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto configuration for Spring Data Envers
5 participants