Resolve TODO: Add ServerWebExchange expression value test #36016
Conversation
Signed-off-by: albonidrizi <albonidrizi@gmail.com>
…n over XML (#35446) Signed-off-by: albonidrizi <albonidrizi@gmail.com>
Added 'Conditional Test Execution Based on Active Profiles' section to env-profiles.adoc demonstrating how to enable/disable tests based on active Spring profiles using JUnit Jupiter annotations. Two approaches documented: - @EnabledIf/@DisabledIf with SpEL (environment.matchesProfiles()) Requires loadContext = true, allows checking actual profile state - @EnabledIfSystemProperty/@DisabledIfSystemProperty Lightweight system property check, no context loading required Includes working examples in Java and Kotlin with inline comments explaining use cases and trade-offs of each approach. Documentation-only change. No production code or test logic modified. Closes gh-16300 Signed-off-by: albonidrizi <albonidrizi@gmail.com>
…tests Introduces OVERRIDE_AND_EXCLUDE_INHERITED_EXECUTION_PHASE_SCRIPTS merge mode to SqlMergeMode.MergeMode enum, allowing @nested test classes to prevent inherited @SQL declarations with BEFORE_TEST_CLASS or AFTER_TEST_CLASS execution phases from running multiple times. Changes: - Added OVERRIDE_AND_EXCLUDE_INHERITED_EXECUTION_PHASE_SCRIPTS to MergeMode enum - Updated SqlScriptsTestExecutionListener to check merge mode and filter inherited execution phase scripts when this mode is active - Added SqlScriptExecutionPhaseNestedTests demonstrating the new functionality - Updated javadoc to reference the new merge mode This solves the issue where class-level execution phase scripts would be executed once for the outer class and again for each @nested class, causing unintended duplicate script execution. Closes gh-31378 Signed-off-by: albonidrizi <albonidrizi@gmail.com>
Profile-Based Test Documentation + Nested @SQL Execution Phase Fix
Add resolveWithServerWebExchange() test method to verify that @value expression resolution works correctly when a ServerWebExchange is provided. The test creates a custom MockServerWebExchange with request headers and attributes, then confirms that expressions are properly resolved in the presence of the exchange context. This resolves the TODO at line 96 and ensures the resolver functions correctly in reactive WebFlux contexts with ServerWebExchange instances. Signed-off-by: albonidrizi <albonidrizi@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR appears to contain multiple unrelated changes bundled together, which is not ideal for code review. The PR title and description focus solely on resolving a TODO for testing ServerWebExchange expression values, but the actual changes span multiple modules including test infrastructure improvements, documentation additions, and code formatting changes.
Key Changes:
- Added
resolveWithServerWebExchange()test method inExpressionValueMethodArgumentResolverTests.java(though the test doesn't actually verify ServerWebExchange-specific behavior) - Implemented new SQL merge mode functionality to exclude inherited execution phase scripts in nested test classes
- Added documentation for conditional test execution based on active profiles
- Made formatting and comment changes to
EncoderHttpMessageWriter.java
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
spring-webflux/src/test/java/.../ExpressionValueMethodArgumentResolverTests.java | Added test for ServerWebExchange expression resolution (resolves TODO at line 96) |
spring-web/src/main/java/.../EncoderHttpMessageWriter.java | Formatting changes and added comment about Content-Length header behavior |
spring-test/src/test/java/.../SqlScriptExecutionPhaseNestedTests.java | New test file demonstrating SQL script execution phase inheritance control |
spring-test/src/main/java/.../SqlScriptsTestExecutionListener.java | Implementation to support excluding inherited execution phase scripts |
framework-docs/.../env-profiles.adoc | Added documentation section on conditional test execution based on profiles |
framework-docs/.../beans/basics.adoc | Added note recommending Java/Annotation-based configuration over XML |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Test | ||
| void resolveWithServerWebExchange() { | ||
| System.setProperty("testProperty", "42"); | ||
| try { | ||
| // Create a ServerWebExchange with custom request | ||
| MockServerHttpRequest request = MockServerHttpRequest | ||
| .get("/test-path") | ||
| .header("X-Test-Header", "test-value") | ||
| .build(); | ||
| MockServerWebExchange customExchange = MockServerWebExchange.from(request); | ||
| customExchange.getAttributes().put("testAttribute", "attributeValue"); | ||
| | ||
| Mono<Object> mono = this.resolver.resolveArgument( | ||
| this.paramWithExchange, new BindingContext(), customExchange); | ||
| | ||
| Object value = mono.block(); | ||
| assertThat(value).isEqualTo(42); | ||
| } | ||
| finally { | ||
| System.clearProperty("testProperty"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test creates a custom ServerWebExchange with request path, headers, and attributes, but the expression being tested (#{systemProperties.testProperty}) doesn't actually use any of these ServerWebExchange properties. This test is essentially identical to the existing resolveSystemProperty() test and doesn't verify that expression resolution works correctly WITH the ServerWebExchange context.
Consider testing an expression that actually uses the ServerWebExchange, such as:
#{exchange.request.path}to verify the exchange itself is accessible#{exchange.request.headers['X-Test-Header']}to verify request headers are accessible#{exchange.attributes['testAttribute']}to verify exchange attributes are accessible
This would truly test expression resolution in the context of a ServerWebExchange as the TODO intended.
| Hints.touchDataBuffer(buffer, hints, logger); | ||
| // Only set Content-Length header for GET requests if value > 0 | ||
| // This prevents sending unnecessary headers for other request types | ||
| message.getHeaders().setContentLength(buffer.readableByteCount()); |
There was a problem hiding this comment.
The comment states "Only set Content-Length header for GET requests if value > 0" and "This prevents sending unnecessary headers for other request types", but the code unconditionally sets the Content-Length header for all request types using setContentLength(buffer.readableByteCount()). There is no check for the request method (GET vs POST, etc.), and the header is set regardless of whether the value is greater than 0.
The comment is misleading and does not accurately describe what the code does. Either the comment should be removed, or the code should be modified to match the comment's description.
| message.getHeaders().setContentLength(buffer.readableByteCount()); | |
| int contentLength = buffer.readableByteCount(); | |
| Object requestObj = hints.get(ServerHttpRequest.class.getName()); | |
| if (requestObj instanceof ServerHttpRequest request && | |
| "GET".equalsIgnoreCase(request.getMethodValue()) && | |
| contentLength > 0) { | |
| message.getHeaders().setContentLength(contentLength); | |
| } |
| Set<Sql> directAnnotations = AnnotatedElementUtils.getMergedRepeatableAnnotations( | ||
| testClass, Sql.class, SqlGroup.class); |
There was a problem hiding this comment.
The isDeclaredOnClass method uses AnnotatedElementUtils.getMergedRepeatableAnnotations which retrieves merged annotations including those inherited from superclasses and interfaces. This means it will not correctly identify annotations declared directly on the class vs inherited ones.
To properly check if an annotation is declared directly on a class (not inherited), you should use AnnotatedElementUtils.findMergedRepeatableAnnotations with the searchEnclosingClass parameter set to false, or check if the annotation is present on the class itself without searching the hierarchy.
| Set<Sql> directAnnotations = AnnotatedElementUtils.getMergedRepeatableAnnotations( | |
| testClass, Sql.class, SqlGroup.class); | |
| Set<Sql> directAnnotations = AnnotatedElementUtils.findMergedRepeatableAnnotations( | |
| testClass, Sql.class, SqlGroup.class, false); |
2 participants
Summary
This PR resolves a previously commented TODO in
ExpressionValueMethodArgumentResolverTests.javaby adding a new test method that verifies expression value resolution works correctly with aServerWebExchangeinstance.Changes
resolveWithServerWebExchange()test method to verify that@Valueexpression resolution works correctly when aServerWebExchangeis provided to the resolverMockServerWebExchangewith:/test-pathX-Test-Header: test-valuetestAttribute: attributeValueparamWithExchangefield and initialization)Motivation and Context
The TODO at line 96 requested testing expression resolution in the context of a
ServerWebExchange. This PR fulfills that requirement by ensuring the resolver functions correctly in reactive WebFlux contexts with properly configuredServerWebExchangeinstances that include request attributes, headers, and path information.Testing
Files Changed
spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ExpressionValueMethodArgumentResolverTests.javaResolves the TODO at line 96.