Skip to content

Java: Fix SpringRequestMappingMethod URL Extraction #2 - #19556

Merged
owen-mc merged 6 commits into
github:mainfrom
owen-mc:java/pr/19512
May 22, 2025
Merged

Java: Fix SpringRequestMappingMethod URL Extraction #2#19556
owen-mc merged 6 commits into
github:mainfrom
owen-mc:java/pr/19512

Conversation

@owen-mc

Copy link
Copy Markdown
Contributor

This supersedes #19512.

CopilotAI 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 ownerMay 22, 2025 11:30

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

FileDescription
java/ql/test/library-tests/frameworks/spring/controller/Test.javaInserted RequestMappingURL tags on mapping methods and added a multi-value test class
java/ql/test/library-tests/frameworks/spring/controller/RequestController.qlNew test module defining expectations for RequestMappingURL tags
java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qllDeprecated getValue(), introduced getAValue() method
java/ql/lib/change-notes/2025-05-22-spring-request-mapping-value.mdDocumented 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. */

CopilotAIMay 22, 2025

Copy link

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. */
/**
*Getsa single "value"from the @RequestMapping annotation array string, if present.
*Ifthe annotation specifies multiple values,this method retrieves one of them.
*/

Copilot uses AI. Check for mistakes.
@owen-mcowen-mc changed the title Java: Fix SpringRequestMappingMethod URL ExtractionJava: Fix SpringRequestMappingMethod URL Extraction #2May 22, 2025
michaelnebel
michaelnebel previously approved these changes May 22, 2025

@michaelnebelmichaelnebel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@michaelnebelmichaelnebel left a comment

Copy link
Copy Markdown
Contributor

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:mainMay 22, 2025
@owen-mc
owen-mc deleted the java/pr/19512 branch May 22, 2025 14:08
Sign up for freeto 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.

4 participants

@owen-mc@michaelnebel@sentient0being