Create PersistenceException mapper #1181

DaniilRoman · 2025-03-15T18:12:38Z

Problem: Polaris Server exposes backend meta in error response body when database is no bootstraped
Solution: Create a separated jakarta.persistence.PersistenceException mapper

flyrain

Thanks @DaniilRoman for working on a fix. Left some comments.

flyrain · 2025-03-16T03:44:35Z

service/common/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java

@@ -191,6 +192,7 @@ static int mapExceptionToResponseCode(RuntimeException rex) {
      case IllegalArgumentException e -> Status.BAD_REQUEST.getStatusCode();
      case UnsupportedOperationException e -> Status.NOT_ACCEPTABLE.getStatusCode();
      case WebApplicationException e -> e.getResponse().getStatus();
+      case PersistenceException e -> Status.UNAUTHORIZED.getStatusCode();


This may not be enough to fix the issue, as line 105 to line 115 still leak the detailed database information.

Another concern is that, the error log has gone after the change as we changed the Status code to 401. I think we still need to log an error in the server side.

if (responseCode == Response.Status.INTERNAL_SERVER_ERROR.getStatusCode()) { LOGGER.error("Unhandled exception returning INTERNAL_SERVER_ERROR", runtimeException); }

Thank you for your feedback 🙏
This makes total sense. Instead of extending the existing mapper, I've created a new one where I set a message from PersistenceException but shouldn't include the information like Exception [EclipseLink-4002] (Eclipse Persistence Services - 4.0.4.v202407190748...

I've checked existing mappers and I see that we use a level INFO for every known exception I've chosen an INFO level as well instead of an ERROR level

dimas-b · 2025-03-17T21:23:32Z

service/common/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java

@@ -191,6 +192,7 @@ static int mapExceptionToResponseCode(RuntimeException rex) {
      case IllegalArgumentException e -> Status.BAD_REQUEST.getStatusCode();
      case UnsupportedOperationException e -> Status.NOT_ACCEPTABLE.getStatusCode();
      case WebApplicationException e -> e.getResponse().getStatus();
+      case PersistenceException e -> Status.UNAUTHORIZED.getStatusCode();


What's the rationale for mapping PersistenceException to UNAUTHORIZED? Are we sure all of those errors are caused by failed authorization checks?

Hmm, looks like 500 is more suitable for PersistenceException. The 401 status code is reserved for authentication failures, which isn't applicable here. We may only need to mask the sensitive information a bit.

Thank you for your feedback 🙏
I've adjusted this PR and changed the error code back to 500

dimas-b · 2025-03-21T03:51:40Z

...on/src/main/java/org/apache/polaris/service/exception/IcebergPersistenceExceptionMapper.java

+
+@Provider
+public class IcebergPersistenceExceptionMapper
+    implements ExceptionMapper<PersistenceException> {


I'm not sure creating a separate mapper just for PersistenceException is a good idea. IcebergExceptionMapper already deals with exception of multiple types. Could you integrate the new logic for PersistenceException there?

I think it's a right thing to use a different class, instead of putting it into IcebergExceptionMapper. I even think we may pull non-iceberg exception out of the class IcebergExceptionMapper, whose name indicates that it handles iceberg exceptions, but not other exceptions.

I'd suggest to rename it to PersistenceExceptionMapper.

I also think it's reasonable to create a separate mapper for PersistenceException. In IcebergExceptionMapper we treat any "500" exceptions like "unhandled" and I would need to create yet another "if else" for PersistenceException which would make IcebergExceptionMapper even more complicated

At the web layer exceptions may be wrapped in RuntimeException and other generic exception classes. There's usually logic to unwrap the root cause and process it depending on the specific type. Also, there's usually logic for logging interesting exceptions.

If more than one ExceptionMapper is used, provisions should be made to apply the same processing logic in all cases.

However, in this PR the exception processing logic is forked.

dimas-b · 2025-03-21T03:52:37Z

...on/src/main/java/org/apache/polaris/service/exception/IcebergPersistenceExceptionMapper.java

+    ErrorResponse icebergErrorResponse =
+        ErrorResponse.builder()
+            .responseCode(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode())
+            .withType(exception.getClass().getSimpleName())


This "type" is supposed to be interpreted by Iceberg REST API clients, but I do not think "PersistenceException" is going to be very meaningful on the client side.

Looks like the class IcebergExceptionMapper also use the exception simple name as the type. I'm also not sure if any iceberg rest client relies on the type. I think the status code is good enough for most of use cases. Here is the error message definition in Iceberg REST spec. Looks like the type isn't tied to the status code. I think it's OK to use the exception name, but open to suggestions.

ErrorModel: type: object description: JSON error payload returned in a response with further details on the error required: - message - type - code properties: message: type: string description: Human-readable error message type: type: string description: Internal type definition of the error example: NoSuchNamespaceException code: type: integer minimum: 400 maximum: 600 description: HTTP response code example: 404

Existing does not mean correct, eh? :)

Well, it's better than nothing... ideally, these exceptions should be caught and wrapped within the business logic before making it back to the client. They would be wrapped with something that the IcebergExceptionMapper or PolarisExceptionMapper can map to a client-friendly response. However in the event that we miss the wrapping, I can see how this additional mapper is useful.

dimas-b · 2025-03-21T03:53:31Z

...on/src/main/java/org/apache/polaris/service/exception/IcebergPersistenceExceptionMapper.java

+        ErrorResponse.builder()
+            .responseCode(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode())
+            .withType(exception.getClass().getSimpleName())
+            .withMessage(exception.getMessage())


The PR description mentions avoiding exposing internal database information in these errors, but will this message not contain database-specific information?

Do you have an example exception and corresponding JSON error response?

This is absolutely right, my bad 🤦
I'm making it correct by using ExceptionUtils.getRootCause

I can reproduce it in PolarisEclipseLinkMetaStoreManagerTest.java by messing with persistence.xml

How can we be sure that ExceptionUtils.getRootCause().getMessage() (current code) does not expose internal database information?

At this moment, I'd suggest to wrap the jakarta.persistence.PersistenceException in EclipseLink module in a RuntimeException, like what we did in JDBC impl.

polaris/extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java

Line 98 in d3b24d2

throw new RuntimeException("Error persisting entity", e);

.

In that case, we don't have to make any change to mappers.

flyrain · 2025-04-30T22:54:13Z

Hi @DaniilRoman, thanks a lot for the contribution. Let me know if you are still working on it, or this can be picked up by others.

DaniilRoman · 2025-05-20T21:43:06Z

Hi @DaniilRoman, thanks a lot for the contribution. Let me know if you are still working on it, or this can be picked up by others.

Hi @flyrain 👋 My apologies for such a late reply. Unfortunately, I don't have any time at the moment, and I'd appreciate it if anyone would contribute to this solution 🙏

flyrain · 2025-05-22T01:03:33Z

Hi @flyrain 👋 My apologies for such a late reply. Unfortunately, I don't have any time at the moment, and I'd appreciate it if anyone would contribute to this solution 🙏

No worries. Thanks for the contributions!

Add PersistenceException handler in IcebergExceptionMapper

67549bc

DaniilRoman requested review from adutra, ashvina, dennishuo, dimas-b, eric-maynard, jackye1995 and jbonofre as code owners March 15, 2025 18:12

github-project-automation bot added this to Basic Kanban Board Mar 15, 2025

DaniilRoman requested review from vvcephei, collado-mike, snazy, RussellSpitzer, takidau, MonkeyCanCode, flyrain and ebyhr as code owners March 15, 2025 18:12

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Mar 15, 2025

flyrain reviewed Mar 16, 2025

View reviewed changes

dimas-b reviewed Mar 17, 2025

View reviewed changes

DaniilRoman added 2 commits March 19, 2025 22:07

Merge branch 'main' into 608-add-persistence-exception-handler

e02cf85

Create a separated PersistenceException's mapper

b9018ee

DaniilRoman changed the title ~~Add the PersistenceException handler in IcebergExceptionMapper~~ Create PersistenceException mapper Mar 20, 2025

dimas-b reviewed Mar 21, 2025

View reviewed changes

Use ExceptionUtils.getRootCause

79e9b24

flyrain closed this Apr 30, 2025

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Apr 30, 2025

flyrain reopened this Apr 30, 2025

github-project-automation bot moved this from Done to PRs In Progress in Basic Kanban Board Apr 30, 2025

flyrain mentioned this pull request Apr 30, 2025

Polaris Server exposes backend meta in error response body #608

Open

Create PersistenceException mapper #1181

Are you sure you want to change the base?

Create PersistenceException mapper #1181

Uh oh!

Conversation

DaniilRoman commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaniilRoman Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain commented Apr 30, 2025

Uh oh!

DaniilRoman commented May 20, 2025

Uh oh!

flyrain commented May 22, 2025

Uh oh!

Uh oh!

DaniilRoman commented Mar 15, 2025 •

edited

Loading

DaniilRoman Mar 21, 2025 •

edited

Loading