Skip to content
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

IBX-8784: Added AbstractExceptionVisitor template to allow fine-grained control of output #128

Merged
merged 5 commits into from
Oct 11, 2024

Conversation

Steveb-p
Copy link
Contributor

🎫 Issue IBX-8784

Description:

This PR enables child Exception visitors to more easily control exception message and description presented by REST endpoints.

For QA:

Documentation:

konradoboza
konradoboza previously approved these changes Aug 29, 2024
ciastektk
ciastektk previously approved these changes Aug 29, 2024
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

What's an example response after the change?

@Steveb-p
Copy link
Contributor Author

What's an example response after the change?

No changes for the built-in exceptions. It's used as a template visitor for external packages to re-use code for error handling data shape.

What I consider currently:

  • Extract parts of this class into an abstract template, and move it into Contracts
  • Current visitor design does not work for interfaces (only concrete classes) which limits external use a bit.

I'll provide a separate PR for relevant package in a few hours.

alongosz
alongosz previously approved these changes Aug 30, 2024
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

What's an example response after the change?

No changes for the built-in exceptions. It's used as a template visitor for external packages to re-use code for error handling data shape.

What I consider currently:

  • Extract parts of this class into an abstract template, and move it into Contracts
  • Current visitor design does not work for interfaces (only concrete classes) which limits external use a bit.

I'll provide a separate PR for relevant package in a few hours.

Ok. +1 for now. I think when I see usage it will be more clear to me. Do you think this approach should replace #63 or is it rather more about "how" to display specific exception instead of translating one exception into another?

@Steveb-p
Copy link
Contributor Author

Steveb-p commented Aug 30, 2024

@alongosz

Do you think this approach should replace #63 or is it rather more about "how" to display specific exception instead of translating one exception into another?

This PR doesn't change anything related to REST / pure API object handling.

As for the mapping provided by #63, I don't think it's the responsibility of the service that provides the response. As you noted, we have different practices within the same package.

The only reason you provide this mapping is because there is a chance that REST endpoint will forget to map it itself. So I'd just put your solution as a separate event listener with higher priority. This way it would have a separate purpose and clean responsibility.

@Steveb-p Steveb-p changed the title IBX-8784: Added getErrorMessage and getErrorDescription to Exception REST visitor IBX-8784: Added AbstractExceptionVisitor template to allow fine-grained control of output Aug 30, 2024
@Steveb-p Steveb-p requested a review from alongosz August 30, 2024 10:09
@Steveb-p Steveb-p dismissed alongosz’s stale review August 30, 2024 10:10

Substantial changes to PR code

Copy link

sonarcloud bot commented Aug 30, 2024

@Steveb-p Steveb-p dismissed stale reviews from ciastektk and konradoboza August 30, 2024 10:10

Substantial changes to PR code

@Steveb-p Steveb-p requested review from ciastektk, konradoboza and a team August 30, 2024 10:10
@konradoboza konradoboza requested a review from a team August 30, 2024 10:49
@barbaragr barbaragr self-assigned this Oct 8, 2024
@dew326 dew326 force-pushed the ibx-8784/exception-message-extension branch from e7a32d0 to 2bc129a Compare October 11, 2024 07:51
@Steveb-p Steveb-p force-pushed the ibx-8784/exception-message-extension branch from 2bc129a to 59e5424 Compare October 11, 2024 08:59
Copy link

sonarcloud bot commented Oct 11, 2024

@Steveb-p Steveb-p merged commit fd2cbf5 into 4.6 Oct 11, 2024
11 checks passed
@Steveb-p Steveb-p deleted the ibx-8784/exception-message-extension branch October 11, 2024 09:22
Sign up for free to 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.

7 participants