Skip to content

Fix proxy attribute name mismatch in RetryOperationsInterceptor #490

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

Conversation

junhyeongkim2
Copy link
Contributor

  • Changed removeAttribute to use the same key as setAttribute("proxy").
  • Fixed an issue where a proxy reference remained in the context due to the previous "proxy" key mismatch.

Found a small bug: I might be mistaken, but I think the proxy key mismatch is preventing removal. Please have your team take a look when you have a moment. Thanks!

@fengkongling
Copy link

fengkongling commented May 7, 2025 via email

@gaofengIt
Copy link

gaofengIt commented May 7, 2025 via email

@junhyeongkim2 junhyeongkim2 force-pushed the fix-proxy-attribute-mismatch branch from d1c2a7c to 57f5480 Compare May 7, 2025 07:05
@artembilan artembilan added this to the 2.0.12 milestone May 7, 2025
@artembilan artembilan added the bug label May 7, 2025
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Please, consider to use your legal name for Git client.
That Signed-off-by: junhyeongkim2 <ggprgrkjh@naver.com> is not a official name and we cannot accept such a contribution.

See more about DCO: https://en.wikipedia.org/wiki/Developer_Certificate_of_Origin

@junhyeongkim2 junhyeongkim2 force-pushed the fix-proxy-attribute-mismatch branch from 57f5480 to 0bb8e1a Compare May 7, 2025 15:00
@junhyeongkim2
Copy link
Contributor Author

I've updated the DCO signature with my legal name as requested. Thank you for your guidance!

Signed-off-by: Kim Jun Hyeong <ggprgrkjh@naver.com>
@junhyeongkim2 junhyeongkim2 force-pushed the fix-proxy-attribute-mismatch branch from 0bb8e1a to 44dd551 Compare May 7, 2025 15:02
@junhyeongkim2
Copy link
Contributor Author

I've removed the duplicate DCO signatures and kept only my legal name version. The issue is now resolved.

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

I wonder if that would be possible to cover your fix with some unit test.
Apparently we don't have one since this was wrong there for years.
Thanks

@junhyeongkim2
Copy link
Contributor Author

I appreciate the opportunity. I'll create a unit test for this fix and send it over for review.

Signed-off-by: Kim Jun Hyeong <ggprgrkjh@naver.com>
Signed-off-by: Kim Jun Hyeong <ggprgrkjh@naver.com>
@junhyeongkim2 junhyeongkim2 force-pushed the fix-proxy-attribute-mismatch branch from deb7c99 to 606eacc Compare May 7, 2025 19:12
@junhyeongkim2
Copy link
Contributor Author

junhyeongkim2 commented May 7, 2025

Added tests for RetryOperationsInterceptor's proxy attribute cleanup:

Verifies correct removal with proper key (" _ _ _ proxy _ _ _ ")
Confirms attribute persists when using wrong key (" _ _ proxy _ _ ")
Ensures cleanup works even when exceptions occur

I’m still learning, so I’d really appreciate any feedback or suggestions. thanks!

Signed-off-by: Kim Jun Hyeong <ggprgrkjh@naver.com>
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Please, also update Copyright of the affected classes to current 2025.
Thanks

RetrySynchronizationManager.register(context);
context.setAttribute("___proxy___", mockProxy);
assertThat(context.getAttribute("___proxy___")).isNotNull();
assertThatIllegalStateException().isThrownBy(() -> this.interceptor.invoke(new MethodInvocation() {
Copy link
Member

Choose a reason for hiding this comment

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

Please, consider to extract a MethodInvocation into a variable before this assertion.
After that the assertion sentence would be much easier to read.
I also don't see a reason to have the whole interface implementation.
We probably can simply use a mock() and then stub that getMethod().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback! I’ve applied all of your suggestions. Please let me know if you’d like any further adjustments.

…lify mocking

Signed-off-by: Kim Jun Hyeong <ggprgrkjh@naver.com>
@junhyeongkim2 junhyeongkim2 force-pushed the fix-proxy-attribute-mismatch branch from c93e9e6 to be5bc04 Compare May 8, 2025 08:31
@artembilan artembilan merged commit 936e720 into spring-projects:main May 8, 2025
2 checks passed
@artembilan
Copy link
Member

@junhyeongkim2 ,

thank you very much for the contribution; looking forward for more!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants