Skip to content

Commit

Permalink
refactor(tests): replace AntPathMatcher as path matching strategy ins…
Browse files Browse the repository at this point in the history 
…tead 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
spring-projects/spring-framework#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.<init>(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.<init>(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: spinnaker#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.
  • Loading branch information
@j-sandy
j-sandy committed Jan 10, 2025
1 parent 0a0c5e8 commit fe35662
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 60 deletions.
83 changes: 31 additions & 52 deletions igor-web/src/test/groovy/com/netflix/spinnaker/igor/build/BuildControllerSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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'
Expand All @@ -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'() {
Expand Down Expand Up @@ -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)

Expand All @@ -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)

Expand All @@ -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:
Expand All @@ -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"])
Expand All @@ -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."
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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:
Expand Down
17 changes: 9 additions & 8 deletions igor-web/src/test/groovy/com/netflix/spinnaker/igor/gcb/GoogleCloudBuildTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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;
Expand All @@ -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();
}
}

Expand Down

0 comments on commit fe35662

Please sign in to comment.