From aa0eb675a1963d05f65ab5f33346cc04e03ecdd8 Mon Sep 17 00:00:00 2001 From: morrySnow Date: Thu, 10 Sep 2026 20:47:07 +0800 Subject: [PATCH] [fix](http) Handle metadata pagination boundaries ### What problem does this PR solve? Problem Summary: Metadata REST endpoints could return the last item for an offset beyond the list, overflow when adding large limit and offset values, or expose numeric parsing failures as internal errors. This change centralizes HTTP limit and offset parsing in RestBaseController and delegates list slicing to the existing LimitElement.applyTo implementation. LimitElement now uses Utils.addOverflows, so REST pagination, SHOW pagination, and optimizer limit handling share the same overflow rule. The legacy and v2 metadata endpoints return an empty page for out-of-range offsets and stable bad-request responses for invalid values. Unit and HTTP regression tests cover normal, out-of-range, overflow, and malformed inputs. ### Release note Fix metadata REST pagination for large and invalid limit or offset values. ### Check List (For Author) - Test: Unit Test and Regression Test - Behavior changed: Yes. Out-of-range offsets now return an empty page, and invalid pagination values return a stable bad-request response. - Does this need documentation: No --- .../apache/doris/analysis/LimitElement.java | 10 ++- .../doris/httpv2/rest/MetaInfoAction.java | 44 +----------- .../doris/httpv2/rest/RestBaseController.java | 37 ++++++++++ .../doris/httpv2/restv2/MetaInfoActionV2.java | 48 +------------ .../httpv2/rest/RestBaseControllerTest.java | 59 ++++++++++++++++ .../auth_p0/test_http_meta_pagination.groovy | 67 +++++++++++++++++++ 6 files changed, 172 insertions(+), 93 deletions(-) create mode 100644 regression-test/suites/auth_p0/test_http_meta_pagination.groovy diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/LimitElement.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/LimitElement.java index 10c70010c798c9..f5bef484541abb 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/LimitElement.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/LimitElement.java @@ -20,6 +20,8 @@ package org.apache.doris.analysis; +import org.apache.doris.nereids.util.Utils; + import java.util.List; /** @@ -79,12 +81,8 @@ public List applyTo(List rows) { int size = rows.size(); long begin = Math.min(Math.max(offset, 0L), size); long end = size; - if (hasLimit()) { - end = begin + limit; - // A negative sum means the long addition itself overflowed. - if (end < 0 || end > size) { - end = size; - } + if (hasLimit() && !Utils.addOverflows(begin, limit)) { + end = Math.min(begin + limit, size); } return rows.subList((int) begin, (int) end); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/MetaInfoAction.java b/fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/MetaInfoAction.java index eb9b591e1e2898..cd2beb2757ea1d 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/MetaInfoAction.java +++ b/fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/MetaInfoAction.java @@ -24,7 +24,6 @@ import org.apache.doris.common.DdlException; import org.apache.doris.common.FeConstants; import org.apache.doris.common.MetaNotFoundException; -import org.apache.doris.common.Pair; import org.apache.doris.common.UserException; import org.apache.doris.common.proc.ProcNodeInterface; import org.apache.doris.common.proc.ProcResult; @@ -33,7 +32,6 @@ import org.apache.doris.datasource.InternalCatalog; import org.apache.doris.httpv2.controller.BaseController.ActionAuthorizationInfo; import org.apache.doris.httpv2.entity.ResponseEntityBuilder; -import org.apache.doris.httpv2.exception.BadRequestException; import org.apache.doris.mysql.privilege.PrivPredicate; import org.apache.doris.qe.ConnectContext; import org.apache.doris.system.SystemInfoService; @@ -64,8 +62,6 @@ public class MetaInfoAction extends RestBaseController { private static final String NAMESPACES = "namespaces"; private static final String DATABASES = "databases"; private static final String TABLES = "tables"; - private static final String PARAM_LIMIT = "limit"; - private static final String PARAM_OFFSET = "offset"; private static final String PARAM_WITH_MV = "with_mv"; @@ -124,8 +120,7 @@ public Object getAllDatabases( Collections.sort(visibleDbNames); // handle limit offset - Pair fromToIndex = getFromToIndex(request, visibleDbNames.size()); - return ResponseEntityBuilder.ok(visibleDbNames.subList(fromToIndex.first, fromToIndex.second)); + return ResponseEntityBuilder.ok(paginate(request, visibleDbNames)); } /** Get all tables of a database @@ -174,8 +169,7 @@ public Object getTables( Collections.sort(tblNames); // handle limit offset - Pair fromToIndex = getFromToIndex(request, tblNames.size()); - return ResponseEntityBuilder.ok(tblNames.subList(fromToIndex.first, fromToIndex.second)); + return ResponseEntityBuilder.ok(paginate(request, tblNames)); } /** Get schema of a table @@ -309,38 +303,4 @@ private String convertIfNull(String val) { return val.equals(FeConstants.null_string) ? null : val; } - // get limit and offset from query parameter - // and return fromIndex and toIndex of a list - private Pair getFromToIndex(HttpServletRequest request, int maxNum) { - String limitStr = request.getParameter(PARAM_LIMIT); - String offsetStr = request.getParameter(PARAM_OFFSET); - - int offset = 0; - int limit = Integer.MAX_VALUE; - if (Strings.isNullOrEmpty(limitStr)) { - // limit not set - if (!Strings.isNullOrEmpty(offsetStr)) { - throw new BadRequestException("Param offset should be set with param limit"); - } - } else { - // limit is set - limit = Integer.valueOf(limitStr); - if (limit < 0) { - throw new BadRequestException("Param limit should >= 0"); - } - - offset = 0; - if (!Strings.isNullOrEmpty(offsetStr)) { - offset = Integer.valueOf(offsetStr); - if (offset < 0) { - throw new BadRequestException("Param offset should >= 0"); - } - } - } - - if (maxNum <= 0) { - return Pair.of(0, 0); - } - return Pair.of(Math.min(offset, maxNum - 1), Math.min(limit + offset, maxNum)); - } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/RestBaseController.java b/fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/RestBaseController.java index dd77b33d3a4b10..dff7ac071a4203 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/RestBaseController.java +++ b/fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/RestBaseController.java @@ -17,6 +17,7 @@ package org.apache.doris.httpv2.rest; +import org.apache.doris.analysis.LimitElement; import org.apache.doris.analysis.UserIdentity; import org.apache.doris.catalog.Env; import org.apache.doris.common.Config; @@ -27,6 +28,7 @@ import org.apache.doris.common.util.NetUtils; import org.apache.doris.httpv2.controller.BaseController; import org.apache.doris.httpv2.entity.ResponseEntityBuilder; +import org.apache.doris.httpv2.exception.BadRequestException; import org.apache.doris.httpv2.exception.UnauthorizedException; import org.apache.doris.master.MetaHelper; import org.apache.doris.qe.ConnectContext; @@ -59,6 +61,7 @@ import java.net.URI; import java.net.URISyntaxException; import java.util.Collections; +import java.util.List; import java.util.stream.Collectors; import javax.annotation.Nullable; import javax.net.ssl.HttpsURLConnection; @@ -74,8 +77,42 @@ public class RestBaseController extends BaseController { protected static final String TXN_OPERATION_KEY = "txn_operation"; protected static final String SINGLE_REPLICA_KEY = "single_replica"; protected static final String FORWARD_MASTER_UT_TEST = "forward_master_ut_test"; + private static final String PARAM_LIMIT = "limit"; + private static final String PARAM_OFFSET = "offset"; private static final Logger LOG = LogManager.getLogger(RestBaseController.class); + /** + * Apply the limit and offset query parameters to a list. + */ + protected List paginate(HttpServletRequest request, List rows) { + String limitString = request.getParameter(PARAM_LIMIT); + String offsetString = request.getParameter(PARAM_OFFSET); + + if (Strings.isNullOrEmpty(limitString)) { + if (!Strings.isNullOrEmpty(offsetString)) { + throw new BadRequestException("Param offset should be set with param limit"); + } + return new LimitElement(0, -1).applyTo(rows); + } + + long limit = parseNonNegativeLong(limitString, PARAM_LIMIT); + long offset = Strings.isNullOrEmpty(offsetString) + ? 0 : parseNonNegativeLong(offsetString, PARAM_OFFSET); + return new LimitElement(offset, limit).applyTo(rows); + } + + private long parseNonNegativeLong(String value, String parameterName) { + try { + long parsedValue = Long.parseLong(value); + if (parsedValue >= 0) { + return parsedValue; + } + } catch (NumberFormatException ignored) { + // Converted to a stable bad-request response below. + } + throw new BadRequestException("Param " + parameterName + " should be a non-negative integer"); + } + public ActionAuthorizationInfo executeCheckPassword(HttpServletRequest request, HttpServletResponse response) throws UnauthorizedException { ActionAuthorizationInfo authInfo = getAuthorizationInfo(request); diff --git a/fe/fe-core/src/main/java/org/apache/doris/httpv2/restv2/MetaInfoActionV2.java b/fe/fe-core/src/main/java/org/apache/doris/httpv2/restv2/MetaInfoActionV2.java index b4f57278b227db..9427f4e2f7272f 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/httpv2/restv2/MetaInfoActionV2.java +++ b/fe/fe-core/src/main/java/org/apache/doris/httpv2/restv2/MetaInfoActionV2.java @@ -25,13 +25,11 @@ import org.apache.doris.catalog.TableIf; import org.apache.doris.common.FeConstants; import org.apache.doris.common.MetaNotFoundException; -import org.apache.doris.common.Pair; import org.apache.doris.common.UserException; import org.apache.doris.datasource.CatalogIf; import org.apache.doris.datasource.InternalCatalog; import org.apache.doris.httpv2.controller.BaseController.ActionAuthorizationInfo; import org.apache.doris.httpv2.entity.ResponseEntityBuilder; -import org.apache.doris.httpv2.exception.BadRequestException; import org.apache.doris.httpv2.rest.RestBaseController; import org.apache.doris.mysql.privilege.PrivPredicate; import org.apache.doris.qe.ConnectContext; @@ -63,8 +61,6 @@ public class MetaInfoActionV2 extends RestBaseController { private static final String NAMESPACES = "namespaces"; private static final String DATABASES = "databases"; private static final String TABLES = "tables"; - private static final String PARAM_LIMIT = "limit"; - private static final String PARAM_OFFSET = "offset"; private static final String PARAM_WITH_MV = "with_mv"; /** @@ -98,8 +94,7 @@ public Object getAllCatalogs( ctlsNames.add(0, InternalCatalog.INTERNAL_CATALOG_NAME); // handle limit offset - Pair fromToIndex = getFromToIndex(request, ctlsNames.size()); - return ResponseEntityBuilder.ok(ctlsNames.subList(fromToIndex.first, fromToIndex.second)); + return ResponseEntityBuilder.ok(paginate(request, ctlsNames)); } /** @@ -146,8 +141,7 @@ public Object getAllDatabases( Collections.sort(filteredDbNames); // handle limit offset - Pair fromToIndex = getFromToIndex(request, filteredDbNames.size()); - return ResponseEntityBuilder.ok(filteredDbNames.subList(fromToIndex.first, fromToIndex.second)); + return ResponseEntityBuilder.ok(paginate(request, filteredDbNames)); } /** Get all tables of a database @@ -202,8 +196,7 @@ public Object getTables( Collections.sort(tblNames); // handle limit offset - Pair fromToIndex = getFromToIndex(request, tblNames.size()); - return ResponseEntityBuilder.ok(tblNames.subList(fromToIndex.first, fromToIndex.second)); + return ResponseEntityBuilder.ok(paginate(request, tblNames)); } /** @@ -362,41 +355,6 @@ private String convertIfNull(String val) { return val.equals(FeConstants.null_string) ? null : val; } - // get limit and offset from query parameter - // and return fromIndex and toIndex of a list - private Pair getFromToIndex(HttpServletRequest request, int maxNum) { - String limitStr = request.getParameter(PARAM_LIMIT); - String offsetStr = request.getParameter(PARAM_OFFSET); - - int offset = 0; - int limit = Integer.MAX_VALUE; - if (Strings.isNullOrEmpty(limitStr)) { - // limit not set - if (!Strings.isNullOrEmpty(offsetStr)) { - throw new BadRequestException("Param offset should be set with param limit"); - } - } else { - // limit is set - limit = Integer.valueOf(limitStr); - if (limit < 0) { - throw new BadRequestException("Param limit should >= 0"); - } - - offset = 0; - if (!Strings.isNullOrEmpty(offsetStr)) { - offset = Integer.valueOf(offsetStr); - if (offset < 0) { - throw new BadRequestException("Param offset should >= 0"); - } - } - } - - if (maxNum <= 0) { - return Pair.of(0, 0); - } - return Pair.of(Math.min(offset, maxNum - 1), Math.min(limit + offset, maxNum)); - } - @Getter @Setter public static class TableSchemaInfo { diff --git a/fe/fe-core/src/test/java/org/apache/doris/httpv2/rest/RestBaseControllerTest.java b/fe/fe-core/src/test/java/org/apache/doris/httpv2/rest/RestBaseControllerTest.java index ad7113706b478f..63c62be7a25fab 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/httpv2/rest/RestBaseControllerTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/httpv2/rest/RestBaseControllerTest.java @@ -17,6 +17,7 @@ package org.apache.doris.httpv2.rest; +import org.apache.doris.httpv2.exception.BadRequestException; import org.apache.doris.thrift.TNetworkAddress; import jakarta.servlet.http.HttpServletRequest; @@ -24,8 +25,13 @@ import org.junit.jupiter.api.Test; import org.mockito.Mockito; +import java.util.Arrays; +import java.util.List; + public class RestBaseControllerTest { + private static final List ROWS = Arrays.asList(0, 1, 2); + @Test public void testBuildRedirectUrlPreservesEncodedPath() { // Keep the original encoded path unchanged when rebuilding the redirect URL. @@ -68,6 +74,55 @@ public void testBuildRedirectUrlToBackendForcesHttpEvenWhenRequestIsHttps() { Assertions.assertEquals("http://be-host:8040/api/db/tbl/_stream_load?k=v", redirectUrl); } + @Test + public void testPaginateDefaultAndNormalRanges() { + assertPage(null, null, Arrays.asList(0, 1, 2)); + assertPage("1", null, Arrays.asList(0)); + assertPage("1", "1", Arrays.asList(1)); + assertPage("0", "1", Arrays.asList()); + assertPage("10", "1", Arrays.asList(1, 2)); + } + + @Test + public void testPaginateLargeValues() { + assertPage("1", Long.toString(Long.MAX_VALUE), Arrays.asList()); + assertPage(Long.toString(Long.MAX_VALUE), "1", Arrays.asList(1, 2)); + } + + @Test + public void testPaginateOffsetRequiresLimit() { + BadRequestException exception = Assertions.assertThrows(BadRequestException.class, + () -> paginate(null, "1")); + Assertions.assertEquals("Param offset should be set with param limit", exception.getMessage()); + } + + @Test + public void testPaginateInvalidParameters() { + assertInvalid("-1", null, "Param limit should be a non-negative integer"); + assertInvalid("not-a-number", null, "Param limit should be a non-negative integer"); + assertInvalid("9223372036854775808", null, "Param limit should be a non-negative integer"); + assertInvalid("1", "-1", "Param offset should be a non-negative integer"); + assertInvalid("1", "not-a-number", "Param offset should be a non-negative integer"); + assertInvalid("1", "9223372036854775808", "Param offset should be a non-negative integer"); + } + + private void assertPage(String limit, String offset, List expected) { + Assertions.assertEquals(expected, paginate(limit, offset)); + } + + private void assertInvalid(String limit, String offset, String expectedMessage) { + BadRequestException exception = Assertions.assertThrows(BadRequestException.class, + () -> paginate(limit, offset)); + Assertions.assertEquals(expectedMessage, exception.getMessage()); + } + + private List paginate(String limit, String offset) { + HttpServletRequest request = Mockito.mock(HttpServletRequest.class); + Mockito.when(request.getParameter("limit")).thenReturn(limit); + Mockito.when(request.getParameter("offset")).thenReturn(offset); + return new TestRestController().paginateForTest(request, ROWS); + } + // Expose the protected helper so the redirect URL can be verified directly. private static class TestRestController extends RestBaseController { private String buildRedirectUrlForTest(HttpServletRequest request, TNetworkAddress addr, @@ -79,5 +134,9 @@ private String buildRedirectUrlToBackendForTest(HttpServletRequest request, TNet String requestPath, String queryString) { return buildRedirectUrlToBackend(request, addr, requestPath, queryString); } + + private List paginateForTest(HttpServletRequest request, List rows) { + return paginate(request, rows); + } } } diff --git a/regression-test/suites/auth_p0/test_http_meta_pagination.groovy b/regression-test/suites/auth_p0/test_http_meta_pagination.groovy new file mode 100644 index 00000000000000..b61d2cd2c92252 --- /dev/null +++ b/regression-test/suites/auth_p0/test_http_meta_pagination.groovy @@ -0,0 +1,67 @@ +// 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. + +suite("test_http_meta_pagination", "p0,auth,nonConcurrent") { + def getMeta = { uriPath, checkFunc -> + httpTest { + basicAuthorization "${context.config.jdbcUser}", "${context.config.jdbcPassword}" + endpoint "${context.config.feHttpAddress}" + uri uriPath + op "get" + check checkFunc + } + } + + def assertEmptyPage = { uriPath -> + getMeta.call(uriPath) { + respCode, body -> + assertEquals(200, respCode) + def json = parseJson(body) + assertEquals(0, json.code) + assertEquals([], json.data) + } + } + + def assertBadRequest = { uriPath, message -> + getMeta.call(uriPath) { + respCode, body -> + assertEquals(200, respCode) + def json = parseJson(body) + assertEquals(403, json.code) + assertEquals(message, json.data) + } + } + + [ + "/api/meta/namespaces/default_cluster/databases", + "/rest/v2/api/meta/namespaces" + ].each { uriPath -> + assertEmptyPage.call("${uriPath}?limit=1&offset=999999999") + assertEmptyPage.call("${uriPath}?limit=2147483647&offset=2147483647") + + getMeta.call("${uriPath}?limit=2147483648") { + respCode, body -> + assertEquals(200, respCode) + assertEquals(0, parseJson(body).code) + } + + assertBadRequest.call("${uriPath}?limit=9223372036854775808", + "Param limit should be a non-negative integer") + assertBadRequest.call("${uriPath}?limit=1&offset=invalid", + "Param offset should be a non-negative integer") + } +}