yuqi1129 commented on code in PR #12880:
URL: https://github.com/apache/gravitino/pull/12880#discussion_r3931521841
##########
server/src/main/java/org/apache/gravitino/server/web/rest/StatisticOperations.java:
##########
@@ -172,7 +172,7 @@ public Response updateStatistics(
MetadataObjects.parse(
fullName,
MetadataObject.Type.valueOf(type.toUpperCase(Locale.ROOT)));
if (object.type() != MetadataObject.Type.TABLE) {
- throw new UnsupportedOperationException(
+ throw new IllegalArgumentException(
"Update statistics is only supported for tables now.");
}
Review Comment:
Covered by testStatisticsEndpointsRejectNonTableObject, which exercises
list, update, drop, and all three partition-statistics routes and asserts HTTP
400 with ILLEGAL_ARGUMENTS_CODE.
##########
web-v2/web/src/lib/utils/axios/unsupportedOperation.test.js:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.
+ */
+
+import { describe, expect, it } from 'vitest'
+import { isUnsupportedOperationError } from
'@/lib/utils/axios/unsupportedOperation'
+
+describe('isUnsupportedOperationError', () => {
+ it('recognizes HTTP 501 as an unsupported operation', () => {
+ expect(isUnsupportedOperationError({ response: { status: 501 }
})).toBe(true)
+ })
+
+ it('recognizes the legacy HTTP 405 response by its application error code',
() => {
+ expect(isUnsupportedOperationError({ response: { status: 405, data: {
code: 1006 } } })).toBe(true)
+ })
+
+ it('does not hide an actual HTTP method mismatch', () => {
+ expect(isUnsupportedOperationError({ response: { status: 405, data: {
code: 1000 } } })).toBe(false)
+ })
Review Comment:
Added a regression test asserting that HTTP 409 with application code 1006
is not treated as an unsupported operation.
##########
web-v2/web/src/lib/utils/axios/unsupportedOperation.js:
##########
@@ -0,0 +1,24 @@
+/*
+ * 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.
+ */
+
+const UNSUPPORTED_OPERATION_CODE = 1006
+
+/** Returns whether an HTTP client error represents an unsupported server
operation. */
+export const isUnsupportedOperationError = error =>
+ error?.response?.data?.code === UNSUPPORTED_OPERATION_CODE ||
error?.response?.status === 501
Review Comment:
Updated the detector to accept HTTP 501, or the legacy HTTP 405 only when
paired with application code 1006. HTTP 409 is no longer misclassified.
##########
plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/IdpRESTUtils.java:
##########
@@ -121,7 +122,10 @@ private static Response toErrorResponse(String errorMsg,
Exception e) {
if (e instanceof AlreadyExistsException) {
return alreadyExists(errorMsg, e);
}
- if (e instanceof IllegalStateException || e instanceof
UnsupportedOperationException) {
+ if (e instanceof NonEmptyEntityException) {
+ return Utils.nonEmpty(errorMsg, e);
+ }
Review Comment:
Updated IdpRESTUtils to construct both non-empty and unsupported-operation
responses through its local json helper, removing the Utils dependency while
preserving the 409 and 501 contracts.
##########
server-common/src/main/java/org/apache/gravitino/server/web/Utils.java:
##########
@@ -189,17 +189,43 @@ public static Response nonEmpty(String type, String
message, Throwable throwable
.build();
}
+ /**
+ * Returns an HTTP 501 response for functionality that the server does not
implement.
+ *
+ * @param message the error message
+ * @return the HTTP response
+ */
public static Response unsupportedOperation(String message) {
return unsupportedOperation(message, null);
}
+ /**
+ * Returns an HTTP 501 response for functionality that the server does not
implement.
+ *
+ * @param message the error message
+ * @param throwable the exception that caused the error
+ * @return the HTTP response
+ */
public static Response unsupportedOperation(String message, Throwable
throwable) {
- return Response.status(Response.Status.METHOD_NOT_ALLOWED)
+ return Response.status(Response.Status.NOT_IMPLEMENTED)
.entity(ErrorResponse.unsupportedOperation(message, throwable))
.type(MediaType.APPLICATION_JSON)
.build();
}
+ /**
+ * Returns an HTTP 405 response when the target resource does not allow the
request method.
+ *
+ * @param message the error message
+ * @return the HTTP response
+ */
+ public static Response methodNotAllowed(String message) {
+ return Response.status(Response.Status.METHOD_NOT_ALLOWED)
+ .entity(ErrorResponse.unsupportedOperation(message))
+ .type(MediaType.APPLICATION_JSON)
+ .build();
Review Comment:
Documented that the 405 helper intentionally retains the
unsupported-operation payload for application-code compatibility.
##########
web-v2/web/src/lib/store/metalakes/index.js:
##########
@@ -2250,8 +2251,8 @@ export const fetchViews = createAsyncThunk(
const [err, res] = await to(getViewsApi({ metalake, catalog, schema }, {
errorMessageMode: 'none' }))
if (err || !res) {
- // Catalog doesn't support views (HTTP 405) — return empty views silently
- if (err?.response?.status === 405) {
+ // Catalog doesn't support views (HTTP 501) — return empty views silently
Review Comment:
Updated both comments to describe the current HTTP 501 and legacy HTTP 405
with code 1006 behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
