From 6e50b822dbeb699dfb98daa6ac0b8f56a22a5e02 Mon Sep 17 00:00:00 2001 From: Jiajia Li Date: Fri, 28 Aug 2026 13:03:28 -0400 Subject: [PATCH] [api] Stop reading a server error message as a format string --- .../paimon/rest/DefaultErrorHandler.java | 4 +- .../java/org/apache/paimon/rest/RESTApi.java | 3 +- .../rest/exceptions/BadRequestException.java | 4 ++ .../exceptions/NotAuthorizedException.java | 2 +- .../exceptions/NotImplementedException.java | 2 +- .../exceptions/ServiceFailureException.java | 2 +- .../ServiceUnavailableException.java | 2 +- .../paimon/rest/RESTApiFunctionNameTest.java | 51 +++++++++++++++++++ .../org/apache/paimon/rest/RESTCatalog.java | 2 +- .../paimon/rest/DefaultErrorHandlerTest.java | 20 ++++++++ .../paimon/rest/MockRESTCatalogTest.java | 14 +++++ 11 files changed, 98 insertions(+), 8 deletions(-) create mode 100644 paimon-api/src/test/java/org/apache/paimon/rest/RESTApiFunctionNameTest.java diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/DefaultErrorHandler.java b/paimon-api/src/main/java/org/apache/paimon/rest/DefaultErrorHandler.java index c9f4ae0df890..67ce6ced18f0 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/DefaultErrorHandler.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/DefaultErrorHandler.java @@ -52,7 +52,7 @@ public void accept(ErrorResponse error, String requestId) { } switch (code) { case 400: - throw new BadRequestException(String.format("%s", message)); + throw new BadRequestException("%s", message); case 401: throw new NotAuthorizedException("Not authorized: %s", message); case 403: @@ -69,7 +69,7 @@ public void accept(ErrorResponse error, String requestId) { case 500: throw new ServiceFailureException("Server error: %s", message); case 501: - throw new NotImplementedException(message); + throw new NotImplementedException("%s", message); case 503: throw new ServiceUnavailableException("Service unavailable: %s", message); default: diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java b/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java index b4b110cfec20..9fb56cca26d2 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java @@ -1487,7 +1487,8 @@ public GetFunctionResponse getFunction(Identifier identifier) { throw new NoSuchResourceException( ErrorResponse.RESOURCE_TYPE_FUNCTION, identifier.getObjectName(), - "Invalid function name: " + identifier.getObjectName()); + "Invalid function name: %s", + identifier.getObjectName()); } return client.get( resourcePaths.function(identifier.getDatabaseName(), identifier.getObjectName()), diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/BadRequestException.java b/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/BadRequestException.java index 301f3bd63f88..9095d0165175 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/BadRequestException.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/BadRequestException.java @@ -24,4 +24,8 @@ public class BadRequestException extends RESTException { public BadRequestException(String message, Object... args) { super(message, args); } + + public BadRequestException(Throwable cause, String message, Object... args) { + super(cause, message, args); + } } diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/NotAuthorizedException.java b/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/NotAuthorizedException.java index 79c9aa4e6773..cd4f0a27dac3 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/NotAuthorizedException.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/NotAuthorizedException.java @@ -22,6 +22,6 @@ public class NotAuthorizedException extends RESTException { public NotAuthorizedException(String message, Object... args) { - super(String.format(message, args)); + super(message, args); } } diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/NotImplementedException.java b/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/NotImplementedException.java index a6a345434314..a70fa7828a69 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/NotImplementedException.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/NotImplementedException.java @@ -22,6 +22,6 @@ public class NotImplementedException extends RESTException { public NotImplementedException(String message, Object... args) { - super(String.format(message, args)); + super(message, args); } } diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/ServiceFailureException.java b/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/ServiceFailureException.java index 1df196d90fd4..4e6ecc3a2593 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/ServiceFailureException.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/ServiceFailureException.java @@ -22,6 +22,6 @@ public class ServiceFailureException extends RESTException { public ServiceFailureException(String message, Object... args) { - super(String.format(message, args)); + super(message, args); } } diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/ServiceUnavailableException.java b/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/ServiceUnavailableException.java index c466b4c901d1..f9e300d0314a 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/ServiceUnavailableException.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/exceptions/ServiceUnavailableException.java @@ -22,6 +22,6 @@ public class ServiceUnavailableException extends RESTException { public ServiceUnavailableException(String message, Object... args) { - super(String.format(message, args)); + super(message, args); } } diff --git a/paimon-api/src/test/java/org/apache/paimon/rest/RESTApiFunctionNameTest.java b/paimon-api/src/test/java/org/apache/paimon/rest/RESTApiFunctionNameTest.java new file mode 100644 index 000000000000..0580ae12738f --- /dev/null +++ b/paimon-api/src/test/java/org/apache/paimon/rest/RESTApiFunctionNameTest.java @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.rest; + +import org.apache.paimon.catalog.Identifier; +import org.apache.paimon.options.Options; +import org.apache.paimon.rest.exceptions.NoSuchResourceException; + +import org.junit.jupiter.api.Test; + +import static org.apache.paimon.rest.RESTCatalogInternalOptions.PREFIX; +import static org.apache.paimon.rest.RESTCatalogOptions.TOKEN; +import static org.apache.paimon.rest.RESTCatalogOptions.TOKEN_PROVIDER; +import static org.apache.paimon.rest.RESTCatalogOptions.URI; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** Tests for the name check {@link RESTApi} makes before it sends anything. */ +public class RESTApiFunctionNameTest { + + @Test + public void testARejectedFunctionNameIsReportedAndNotFormatted() { + Options options = new Options(); + options.set(URI, "http://127.0.0.1:1"); + options.set(TOKEN_PROVIDER, "bear"); + options.set(TOKEN, "secret"); + options.set(PREFIX, "catalog"); + RESTApi api = new RESTApi(options, false); + + // the name is why the request is refused, so it belongs in the message as data -- the + // validator rejects '%', which is also what java.util.Formatter reads as a conversion + assertThatThrownBy(() -> api.getFunction(Identifier.create("db", "function%"))) + .isInstanceOf(NoSuchResourceException.class) + .hasMessageContaining("function%"); + } +} diff --git a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java index 9a21f3fa41e2..50fe373d6a03 100644 --- a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java @@ -322,7 +322,7 @@ private PagedList listSystemTablesPaged( return SystemTableLoader.loadGlobalTableNamesPaged( context.options(), maxResults, pageToken, tableNamePattern, tableType); } catch (IllegalArgumentException e) { - throw new BadRequestException(e.getMessage()); + throw new BadRequestException(e, "%s", e.getMessage()); } } diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/DefaultErrorHandlerTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/DefaultErrorHandlerTest.java index c46c596287dc..9f8c6904664f 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/DefaultErrorHandlerTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/DefaultErrorHandlerTest.java @@ -36,6 +36,7 @@ import static org.apache.paimon.rest.interceptor.LoggingInterceptor.DEFAULT_REQUEST_ID; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; /** Test for {@link DefaultErrorHandler}. */ public class DefaultErrorHandlerTest { @@ -83,6 +84,25 @@ public void testHandleErrorResponse() { () -> defaultErrorHandler.accept(generateErrorResponse(503), DEFAULT_REQUEST_ID)); } + @Test + public void testErrorMessageIsNotReadAsAFormatString() { + // a server message is data, and this one holds what java.util.Formatter reads as syntax + String message = "quota 80% exceeded"; + for (int code : new int[] {400, 401, 403, 404, 405, 406, 409, 500, 501, 502, 503}) { + RESTException exception = + assertThrows( + RESTException.class, + () -> + defaultErrorHandler.accept( + new ErrorResponse("table", "t", message, code), + DEFAULT_REQUEST_ID), + "status " + code); + assertTrue( + exception.getMessage().contains(message), + "status " + code + " lost the message: " + exception.getMessage()); + } + } + private ErrorResponse generateErrorResponse(int code) { return new ErrorResponse(null, null, "message", code); } diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/MockRESTCatalogTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/MockRESTCatalogTest.java index bd7fedce3ae3..7c5503534581 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/MockRESTCatalogTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/MockRESTCatalogTest.java @@ -85,6 +85,7 @@ import java.util.Map; import java.util.UUID; +import static org.apache.paimon.catalog.Catalog.SYSTEM_DATABASE_NAME; import static org.apache.paimon.catalog.Catalog.TABLE_DEFAULT_OPTION_PREFIX; import static org.apache.paimon.rest.RESTApi.HEADER_PREFIX; import static org.apache.paimon.rest.RESTApi.READ_VIA_HEADER; @@ -142,6 +143,19 @@ void testAuthFail() { .isInstanceOf(NotAuthorizedException.class); } + @Test + void testRejectedSystemTablePatternKeepsWhatRaisedIt() { + // the message is the argument, never the format, and the cause is what says where it + // came from -- the caller sees only what this exception carries + assertThatThrownBy( + () -> + catalog.listTablesPaged( + SYSTEM_DATABASE_NAME, null, null, "a%b", null)) + .isInstanceOf(BadRequestException.class) + .hasMessageContaining("prefix sql like pattern") + .hasCauseInstanceOf(IllegalArgumentException.class); + } + @Test void testInvalidManagementDtoReturnsBadRequest() throws Exception { String database = "invalid_management_dto";