Skip to content

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

Merged
merged 6 commits into from
May 22, 2025

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented May 22, 2025

This supersedes #19512.

@Copilot Copilot AI review requested due to automatic review settings May 22, 2025 11:30
@owen-mc owen-mc requested a review from a team as a code owner May 22, 2025 11:30
Copy link
Contributor

@Copilot Copilot AI left a 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 add getAValue() 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 like getValues() or getValueArray() 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 a sink(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. */
Copy link
Preview

Copilot AI May 22, 2025

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).

Suggested change
/** 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.

@owen-mc owen-mc changed the title Java: Fix SpringRequestMappingMethod URL Extraction Java: Fix SpringRequestMappingMethod URL Extraction #2 May 22, 2025
michaelnebel
michaelnebel previously approved these changes May 22, 2025
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@owen-mc owen-mc merged commit 663c83d into github:main May 22, 2025
16 checks passed
@owen-mc owen-mc deleted the java/pr/19512 branch May 22, 2025 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants