From fe356624281e1f380bc8a60f23b54656faf4b206 Mon Sep 17 00:00:00 2001 From: j-sandy <30489233+j-sandy@users.noreply.github.com> Date: Fri, 10 Jan 2025 12:11:05 +0530 Subject: [PATCH] refactor(tests): replace AntPathMatcher as path matching strategy instead of default PathPatternParser and refactor spring security from 5.x to 6.x during upgrade of spring boot 3.0.x As of Spring Boot 2.6, the PathPatternParser is used by default. However, for pattern like /abc/**/xyz, PathPattenParser does not resolve the path. https://spring.io/blog/2022/05/24/preparing-for-spring-boot-3-0#use-spring-mvcs-pathpatternparser https://github.com/spring-projects/spring-framework/issues/24952 Due to this change, encountered below errors in igor-web module: ``` BuildControllerSpec > get the status of a build FAILED java.lang.IllegalStateException: Invalid mapping on handler class [com.netflix.spinnaker.igor.build.BuildController]: public void com.netflix.spinnaker.igor.build.BuildController.update(java.lang.String,java.lang.Integer,com.netflix.spinnaker.igor.build.model.UpdatedBuild,jakarta.servlet.http.HttpServletRequest) at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.lambda$detectHandlerMethods$1(AbstractHandlerMethodMapping.java:287) at org.springframework.core.MethodIntrospector.lambda$selectMethods$0(MethodIntrospector.java:74) at org.springframework.util.ReflectionUtils.doWithMethods(ReflectionUtils.java:366) at org.springframework.core.MethodIntrospector.selectMethods(MethodIntrospector.java:72) at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.detectHandlerMethods(AbstractHandlerMethodMapping.java:280) at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.processCandidateBean(AbstractHandlerMethodMapping.java:265) at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.initHandlerMethods(AbstractHandlerMethodMapping.java:224) at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.afterPropertiesSet(AbstractHandlerMethodMapping.java:212) at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.afterPropertiesSet(RequestMappingHandlerMapping.java:225) at org.springframework.test.web.servlet.setup.StandaloneMockMvcBuilder.registerMvcSingletons(StandaloneMockMvcBuilder.java:419) at org.springframework.test.web.servlet.setup.StandaloneMockMvcBuilder.initWebAppContext(StandaloneMockMvcBuilder.java:391) at org.springframework.test.web.servlet.setup.AbstractMockMvcBuilder.build(AbstractMockMvcBuilder.java:157) at com.netflix.spinnaker.igor.build.BuildControllerSpec.setup(BuildControllerSpec.groovy:116) Caused by: org.springframework.web.util.pattern.PatternParseException: No more pattern data allowed after {*...} or ** pattern element at app//org.springframework.web.util.pattern.InternalPathPatternParser.peekDoubleWildcard(InternalPathPatternParser.java:250) at app//org.springframework.web.util.pattern.InternalPathPatternParser.parse(InternalPathPatternParser.java:113) at app//org.springframework.web.util.pattern.PathPatternParser.parse(PathPatternParser.java:129) at app//org.springframework.web.servlet.mvc.condition.PathPatternsRequestCondition.parse(PathPatternsRequestCondition.java:84) at app//org.springframework.web.servlet.mvc.condition.PathPatternsRequestCondition.(PathPatternsRequestCondition.java:74) at app//org.springframework.web.servlet.mvc.method.RequestMappingInfo$DefaultBuilder.build(RequestMappingInfo.java:714) at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.createRequestMappingInfo(RequestMappingHandlerMapping.java:400) at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.createRequestMappingInfo(RequestMappingHandlerMapping.java:345) at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.getMappingForMethod(RequestMappingHandlerMapping.java:302) at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.getMappingForMethod(RequestMappingHandlerMapping.java:76) at app//org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.lambda$detectHandlerMethods$1(AbstractHandlerMethodMapping.java:283) ... 12 more ``` ``` GoogleCloudBuildTest > missingAccountTest() FAILED java.lang.IllegalStateException: Failed to load ApplicationContext at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContext(DefaultCacheAwareContextLoaderDelegate.java:143) at org.springframework.test.context.support.DefaultTestContext.getApplicationContext(DefaultTestContext.java:127) at org.springframework.test.context.web.ServletTestExecutionListener.setUpRequestContextIfNecessary(ServletTestExecutionListener.java:191) at org.springframework.test.context.web.ServletTestExecutionListener.prepareTestInstance(ServletTestExecutionListener.java:130) at org.springframework.test.context.TestContextManager.prepareTestInstance(TestContextManager.java:241) at org.springframework.test.context.junit.jupiter.SpringExtension.postProcessTestInstance(SpringExtension.java:138) Caused by: org.springframework.web.util.pattern.PatternParseException: No more pattern data allowed after {*...} or ** pattern element at app//org.springframework.web.util.pattern.InternalPathPatternParser.peekDoubleWildcard(InternalPathPatternParser.java:250) at app//org.springframework.web.util.pattern.InternalPathPatternParser.parse(InternalPathPatternParser.java:113) at app//org.springframework.web.util.pattern.PathPatternParser.parse(PathPatternParser.java:129) at app//org.springframework.web.servlet.mvc.condition.PathPatternsRequestCondition.parse(PathPatternsRequestCondition.java:84) at app//org.springframework.web.servlet.mvc.condition.PathPatternsRequestCondition.(PathPatternsRequestCondition.java:74) at app//org.springframework.web.servlet.mvc.method.RequestMappingInfo$DefaultBuilder.build(RequestMappingInfo.java:714) at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.createRequestMappingInfo(RequestMappingHandlerMapping.java:400) at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.createRequestMappingInfo(RequestMappingHandlerMapping.java:345) at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.getMappingForMethod(RequestMappingHandlerMapping.java:302) at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.getMappingForMethod(RequestMappingHandlerMapping.java:76) at app//org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.lambda$detectHandlerMethods$1(AbstractHandlerMethodMapping.java:283) ... 118 more ``` So, refactoring the tests to replace AntPathMatcher instead of PathPatternParser by adding the property `spring.mvc.pathmatch.matching-strategy = ANT_PATH_MATCHER` Ref: https://github.com/spinnaker/igor/pull/1211 ======================================================== With spring boot upgrade, spring security also upgrades from 5.x to 6.x. As per the migration [steps](https://www.baeldung.com/spring-security-migrate-5-to-6), `WebSecurityConfigurerAdapter` has been removed. So, it is not required to be extended, instead bean can be registered. --- .../igor/build/BuildControllerSpec.groovy | 83 +++++++------------ .../igor/gcb/GoogleCloudBuildTest.java | 17 ++-- 2 files changed, 40 insertions(+), 60 deletions(-) diff --git a/igor-web/src/test/groovy/com/netflix/spinnaker/igor/build/BuildControllerSpec.groovy b/igor-web/src/test/groovy/com/netflix/spinnaker/igor/build/BuildControllerSpec.groovy index b81aed788..14d4b3c72 100644 --- a/igor-web/src/test/groovy/com/netflix/spinnaker/igor/build/BuildControllerSpec.groovy +++ b/igor-web/src/test/groovy/com/netflix/spinnaker/igor/build/BuildControllerSpec.groovy @@ -16,6 +16,7 @@ package com.netflix.spinnaker.igor.build +import com.netflix.spinnaker.config.PluginsAutoConfiguration import com.netflix.spinnaker.igor.PendingOperationsCache import com.netflix.spinnaker.igor.build.model.GenericBuild import com.netflix.spinnaker.igor.build.model.UpdatedBuild @@ -33,15 +34,18 @@ import com.netflix.spinnaker.igor.travis.service.TravisService import com.netflix.spinnaker.kork.web.exceptions.ExceptionMessageDecorator import com.netflix.spinnaker.kork.web.exceptions.GenericExceptionHandlers import com.netflix.spinnaker.kork.web.exceptions.NotFoundException -import okhttp3.mockwebserver.MockWebServer +import org.spockframework.spring.SpringBean +import org.springframework.beans.factory.annotation.Autowired +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc +import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest import org.springframework.http.HttpStatus import org.springframework.http.MediaType import org.springframework.mock.web.MockHttpServletResponse +import org.springframework.test.context.ContextConfiguration +import org.springframework.test.context.TestPropertySource import org.springframework.test.web.servlet.MockMvc -import org.springframework.test.web.servlet.setup.MockMvcBuilders import retrofit.client.Header import retrofit.client.Response -import spock.lang.Shared import spock.lang.Specification import static com.netflix.spinnaker.igor.build.BuildController.InvalidJobParameterException @@ -55,19 +59,24 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers. * Tests for BuildController */ @SuppressWarnings(['DuplicateNumberLiteral', 'PropertyName']) +@AutoConfigureMockMvc(addFilters = false) +@WebMvcTest(controllers = [BuildController]) +@ContextConfiguration(classes = [PluginsAutoConfiguration, BuildController, GenericExceptionHandlers]) +@TestPropertySource( + properties = [ + "spring.application.name = igor", + "spring.mvc.pathmatch.matching-strategy = ANT_PATH_MATCHER" + ]) class BuildControllerSpec extends Specification { + @Autowired MockMvc mockMvc - BuildServices buildServices - BuildCache cache - JenkinsService jenkinsService - BuildOperations service - TravisService travisService - PendingOperationsCache pendingOperationService - ExceptionMessageDecorator exceptionMessageDecorator - - @Shared - MockWebServer server + @SpringBean BuildServices buildServices = Mock() + @SpringBean JenkinsService jenkinsService = Mock() + @SpringBean BuildOperations service = Mock() + @SpringBean TravisService travisService = Mock() + @SpringBean PendingOperationsCache pendingOperationService = Mock() + @SpringBean ExceptionMessageDecorator exceptionMessageDecorator = Mock() def SERVICE = 'SERVICE' def JENKINS_SERVICE = 'JENKINS_SERVICE' @@ -83,38 +92,16 @@ class BuildControllerSpec extends Specification { GenericBuild genericBuild - void cleanup() { - server.shutdown() - } - void setup() { - exceptionMessageDecorator = Mock(ExceptionMessageDecorator) - service = Mock(BuildOperations) - jenkinsService = Mock(JenkinsService) jenkinsService.getBuildServiceProvider() >> BuildServiceProvider.JENKINS - travisService = Mock(TravisService) travisService.getBuildServiceProvider() >> BuildServiceProvider.TRAVIS - buildServices = new BuildServices() - buildServices.addServices([ - (SERVICE) : service, - (JENKINS_SERVICE): jenkinsService, - (TRAVIS_SERVICE) : travisService, - ]) + buildServices.getService(SERVICE) >> service + buildServices.getService(JENKINS_SERVICE) >> jenkinsService + buildServices.getService(TRAVIS_SERVICE) >> travisService + genericBuild = new GenericBuild() genericBuild.number = BUILD_NUMBER genericBuild.id = BUILD_ID - - cache = Mock(BuildCache) - server = new MockWebServer() - pendingOperationService = Mock(PendingOperationsCache) - pendingOperationService.getAndSetOperationStatus(_, _, _) >> { - return new PendingOperationsCache.OperationState() - } - - mockMvc = MockMvcBuilders - .standaloneSetup(new BuildController(buildServices, pendingOperationService, Optional.empty(), Optional.empty(), Optional.empty())) - .setControllerAdvice(new GenericExceptionHandlers(exceptionMessageDecorator)) - .build() } void 'get the status of a build'() { @@ -285,6 +272,7 @@ class BuildControllerSpec extends Specification { void 'trigger a build with parameters to a job with parameters'() { given: + pendingOperationService.getAndSetOperationStatus(_, _, _) >> new PendingOperationsCache.OperationState() 1 * jenkinsService.getJobConfig(JOB_NAME) >> new JobConfig(buildable: true, parameterDefinitionList: [new ParameterDefinition(defaultParameterValue: [name: "name", value: null], description: "description")]) 1 * jenkinsService.buildWithParameters(JOB_NAME, [name: "myName"]) >> new Response("http://test.com", HTTP_201, "", [new Header("Location", "foo/${BUILD_NUMBER}")], null) @@ -298,6 +286,7 @@ class BuildControllerSpec extends Specification { void 'trigger a build without parameters to a job with parameters with default values'() { given: + pendingOperationService.getAndSetOperationStatus(_, _, _) >> new PendingOperationsCache.OperationState() 1 * jenkinsService.getJobConfig(JOB_NAME) >> new JobConfig(buildable: true, parameterDefinitionList: [new ParameterDefinition(defaultParameterValue: [name: "name", value: "value"], description: "description")]) 1 * jenkinsService.buildWithParameters(JOB_NAME, ['startedBy': "igor"]) >> new Response("http://test.com", HTTP_201, "", [new Header("Location", "foo/${BUILD_NUMBER}")], null) @@ -311,6 +300,7 @@ class BuildControllerSpec extends Specification { void 'trigger a build with parameters to a job without parameters'() { given: + pendingOperationService.getAndSetOperationStatus(_, _, _) >> new PendingOperationsCache.OperationState() 1 * jenkinsService.getJobConfig(JOB_NAME) >> new JobConfig(buildable: true) when: @@ -323,6 +313,7 @@ class BuildControllerSpec extends Specification { void 'trigger a build with an invalid choice'() { given: + pendingOperationService.getAndSetOperationStatus(_, _, _) >> new PendingOperationsCache.OperationState() JobConfig config = new JobConfig(buildable: true) config.parameterDefinitionList = [ new ParameterDefinition(type: "ChoiceParameterDefinition", name: "foo", choices: ["bar", "baz"]) @@ -342,6 +333,7 @@ class BuildControllerSpec extends Specification { void 'trigger a disabled build'() { given: + pendingOperationService.getAndSetOperationStatus(_, _, _) >> new PendingOperationsCache.OperationState() JobConfig config = new JobConfig() 1 * jenkinsService.getJobConfig(JOB_NAME) >> config 1 * exceptionMessageDecorator.decorate(_, _) >> "Job '${JOB_NAME}' is not buildable. It may be disabled." @@ -392,16 +384,10 @@ class BuildControllerSpec extends Specification { void "doesn't trigger a build when previous request is still in progress"() { given: - pendingOperationService = Stub(PendingOperationsCache) pendingOperationService.getAndSetOperationStatus("${JENKINS_SERVICE}:${PENDING_JOB_NAME}:NO_EXECUTION_ID:foo=bat", _, _) >> { return new PendingOperationsCache.OperationState(PendingOperationsCache.OperationStatus.PENDING) } - mockMvc = MockMvcBuilders - .standaloneSetup(new BuildController(buildServices, pendingOperationService, Optional.empty(), Optional.empty(), Optional.empty())) - .setControllerAdvice(new GenericExceptionHandlers()) - .build() - when: MockHttpServletResponse response = mockMvc.perform(put("/masters/${JENKINS_SERVICE}/jobs/${PENDING_JOB_NAME}") .contentType(MediaType.APPLICATION_JSON).param("foo", "bat")).andReturn().response @@ -412,18 +398,12 @@ class BuildControllerSpec extends Specification { void "resets the cache once the build status has been retrieved"() { given: - pendingOperationService = Mock(PendingOperationsCache) pendingOperationService.getAndSetOperationStatus("${JENKINS_SERVICE}:${JOB_NAME}:NO_EXECUTION_ID:foo=bat", _, _) >> { PendingOperationsCache.OperationState state = new PendingOperationsCache.OperationState() state.load(PendingOperationsCache.OperationStatus.COMPLETED.toString() + ":" + BUILD_NUMBER) return state } - mockMvc = MockMvcBuilders - .standaloneSetup(new BuildController(buildServices, pendingOperationService, Optional.empty(), Optional.empty(), Optional.empty())) - .setControllerAdvice(new GenericExceptionHandlers()) - .build() - when: MockHttpServletResponse response = mockMvc.perform(put("/masters/${JENKINS_SERVICE}/jobs/${JOB_NAME}") .contentType(MediaType.APPLICATION_JSON).param("foo", "bat")).andReturn().response @@ -448,7 +428,6 @@ class BuildControllerSpec extends Specification { then: 1 * jenkinsService.updateBuild(jobName, BUILD_NUMBER, new UpdatedBuild("this is my new description")) - 0 * _ response.status == 200 where: diff --git a/igor-web/src/test/groovy/com/netflix/spinnaker/igor/gcb/GoogleCloudBuildTest.java b/igor-web/src/test/groovy/com/netflix/spinnaker/igor/gcb/GoogleCloudBuildTest.java index 4cb87d951..0fd230d1f 100644 --- a/igor-web/src/test/groovy/com/netflix/spinnaker/igor/gcb/GoogleCloudBuildTest.java +++ b/igor-web/src/test/groovy/com/netflix/spinnaker/igor/gcb/GoogleCloudBuildTest.java @@ -53,20 +53,19 @@ import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.context.TestConfiguration; +import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.ComponentScan; import org.springframework.core.annotation.Order; import org.springframework.http.MediaType; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; -import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; +import org.springframework.security.web.SecurityFilterChain; import org.springframework.test.context.TestPropertySource; import org.springframework.test.context.junit.jupiter.SpringExtension; import org.springframework.test.web.servlet.MockMvc; -import org.springframework.web.servlet.config.annotation.EnableWebMvc; @ExtendWith(SpringExtension.class) @AutoConfigureMockMvc -@EnableWebMvc @ComponentScan({"com.netflix.spinnaker.config", "com.netflix.spinnaker.igor"}) @SpringBootTest( classes = { @@ -79,7 +78,8 @@ @TestPropertySource( properties = { "spring.config.location=classpath:gcb/gcb-test.yml", - "spring.application.name = igor" + "spring.application.name = igor", + "spring.mvc.pathmatch.matching-strategy = ANT_PATH_MATCHER" }) public class GoogleCloudBuildTest { @Autowired private MockMvc mockMvc; @@ -93,10 +93,11 @@ public class GoogleCloudBuildTest { @TestConfiguration @EnableWebSecurity @Order(1) - static class WebSecurityConfig extends WebSecurityConfigurerAdapter { - @Override - protected void configure(HttpSecurity httpSecurity) throws Exception { - httpSecurity.authorizeRequests().anyRequest().permitAll().and().csrf().disable(); + static class WebSecurityConfig { + @Bean + public SecurityFilterChain filterChain(HttpSecurity http) throws Exception { + http.authorizeHttpRequests().anyRequest().permitAll().and().csrf().disable(); + return http.build(); } }