-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Java: Fix SpringRequestMappingMethod URL Extraction #2 #19556
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances SpringRequestMappingMethod’s handling of @RequestMapping
values by deprecating the old single-value API, introducing a new array-based API, and updating tests and change notes accordingly.
- Deprecate
getValue()
and addgetAValue()
for multi-value mappings - Update Java test cases to assert
RequestMappingURL
tags, including a new multi-value test - Add change note for deprecation and new API
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
java/ql/test/library-tests/frameworks/spring/controller/Test.java | Inserted RequestMappingURL tags on mapping methods and added a multi-value test class |
java/ql/test/library-tests/frameworks/spring/controller/RequestController.ql | New test module defining expectations for RequestMappingURL tags |
java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll | Deprecated getValue() , introduced getAValue() method |
java/ql/lib/change-notes/2025-05-22-spring-request-mapping-value.md | Documented the deprecation of getValue() in change notes |
Comments suppressed due to low confidence (2)
java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll:160
- [nitpick] The method name
getAValue()
is not very descriptive. Consider renaming it to something likegetValues()
orgetValueArray()
to more clearly indicate that it returns multiple mapping values.
string getAValue() { result = requestMappingAnnotation.getAStringArrayValue("value") }
java/ql/test/library-tests/frameworks/spring/controller/Test.java:208
- The new multi-value test method does not invoke
sink()
, unlike other tests. Adding asink(src)
call inside this method would ensure consistency and make it clear that this endpoint is being exercised.
public void get(WebRequest src) { // $ RequestMappingURL="/a" RequestMappingURL="/b"
/** DEPRECATED: Use `getAValue()` instead. */ | ||
deprecated string getValue() { result = requestMappingAnnotation.getStringValue("value") } | ||
|
||
/** Gets the "value" @RequestMapping annotation array string value, if present. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The doc comment for getAValue()
could clarify the format of the returned string when multiple values are present (e.g., comma-separated).
/** Gets the "value" @RequestMapping annotation array string value, if present. */ | |
/** | |
* Gets a single "value" from the @RequestMapping annotation array string, if present. | |
* If the annotation specifies multiple values, this method retrieves one of them. | |
*/ |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This supersedes #19512.