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.

Sign up for GitHub

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

Jump to bottom

Merge class-level and method-level @Sql declarations #1835

Closed
wants to merge 9 commits into from
Closed

Merge class-level and method-level @Sql declarations #1835

wants to merge 9 commits into from

Conversation

asympro
Copy link
Contributor

@asympro asympro commented May 25, 2018 •
edited by sbrannen
Loading

Pull request for #20570

@asympro asympro changed the title SPR-16021 implemented SPR-16021 Merge class-level and method-level @Sql declarations May 25, 2018
@sbrannen sbrannen self-assigned this Jun 2, 2018
sbrannen
sbrannen requested changes Jun 2, 2018
View reviewed changes
Copy link
Member

@sbrannen sbrannen left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the changes requested in comments.

In addition, please revert all changes to existing test classes. We need new, separate test classes in order to properly test the new feature while ensuring 100% backwards compatibility for existing code bases.

When introducing the new tests, please add some inline comments explaining why you have chosen to drop the schema if that was not done in the existing tests.

Thanks!

...est/src/main/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListener.java Outdated Show resolved Hide resolved
...-test/src/test/java/org/springframework/test/context/jdbc/DataSourceOnlySqlScriptsTests.java Outdated Show resolved Hide resolved
...est/src/test/java/org/springframework/test/context/jdbc/NonTransactionalSqlScriptsTests.java Outdated Show resolved Hide resolved
.../test/java/org/springframework/test/context/jdbc/RepeatableSqlAnnotationSqlScriptsTests.java Outdated Show resolved Hide resolved
@chrismacp
Copy link

This code is looking good, can't wait to use it!

@sbrannen
Copy link
Member

sbrannen commented Aug 3, 2018

@asympro, what's the status of your work on this?

@asympro
Copy link
Contributor Author

asympro commented Aug 3, 2018 via email

@sbrannen
Copy link
Member

sbrannen commented Aug 3, 2018

OK.

thanks for the feedback

@asympro
Copy link
Contributor Author

asympro commented Aug 6, 2018

Should be finished now, please review.

@chrismacp
Copy link

Tests are looking good I think. What do you think @sbrannen?

@spring-projects-issues spring-projects-issues mentioned this pull request Jan 11, 2019
Closed
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 24, 2019
@sbrannen sbrannen added in: test Issues in the test module type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 22, 2019
@sbrannen sbrannen added this to the 5.2 M1 milestone Feb 22, 2019
@sbrannen sbrannen changed the title SPR-16021 Merge class-level and method-level @Sql declarations Merge class-level and method-level @Sql declarations Feb 22, 2019
@sbrannen sbrannen modified the milestones: 5.2 M1, 5.2 M2 Apr 2, 2019
@jhoeller jhoeller modified the milestones: 5.2 M2, 5.2 M3 May 2, 2019
@sbrannen sbrannen removed this from the 5.2 M3 milestone Jun 11, 2019
@sbrannen sbrannen added this to the 5.2 RC1 milestone Jun 11, 2019
@sbrannen
Copy link
Member

Tests are looking good I think. What do you think @sbrannen?

I'm reviewing it locally now.

@sbrannen
Copy link
Member

Current work on this PR can be viewed in the following feature branch.

https://github.com/sbrannen/spring-framework/commits/issues/gh-1835-merge-sql-annotations

@sbrannen
Copy link
Member

In my latest commit on the aforementioned feature branch, I've moved the merge mode configuration to a dedicated @SqlMergeMode annotation in order to avoid ambiguities regarding when the merge mode should be supplied.

@asympro, feel free to chime in if you like. Otherwise, I'll take it from here.

@asympro
Copy link
Contributor Author

asympro commented Jul 21, 2019

In my latest commit on the aforementioned feature branch, I've moved the merge mode configuration to a dedicated @SqlMergeMode annotation in order to avoid ambiguities regarding when the merge mode should be supplied.

@asympro, feel free to chime in if you like. Otherwise, I'll take it from here.

@sbrannen, I have looked through your work in the branch and it looks good.

What can I do further to provide progress on the feature?

@sbrannen
Copy link
Member

I have looked through your work in the branch and it looks good.

Thanks for the feedback.

What can I do further to provide progress on the feature?

Your feedback was enough. I'll take it from here.

sbrannen pushed a commit to sbrannen/spring-framework that referenced this pull request Jul 21, 2019
@asympro @sbrannen
Merge class-level and method-level @Sql declarations
d77b715 
See spring-projectsgh-1835
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Jul 21, 2019
@sbrannen
Polish contribution
ab88762 
See spring-projectsgh-1835
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Jul 21, 2019
@sbrannen
Document @SqlMergeMode support in reference manual
ccdf04e 
See spring-projectsgh-1835
@sbrannen sbrannen closed this in 89571ea Jul 21, 2019
sbrannen added a commit that referenced this pull request Jul 21, 2019
@sbrannen
Merge class-level and method-level @Sql declarations
9cb15ba 
Prior to these commits method-level @Sql declarations always overrode
class-level @Sql declarations, which required developers to redeclare
class-level @Sql declarations on test methods (e.g., via
copy-and-paste) in order to combine them with method-level @Sql
declarations.

These commits provide developers the ability to optionally merge
class-level and method-level @Sql declarations via a new @SqlMergeMode
annotation that can be applied at the class level or method level, with
method-level declarations of @SqlMergeMode overriding class-level
declarations. For example, @SqlMergeMode(MERGE) switches from the
default OVERRIDE behavior to merging behavior, with class-level SQL
scripts and statements executed before method-level SQL scripts and
statements.

Closes gh-1835
@sbrannen
Copy link
Member

This new feature has been merged into master in commit 9cb15ba.

@asympro, thanks again for the PR that got the ball rolling!

@asympro
Copy link
Contributor Author

asympro commented Jul 21, 2019

@sbrannen the pleasure is mine.

Thanks for the reviews and polishing to a better solution!

@aahlenst aahlenst mentioned this pull request Aug 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbrannen sbrannen sbrannen approved these changes

Assignees

@sbrannen sbrannen

Labels
in: test Issues in the test module type: enhancement A general enhancement
Projects
None yet
Milestone
5.2 RC1
Development

Successfully merging this pull request may close these issues.
5 participants
@asympro @chrismacp @sbrannen @jhoeller @spring-projects-issues