Introduce opt-in support for merging class-level and method-level @Sql declarations [SPR-16021]

[spring-projects-issues]

Chris MacPherson opened SPR-16021 and commented

I would like to propose a small improvement which would enable another use-case of the @Sql annotation within test cases.

As mentioned in the docs here "method-level declarations override class-level".

For the following usecase I would like this not to occur so that I can set my tests up as follows:

@Sql(scripts = "/fixtures/cleanup.sql", 
    executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD)
public class TestCase {

  @Test
  @Sql(scripts = "/fixtures/myclass/test1.sql")       
  public void aTestForOneUseCase() {
  }

  @Test
  @Sql(scripts = "/fixtures/myclass/test2.sql")       
  public void aTestForAnotherUseCase() {
  }

}

I can achieve in Java this by just adding the class level @Sql, from above, as a second @Sql instance in each test, but this becomes quite ugly and my test class starts to look like a list of annotations.

In Groovy, using Spock this doesn't work though, I get the following error:

Error:(43, 5) Groovyc: Cannot specify duplicate annotation on the same member : org.springframework.test.context.jdbc.Sql

Then I have to use an @SqlGroup for the two scripts which increases the annotations even more.

@SqlGroup([
       @Sql(scripts = "/fixtures/myclass/test1.sql"),
       @Sql(scripts = "/fixtures/cleanup.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD)])

So this is essentially a way to have a clean way to run an @Sql after every test. I've been trying to figure out another good way of doing it without relying on @Transactional.

I noticed this other ticket #18929 which is kind of asking the same thing, but it's not clear to me in the description. What it's asking for does seem to exist anyway, the Sql.ExecutionPhase part. But when I use them, the method level still seems to override the class level even though there's a different executionPhase.

So maybe this just needs the execution phase to be overridden at the method level? Hopefully that won't mess with other use-cases.

---

Affects: 4.3.11

Reference URL: https://stackoverflow.com/questions/32871817/using-annotation-sql-is-it-possible-to-execute-scripts-in-class-level-before-m

Issue Links:

• Introduce class-level execution phases for @Sql [SPR-14357] #18929 Introduce class-level execution phases for @Sql

Referenced from: pull request #1835

1 votes, 3 watchers

[spring-projects-issues]

Sam Brannen commented

Thanks for creating the JIRA issue.

This has been raised in several different forums over the years, so we'll consider adding "merge support" for class-level and method-level @Sql declarations.

I've renamed this issue accordingly.

#18929 is in fact similar but not exactly the same. So we'll look at the two issues together.

Cheers,

Sam

[spring-projects-issues]

Chris MacPherson commented

Excellent, I'd love to help but not sure my Spring skills would be up to it yet. Thanks for having a look in to it.

[spring-projects-issues]

Semukhin Dmitry commented

Plan of solution:

/**
 * Execute SQL scripts configured via {@link Sql @Sql} for the supplied
 * {@link TestContext} and {@link ExecutionPhase}.
 */
private void executeSqlScripts(TestContext testContext, ExecutionPhase executionPhase) throws Exception {
	boolean classLevel = false;

	Set<Sql> sqlAnnotations = AnnotatedElementUtils.getMergedRepeatableAnnotations(testContext.getTestMethod(),
		Sql.class, SqlGroup.class);
	if (sqlAnnotations.isEmpty()) {
		sqlAnnotations = AnnotatedElementUtils.getMergedRepeatableAnnotations(testContext.getTestClass(), Sql.class,
			SqlGroup.class);
		if (!sqlAnnotations.isEmpty()) {
			classLevel = true;
		}
	}

	for (Sql sql : sqlAnnotations) {
		executeSqlScripts(sql, executionPhase, testContext, classLevel);
	}
}

 
Rework this code into invocation of executeSqlScripts(sql, executionPhase, testContext, classLevel); for each of sqlAnnotation levels (method, class).

 
Therefore invocation will be of the merged 2 sets of annotations.

Note: this solution of the task itself breaks backwards compatibility which I believe is not 100% requirement for new features.

[spring-projects-issues]

Semukhin Dmitry commented

Pull request open:

 

#1835

[spring-projects-issues]

Sam Brannen commented

this solution of the task itself breaks backwards compatibility which I believe is not 100% requirement for new features.

That is unfortunately not an option.

We cannot introduce breaking changes for this feature since existing code relies on the current, documented semantics.

In order to introduce a new feature like this, it will have to be an opt-in feature that is controlled by a flag.

We typically implement such a flag as an enum with the default value of the flag set to an enum constant that represents the original behavior.

A prime example of this technique can be seen in @TestExecutionListeners(mergeMode = MERGE_WITH_DEFAULTS), where the default merge mode is REPLACE_DEFAULTS (i.e., the original behavior for @TestExecutionListeners.

[spring-projects-issues]

Sam Brannen commented

Thanks for submitting the PR. I submitted a review on GitHub.

 

[spring-projects-issues]

Sam Brannen commented

FYI: I took the liberty to edit your comments/proposal.