Skip to content

Conversation

vy
Copy link
Contributor

@vy vy commented Mar 7, 2023

Removes final modifier from ProblemDetailsExceptionHandler implementations to allow proxying these beans. See #34426 for the issue.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 7, 2023
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 15, 2023
@philwebb philwebb added this to the 3.1.x milestone Mar 15, 2023
@philwebb philwebb changed the title Remove final from ProblemDetailsExceptionHandlers to allow proxying Remove final from ProblemDetailsExceptionHandler classes to allow proxying Mar 15, 2023
@philwebb
Copy link
Member

I wonder if we should add some tests just to stop final sneaking back in the future?

@philwebb philwebb added the for: merge-with-amendments Needs some changes when we merge label Mar 15, 2023
@vy
Copy link
Contributor Author

vy commented Mar 15, 2023

@philwebb, sounds like a good idea.

I first thought of adding a @SpringBootTest similar to the one shared in #34426. That requires the spring-boot-starter-aop module, though the classes to be tested are located in spring-boot-autoconfigure. Hence, I settled on ArchUnit tests in s-b-autoconfigure module. Note that while both test classes look identical, they are not due to the scanned packages and the visibility of ProblemDetailsExceptionHandler interfaces.

@philwebb
Copy link
Member

Thanks @vy! Rather than a new dependency we might change this to use standard reflection when we merge it.

@vy
Copy link
Contributor Author

vy commented Mar 15, 2023

@philwebb, ArchUnit was already used in TaskConfigurationAvoidanceTests, hence was my preference for it. But I can replace it with reflection, if that is what you want.

If you prefer reflection, please also let me know if you want to target ProblemDetailsExceptionHandler interfaces or ResponseEntityExceptionHandler subclasses, since the latter will require some stunt to pull.

@philwebb
Copy link
Member

Ahh, I missed that we're already using it. No worries then. We'll take a judgement call when we merge it but don't worry about updating it.

@wilkinsona wilkinsona self-assigned this Mar 23, 2023
@wilkinsona wilkinsona modified the milestones: 3.1.x, 3.1.0-M2 Mar 23, 2023
@wilkinsona
Copy link
Member

Thanks very much for the PR, @vy. I decided to test the changes by verifying that the exception handlers could be proxied. If you're interested, please see 1eb5bbe for details.

@vy vy deleted the problem-handler-proxy-fix branch March 24, 2023 08:13
@vy
Copy link
Contributor Author

vy commented Mar 24, 2023

@wilkinsona, thanks so much for the prompt processing of the ticket and keeping me in the loop. 🙇 I didn't know about AopUtils.isCglibProxy, which makes your tests precisely demonstrative of the bug reported in the first place. 💯

@sbrannen
Copy link
Member

@wilkinsona, at a glance it appears that those two assertThat(AopUtils.isCglibProxy(context.getBean(ProblemDetailsExceptionHandler.class))); assertions are missing .isTrue().

wilkinsona added a commit that referenced this pull request Mar 24, 2023
@wilkinsona
Copy link
Member

Good catch, Sam. Thank you. Corrected in b91f814.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants