Skip to content

Conversation

@mpritham
Copy link
Contributor

@mpritham mpritham commented Jul 14, 2025

Before this PR

When #1352 was added, the test utilities to assert properties on the EndpointServiceException was not added.

After this PR

This PR introduces a common abstract base class AbstractServiceError which both ServiceException and EndpointServiceException extend. It also modifies ServiceExceptionAssert to use AbstractServiceError instead of ServiceException in the template variable.

==COMMIT_MSG==
Add AbstractServiceException and extend ServiceExceptionAssert to support EndpointServiceException
==COMMIT_MSG==

Possible downsides?

We are making AbstractServiceError public so we can use it in the ServiceExceptionAssert which lives in a different package. Ideally, AbstractServiceError will not be public. A warning not to use the class was added to the docstring.

@changelog-app
Copy link

changelog-app bot commented Jul 14, 2025

Generate changelog in changelog/@unreleased

Type (Select exactly one)

  • Feature (Adding new functionality)
  • Improvement (Improving existing functionality)
  • Fix (Fixing an issue with existing functionality)
  • Break (Creating a new major version by breaking public APIs)
  • Deprecation (Removing functionality in a non-breaking way)
  • Migration (Automatically moving data/functionality to a new system)

Description

Add AbstractServiceException and extend ServiceExceptionAssert to support EndpointServiceException

Check the box to generate changelog(s)

  • Generate changelog entry

kunalrkak
kunalrkak previously approved these changes Jul 14, 2025
aldexis
aldexis previously approved these changes Jul 15, 2025
Copy link
Contributor

@aldexis aldexis left a comment

Choose a reason for hiding this comment

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

I'm fairly sure this will not cause ABI breaks (we might want to add revapi if we don't already have it?), however reverting this change (if we end up reconsidering) would be one

/**
* This class should not be used directly. The parent class for ServiceException and EndpointServiceException.
*/
public abstract class AbstractServiceError extends RuntimeException implements SafeLoggable {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could we name this AbstractServiceException since it's not an Error?

Suggested change
public abstract class AbstractServiceError extends RuntimeException implements SafeLoggable {
public abstract class AbstractServiceException extends RuntimeException implements SafeLoggable {

Comment on lines 25 to 29
public abstract ErrorType getErrorType();

protected AbstractServiceError(Throwable cause) {
super(cause);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not share more implementation than this between the two existing exceptions? Or is the goal to share the minimum to start?

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 wanted to make it minimal given I'm making this public and don't want people to use this. We only need the error type to update the ServiceExceptionAssert.

@policy-bot policy-bot bot dismissed stale reviews from kunalrkak and aldexis July 15, 2025 15:25

Invalidated by push of 83da95f

@mpritham mpritham changed the title Add AbstractServiceError and extend ServiceExceptionAssert to support EndpointServiceException Add AbstractServiceException and extend ServiceExceptionAssert to support EndpointServiceException Jul 17, 2025
@stale
Copy link

stale bot commented Oct 18, 2025

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

@stale stale bot added the stale label Oct 18, 2025
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.

6 participants