Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
This repository was archived by the owner on Oct 10, 2025. It is now read-only.

Allow classes without the proper interface to still work on JDK < 16#19

Merged
mcnoreau12 merged 2 commits intomaincoveooss/feign-error-decoder:mainfrom
feature/retro-compatibilitycoveooss/feign-error-decoder:feature/retro-compatibilityCopy head branch name to clipboard
Jul 15, 2022
Merged

Allow classes without the proper interface to still work on JDK < 16#19
mcnoreau12 merged 2 commits intomaincoveooss/feign-error-decoder:mainfrom
feature/retro-compatibilitycoveooss/feign-error-decoder:feature/retro-compatibilityCopy head branch name to clipboard

Conversation

@AndyCoveo
Copy link
Contributor

To facilitate migration for people transitionning from the version 1 to version 2, the mechanism to set the detail message reflectively has been added back as a fallback mechanism. This fallback mechanism is only available while running on JDK < 16.

@@ -0,0 +1,30 @@
package com.coveo.feign;

public abstract class BaseServiceException extends Exception {
Copy link

Choose a reason for hiding this comment

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

why did you add this exception type ? It would have worked with current ServiceException no ? just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed this exception to test checked exception thrown that did not implement the interface. Since the exceptionDecode requires a base class, I added an extra layer without the interface.

Another solution would be to have a different ErrorDecoder with another exception class hierarchy.

I have no preferences and I am open to suggestion.

detailMessageField = Throwable.class.getDeclaredField("detailMessage");
detailMessageField.setAccessible(true);
} catch (Exception e) {
logger.debug("Unable to directly set the detailMessage, make sure exception do implement {}", baseExceptionClass.getName());
Copy link

Choose a reason for hiding this comment

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

i'm not sure debug is enough, while you fixed it so it wont break now, it will break then, and I think debug is not enough in this case

Copy link

Choose a reason for hiding this comment

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

nvm we just discussed the fact that we now have 2 messages, the warning is still here

@mcnoreau12 mcnoreau12 self-requested a review July 14, 2022 13:52
src/main/java/com/coveo/feign/ReflectionErrorDecoder.java Outdated Show resolved Hide resolved
To facilitate migration for people transitionning from the version 1 to version 2, the mechanism to set the detail message reflectively has been added back as a fallback mechanism. **This fallback mechanism is only available while running on JDK < 16**.
@AndyCoveo AndyCoveo force-pushed the feature/retro-compatibility branch from 2da0327 to 616d938 Compare July 14, 2022 14:10
Copy link
Collaborator

@mcnoreau12 mcnoreau12 left a comment

Choose a reason for hiding this comment

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

thanks! gg

@mcnoreau12 mcnoreau12 merged commit 28fa8fb into main Jul 15, 2022
@mcnoreau12 mcnoreau12 deleted the feature/retro-compatibility branch July 15, 2022 18:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments

Close sidebar
Morty Proxy This is a proxified and sanitized view of the page, visit original site.