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.

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

refactor(web): replace the path matching strategy for spring mvc from PathPatternParser to Ant Matcher during upgrade to spring boot 2.6.x #1211

Closed
wants to merge 5 commits into from

Conversation

j-sandy
Copy link
Contributor

@j-sandy j-sandy commented Jan 5, 2024

In spring boot 2.6.x default strategy for matching request paths against registered Spring MVC handler mappings has changed from AntPathMatcher to PathPatternParser here. This causes an incompatibility of specific pattern like /abc/**/xyz and PathPattenParser does not resolve the path. The compatibility difference is given here. Encountered below error in igor-web module for this mapping /masters/{name}/jobs/**/update/{buildNumber} while test execution.

Failed to load ApplicationContext
java.lang.IllegalStateException: Failed to load ApplicationContext
	at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContext(DefaultCacheAwareContextLoaderDelegate.java:98)
	at org.springframework.test.context.support.DefaultTestContext.getApplicationContext(DefaultTestContext.java:124)
	at org.springframework.test.context.web.ServletTestExecutionListener.setUpRequestContextIfNecessary(ServletTestExecutionListener.java:190)
	at org.springframework.test.context.web.ServletTestExecutionListener.prepareTestInstance(ServletTestExecutionListener.java:132)
	at org.springframework.test.context.TestContextManager.prepareTestInstance(TestContextManager.java:248)
	at org.springframework.test.context.junit.jupiter.SpringExtension.postProcessTestInstance(SpringExtension.java:138)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeTestInstancePostProcessors$8(ClassBasedTestDescriptor.java:363)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.executeAndMaskThrowable(ClassBasedTestDescriptor.java:368)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeTestInstancePostProcessors$9(ClassBasedTestDescriptor.java:363)
	...
Caused by: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'requestMappingHandlerMapping' defined in class path resource [org/springframework/boot/autoconfigure/web/servlet/WebMvcAutoConfiguration$EnableWebMvcConfiguration.class]: Invocation of init method failed; nested exception is 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,javax.servlet.http.HttpServletRequest)
	at app//org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1804)
	at app//org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:620)
	at app//org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:542)
	at app//org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:335)
	...
Caused by: 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,javax.servlet.http.HttpServletRequest)
	at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.lambda$detectHandlerMethods$1(AbstractHandlerMethodMapping.java:288)
	at org.springframework.core.MethodIntrospector.lambda$selectMethods$0(MethodIntrospector.java:74)
	at org.springframework.util.ReflectionUtils.doWithMethods(ReflectionUtils.java:367)
	at org.springframework.core.MethodIntrospector.selectMethods(MethodIntrospector.java:72)
	at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.detectHandlerMethods(AbstractHandlerMethodMapping.java:281)
	...
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:110)
	at app//org.springframework.web.servlet.mvc.condition.PathPatternsRequestCondition.parse(PathPatternsRequestCondition.java:82)
	at app//org.springframework.web.servlet.mvc.condition.PathPatternsRequestCondition.<init>(PathPatternsRequestCondition.java:70)
	at app//org.springframework.web.servlet.mvc.method.RequestMappingInfo$DefaultBuilder.build(RequestMappingInfo.java:712)
	at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.createRequestMappingInfo(RequestMappingHandlerMapping.java:379)
	at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.createRequestMappingInfo(RequestMappingHandlerMapping.java:324)
	at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.getMappingForMethod(RequestMappingHandlerMapping.java:284)
	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:284)
	... 112 more

To fix this issue we can add a spring property spring.mvc.pathmatch.matching-strategy = ANT_PATH_MATCHER to kork-config so that it will be applicable to all the spinnaker components. But there is only 1 incompatible pattern available in igor and in rest of the components all patterns are compatible with PathPatternParser. So updating this property only in igor.

… PathPatternParser to Ant Matcher during upgrade to spring boot 2.6.x

In spring boot 2.6.x default strategy for matching request paths against registered Spring MVC handler mappings has changed from AntPathMatcher to PathPatternParser [here](https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-2.6-Release-Notes#pathpattern-based-path-matching-strategy-for-spring-mvc). This causes an incompatibility of specific pattern like `/abc/**/xyz` and `PathPattenParser` does not resolve the path. The compatibility difference is given [here](https://spring.io/blog/2020/06/30/url-matching-with-pathpattern-in-spring-mvc/#pathpattern). Encountered below error in igor-web module for this mapping `/masters/{name}/jobs/**/update/{buildNumber}` while test execution.
```
Failed to load ApplicationContext
java.lang.IllegalStateException: Failed to load ApplicationContext
	at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContext(DefaultCacheAwareContextLoaderDelegate.java:98)
	at org.springframework.test.context.support.DefaultTestContext.getApplicationContext(DefaultTestContext.java:124)
	at org.springframework.test.context.web.ServletTestExecutionListener.setUpRequestContextIfNecessary(ServletTestExecutionListener.java:190)
	at org.springframework.test.context.web.ServletTestExecutionListener.prepareTestInstance(ServletTestExecutionListener.java:132)
	at org.springframework.test.context.TestContextManager.prepareTestInstance(TestContextManager.java:248)
	at org.springframework.test.context.junit.jupiter.SpringExtension.postProcessTestInstance(SpringExtension.java:138)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeTestInstancePostProcessors$8(ClassBasedTestDescriptor.java:363)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.executeAndMaskThrowable(ClassBasedTestDescriptor.java:368)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeTestInstancePostProcessors$9(ClassBasedTestDescriptor.java:363)
	...
Caused by: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'requestMappingHandlerMapping' defined in class path resource [org/springframework/boot/autoconfigure/web/servlet/WebMvcAutoConfiguration$EnableWebMvcConfiguration.class]: Invocation of init method failed; nested exception is 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,javax.servlet.http.HttpServletRequest)
	at app//org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1804)
	at app//org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:620)
	at app//org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:542)
	at app//org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:335)
	...
Caused by: 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,javax.servlet.http.HttpServletRequest)
	at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.lambda$detectHandlerMethods$1(AbstractHandlerMethodMapping.java:288)
	at org.springframework.core.MethodIntrospector.lambda$selectMethods$0(MethodIntrospector.java:74)
	at org.springframework.util.ReflectionUtils.doWithMethods(ReflectionUtils.java:367)
	at org.springframework.core.MethodIntrospector.selectMethods(MethodIntrospector.java:72)
	at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.detectHandlerMethods(AbstractHandlerMethodMapping.java:281)
	...
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:110)
	at app//org.springframework.web.servlet.mvc.condition.PathPatternsRequestCondition.parse(PathPatternsRequestCondition.java:82)
	at app//org.springframework.web.servlet.mvc.condition.PathPatternsRequestCondition.<init>(PathPatternsRequestCondition.java:70)
	at app//org.springframework.web.servlet.mvc.method.RequestMappingInfo$DefaultBuilder.build(RequestMappingInfo.java:712)
	at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.createRequestMappingInfo(RequestMappingHandlerMapping.java:379)
	at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.createRequestMappingInfo(RequestMappingHandlerMapping.java:324)
	at app//org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.getMappingForMethod(RequestMappingHandlerMapping.java:284)
	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:284)
	... 112 more
```
To fix this issue we can add a spring property `spring.mvc.pathmatch.matching-strategy = ANT_PATH_MATCHER` to kork-config so that it will be applicable to all the spinnaker components. But there is only 1 incompatible pattern available in [igor](https://github.com/spinnaker/igor/blob/773759404425b56d9f27a96ab3470d93d2983f4e/igor-web/src/main/groovy/com/netflix/spinnaker/igor/build/BuildController.groovy#L201) and in rest of the components all patterns are compatible with `PathPatternParser`. So updating this property only in igor.
@j-sandy
Copy link
Contributor Author

j-sandy commented Jan 8, 2024

Following are the tests get affected with path matching strategy:

./gradlew :igor-web:test --tests "com.netflix.spinnaker.igor.build.BuildControllerSpec.updates a jenkins build description" --no-build-cache
./gradlew :igor-web:test --tests "com.netflix.spinnaker.igor.MainTest"

Error starting ApplicationContext. To display the conditions report re-run your application with 'debug' enabled.
2024-01-08 16:50:21.895 ERROR 290512 --- [    Test worker] o.s.b.d.LoggingFailureAnalysisReporter   : 

***************************
APPLICATION FAILED TO START
***************************

Description:

Invalid mapping pattern detected: /masters/{name}/jobs/**/update/{buildNumber}
                    ^
No more pattern data allowed after {*...} or ** pattern element

Action:

Fix this pattern in your application or switch to the legacy parser implementation with 'spring.mvc.pathmatch.matching-strategy=ant_path_matcher'.

2024-01-08 16:50:21.902 ERROR 290512 --- [    Test worker] o.s.test.context.TestContextManager      : Caught exception while allowing TestExecutionListener [org.springframework.test.context.web.ServletTestExecutionListener@1f0b2ffb] to prepare test instance [com.netflix.spinnaker.igor.MainTest@3c546124]

java.lang.IllegalStateException: Failed to load ApplicationContext
	at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContext(DefaultCacheAwareContextLoaderDelegate.java:98) ~[spring-test-5.3.27.jar:5.3.27]
	at org.springframework.test.context.support.DefaultTestContext.getApplicationContext(DefaultTestContext.java:124) ~[spring-test-5.3.27.jar:5.3.27]
./gradlew :igor-web:test --tests "com.netflix.spinnaker.igor.artifacts.ArtifactExtractorTest.should_be_able_to_serialize_jsr310_dates"

Error starting ApplicationContext. To display the conditions report re-run your application with 'debug' enabled.
2024-01-08 16:55:34.685 ERROR 304436 --- [    Test worker] o.s.b.d.LoggingFailureAnalysisReporter   : 

***************************
APPLICATION FAILED TO START
***************************

Description:

Invalid mapping pattern detected: /masters/{name}/jobs/**/update/{buildNumber}
                    ^
No more pattern data allowed after {*...} or ** pattern element

Action:

Fix this pattern in your application or switch to the legacy parser implementation with 'spring.mvc.pathmatch.matching-strategy=ant_path_matcher'.

2024-01-08 16:55:34.699 ERROR 304436 --- [    Test worker] o.s.test.context.TestContextManager      : Caught exception while allowing TestExecutionListener [org.springframework.test.context.web.ServletTestExecutionListener@6f89f665] to prepare test instance [com.netflix.spinnaker.igor.artifacts.ArtifactExtractorTest@358ebd61]

java.lang.IllegalStateException: Failed to load ApplicationContext
	at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContext(DefaultCacheAwareContextLoaderDelegate.java:98) ~[spring-test-5.3.27.jar:5.3.27]
	at org.springframework.test.context.support.DefaultTestContext.getApplicationContext(DefaultTestContext.java:124) ~[spring-test-5.3.27.jar:5.3.27]

@j-sandy j-sandy marked this pull request as draft January 8, 2024 17:05
@j-sandy
Copy link
Contributor Author

j-sandy commented Feb 27, 2024

@Mergifyio update

Copy link
Contributor

mergify bot commented Feb 27, 2024

update

✅ Branch has been successfully updated

@j-sandy j-sandy marked this pull request as ready for review February 27, 2024 19:10
@j-sandy
Copy link
Contributor Author

j-sandy commented Mar 11, 2024

@Mergifyio update

Copy link
Contributor

mergify bot commented Mar 11, 2024

update

✅ Branch has been successfully updated

@dbyron-sf
Copy link
Contributor

Included in #1238.

@dbyron-sf dbyron-sf closed this Mar 12, 2024
@j-sandy j-sandy deleted the pathmatch branch March 15, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants