Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Inconsistent discovery of parameter names for selectors in custom actuator endpoints #31240

Closed
ahubold opened this issue Jun 2, 2022 · 1 comment
Assignees
Labels
type: bug A general bug
Milestone

Comments

@ahubold
Copy link

ahubold commented Jun 2, 2022

Version is Spring Boot 2.6.7

I've seen the documentation recommending that "Java code that implements an endpoint should be compiled with -parameters". It says "should", not "must", and projects may have reasons not to do that, see for example https://stackoverflow.com/a/44075684/2086307.

If you don't compile with -parameters, a read operation like the following does not work when invoked by HTTP:

@ReadOperation
public Result operation(@Selector String name) {
org.springframework.boot.actuate.endpoint.invoke.MissingParametersException: 
Failed to invoke operation because the following required parameters were missing: name of type java.lang.String
	at org.springframework.boot.actuate.endpoint.invoke.reflect.ReflectiveOperationInvoker.validateRequiredParameters(ReflectiveOperationInvoker.java:81)
	at org.springframework.boot.actuate.endpoint.invoke.reflect.ReflectiveOperationInvoker.invoke(ReflectiveOperationInvoker.java:70)
	at org.springframework.boot.actuate.endpoint.annotation.AbstractDiscoveredOperation.invoke(AbstractDiscoveredOperation.java:60)
	at org.springframework.boot.actuate.endpoint.web.servlet.AbstractWebMvcEndpointHandlerMapping$ServletWebOperationAdapter.handle(AbstractWebMvcEndpointHandlerMapping.java:353)

Interestingly, Spring Boot is still able to get the parameter name as the above exception message shows: "name of type java.lang.String"

This is because org.springframework.boot.actuate.endpoint.invoke.reflect.OperationMethod uses org.springframework.core.DefaultParameterNameDiscoverer, which can still retrieve parameter names even if the code wasn't compiled with -parameters. (It uses ASM in LocalVariableTableParameterNameDiscoverer). That's why the operation still expects an argument "name" in this example.

The problem here is that the code that supplies these arguments only uses java.lang.reflect.Parameter#getName, for example in org.springframework.boot.actuate.endpoint.web.annotation.RequestPredicateFactory#getPath and org.springframework.boot.actuate.endpoint.web.annotation.DiscoveredWebOperation#dashName. This leads to the synthesized argument name "arg0", which of course doesn't match the expected one "name".

This is inconsistent: The code to collect arguments from the request and the code that consumes these arguments use different strategies for naming if not compiled with -parameters. Either both should only uses synthesized "arg0"-like names, or both should also consider the more sophisticated approach using ASM.

Apart from compiling with -parameters, a workaround would be to rename the parameter to "arg0"

@ReadOperation
public Result operation(@Selector String arg0) { // DO NOT RENAME THE PARAMETER!

This is of course not very nice, and can easily break. It would be better if one could at least specify the argument name with an annotation, but I don't think that's possible?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 2, 2022
@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 6, 2022
@philwebb philwebb added this to the 2.6.x milestone Jun 6, 2022
@wilkinsona wilkinsona modified the milestones: 2.6.x, 2.7.x Nov 24, 2022
@mhalbritter mhalbritter self-assigned this Jan 16, 2023
@mhalbritter
Copy link
Contributor

mhalbritter commented Jan 16, 2023

Hey @ahubold, thanks for that great analysis and the nice write-up. Has really helped me to fix that bug!

@mhalbritter mhalbritter modified the milestones: 2.7.x, 2.7.8 Jan 16, 2023
izeye added a commit to izeye/spring-boot that referenced this issue Jan 19, 2023
izeye added a commit to izeye/spring-boot that referenced this issue Jan 19, 2023
krenson pushed a commit to krenson/test-push that referenced this issue Mar 15, 2023
…ot-starter-parent from 2.7.7 to 2.7.8 (patch)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [org.springframework.boot:spring-boot-starter-parent](https://spring.io/projects/spring-boot) ([source](https://github.com/spring-projects/spring-boot)) | parent | patch | `2.7.7` -> `2.7.8` |

---

### Release Notes

<details>
<summary>spring-projects/spring-boot</summary>

### [`v2.7.8`](https://github.com/spring-projects/spring-boot/releases/tag/v2.7.8)

[Compare Source](spring-projects/spring-boot@v2.7.7...v2.7.8)

#### ⭐ Noteworthy

-   The coordinates of the MySQL JDBC driver have [changed from `mysql:mysql-connector-java` to `com.mysql:mysql-connector-j`](https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-2.7-Release-Notes#mysql-jdbc-driver).

#### 🐞 Bug Fixes

-   Devtools sets non-existent property spring.reactor.debug [#&#8203;33858](spring-projects/spring-boot#33858)
-   Failing calls to reactive health indicators are not logged [#&#8203;33774](spring-projects/spring-boot#33774)
-   Failure analysis of NoUniqueBeanDefinitionException reports "defined in null" when bean definition has no resource description [#&#8203;33765](spring-projects/spring-boot#33765)
-   NPE in RabbitProperties when user is given, but password not [#&#8203;33752](spring-projects/spring-boot#33752)
-   SDKMAN should not use repo.spring.io for releases [#&#8203;33708](spring-projects/spring-boot#33708)
-   Homebrew and Scoop should not use repo.spring.io for releases [#&#8203;33702](spring-projects/spring-boot#33702)
-   EndpointRequestMatcher should have a toString method [#&#8203;33690](spring-projects/spring-boot#33690)
-   It is not possible to provide a custom TransactionProvider bean for JOOQ [#&#8203;32899](spring-projects/spring-boot#32899)
-   SpringBootMockResolver causes AopTestUtils.getUltimateTargetObject to recurse until the stack overflows when it calls it with Spring Security's authentication manager bean [#&#8203;32632](spring-projects/spring-boot#32632)
-   Inconsistent discovery of parameter names for selectors in custom actuator endpoints [#&#8203;31240](spring-projects/spring-boot#31240)
-   `@DeprecatedConfigurationProperty` has no effect when declared on a record component's accessor method [#&#8203;29526](spring-projects/spring-boot#29526)
-   Headless mode is forced when banner.\* file is present. [#&#8203;28803](spring-projects/spring-boot#28803)
-   Diagnostics are poor when the JMX port used by the Maven start goal is in use [#&#8203;24044](spring-projects/spring-boot#24044)

#### 📔 Documentation

-   Replace "via" in documentat...
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants