This is an automated email from the ASF dual-hosted git repository.
jshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 253942db07 [#4377] fix(common): Deprecate the DeleteResponse and use
DropResponse instead (#8146)
253942db07 is described below
commit 253942db079002341a188a31535765cb7fb22d91
Author: Jerry Shao <[email protected]>
AuthorDate: Tue Aug 19 11:40:55 2025 +0800
[#4377] fix(common): Deprecate the DeleteResponse and use DropResponse
instead (#8146)
### What changes were proposed in this pull request?
`DeleteResponse` is duplicated with `DropResponse`, so deprecate the
`DeleteResponse` and use `DropResponse` instead.
### Why are the changes needed?
Fix: #4377
### Does this PR introduce _any_ user-facing change?
Yes
### How was this patch tested?
Add UTs to test the compability.
---
.../apache/gravitino/client/GravitinoMetalake.java | 7 +-
.../apache/gravitino/client/TestDropResponse.java | 83 +++++++++++++++++++++
.../java/org/apache/gravitino/client/TestRole.java | 10 +--
.../gravitino/dto/responses/DeleteResponse.java | 6 +-
.../gravitino/dto/responses/DropResponse.java | 56 ++++++++++++--
.../gravitino/dto/requests/TestDropResponse.java | 87 ++++++++++++++++++++++
.../gravitino/dto/responses/TestResponses.java | 4 +-
.../gravitino/server/web/rest/RoleOperations.java | 4 +-
.../server/web/rest/TestRoleOperations.java | 14 ++--
9 files changed, 245 insertions(+), 26 deletions(-)
diff --git
a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java
b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java
index bc193169be..fe2ade5c4c 100644
---
a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java
+++
b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java
@@ -73,7 +73,6 @@ import org.apache.gravitino.dto.requests.UserAddRequest;
import org.apache.gravitino.dto.responses.BaseResponse;
import org.apache.gravitino.dto.responses.CatalogListResponse;
import org.apache.gravitino.dto.responses.CatalogResponse;
-import org.apache.gravitino.dto.responses.DeleteResponse;
import org.apache.gravitino.dto.responses.DropResponse;
import org.apache.gravitino.dto.responses.EntityListResponse;
import org.apache.gravitino.dto.responses.ErrorResponse;
@@ -1017,18 +1016,18 @@ public class GravitinoMetalake extends MetalakeDTO
* @throws RuntimeException If deleting the Role encounters storage issues.
*/
public boolean deleteRole(String role) throws NoSuchMetalakeException {
- DeleteResponse resp =
+ DropResponse resp =
restClient.delete(
String.format(
API_METALAKES_ROLES_PATH,
RESTUtils.encodeString(this.name()),
RESTUtils.encodeString(role)),
- DeleteResponse.class,
+ DropResponse.class,
Collections.emptyMap(),
ErrorHandlers.roleErrorHandler());
resp.validate();
- return resp.deleted();
+ return resp.dropped();
}
/**
diff --git
a/clients/client-java/src/test/java/org/apache/gravitino/client/TestDropResponse.java
b/clients/client-java/src/test/java/org/apache/gravitino/client/TestDropResponse.java
new file mode 100644
index 0000000000..caabe3b51d
--- /dev/null
+++
b/clients/client-java/src/test/java/org/apache/gravitino/client/TestDropResponse.java
@@ -0,0 +1,83 @@
+/*
+ * 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.gravitino.client;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import org.apache.gravitino.dto.responses.DeleteResponse;
+import org.apache.gravitino.dto.responses.DropResponse;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestDropResponse {
+
+ @Test
+ public void testSupportDropResponseSerDe() throws JsonProcessingException {
+ // test new server with new client
+ String json = "{\"dropped\":true,\"deleted\":null}";
+ DropResponse resp = ObjectMapperProvider.objectMapper().readValue(json,
DropResponse.class);
+ Assertions.assertTrue(resp.dropped());
+
+ json = "{\"dropped\":true,\"deleted\":true}";
+ resp = ObjectMapperProvider.objectMapper().readValue(json,
DropResponse.class);
+ Assertions.assertTrue(resp.dropped());
+
+ json = "{\"dropped\":false,\"deleted\":false}";
+ resp = ObjectMapperProvider.objectMapper().readValue(json,
DropResponse.class);
+ Assertions.assertFalse(resp.dropped());
+
+ json = "{\"dropped\":false}";
+ resp = ObjectMapperProvider.objectMapper().readValue(json,
DropResponse.class);
+ Assertions.assertFalse(resp.dropped());
+
+ json = "{\"dropped\":true}";
+ resp = ObjectMapperProvider.objectMapper().readValue(json,
DropResponse.class);
+ Assertions.assertTrue(resp.dropped());
+
+ // test old server with new client
+ json = "{\"deleted\":true}";
+ resp = ObjectMapperProvider.objectMapper().readValue(json,
DropResponse.class);
+ Assertions.assertTrue(resp.dropped());
+
+ json = "{\"deleted\":false}";
+ resp = ObjectMapperProvider.objectMapper().readValue(json,
DropResponse.class);
+ Assertions.assertFalse(resp.dropped());
+ }
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testSupportDeleteResponseSerDe() throws JsonProcessingException {
+ // New server with old client
+ String json = "{\"dropped\":true,\"deleted\":true}";
+ DeleteResponse resp = ObjectMapperProvider.objectMapper().readValue(json,
DeleteResponse.class);
+ Assertions.assertTrue(resp.deleted());
+
+ json = "{\"dropped\":false,\"deleted\":false}";
+ resp = ObjectMapperProvider.objectMapper().readValue(json,
DeleteResponse.class);
+ Assertions.assertFalse(resp.deleted());
+
+ // Old server with old client
+ json = "{\"deleted\":true}";
+ resp = ObjectMapperProvider.objectMapper().readValue(json,
DeleteResponse.class);
+ Assertions.assertTrue(resp.deleted());
+
+ json = "{\"deleted\":false}";
+ resp = ObjectMapperProvider.objectMapper().readValue(json,
DeleteResponse.class);
+ Assertions.assertFalse(resp.deleted());
+ }
+}
diff --git
a/clients/client-java/src/test/java/org/apache/gravitino/client/TestRole.java
b/clients/client-java/src/test/java/org/apache/gravitino/client/TestRole.java
index 3d0771fc5f..035d330afc 100644
---
a/clients/client-java/src/test/java/org/apache/gravitino/client/TestRole.java
+++
b/clients/client-java/src/test/java/org/apache/gravitino/client/TestRole.java
@@ -37,7 +37,7 @@ import org.apache.gravitino.dto.authorization.PrivilegeDTO;
import org.apache.gravitino.dto.authorization.RoleDTO;
import org.apache.gravitino.dto.authorization.SecurableObjectDTO;
import org.apache.gravitino.dto.requests.RoleCreateRequest;
-import org.apache.gravitino.dto.responses.DeleteResponse;
+import org.apache.gravitino.dto.responses.DropResponse;
import org.apache.gravitino.dto.responses.ErrorResponse;
import org.apache.gravitino.dto.responses.MetalakeResponse;
import org.apache.gravitino.dto.responses.NameListResponse;
@@ -197,13 +197,13 @@ public class TestRole extends TestBase {
String roleName = "role";
String rolePath = withSlash(String.format(API_METALAKES_ROLES_PATH,
metalakeName, roleName));
- DeleteResponse deleteResponse = new DeleteResponse(true);
- buildMockResource(Method.DELETE, rolePath, null, deleteResponse, SC_OK);
+ DropResponse dropResponse = new DropResponse(true, true);
+ buildMockResource(Method.DELETE, rolePath, null, dropResponse, SC_OK);
Assertions.assertTrue(gravitinoClient.deleteRole(roleName));
- deleteResponse = new DeleteResponse(false);
- buildMockResource(Method.DELETE, rolePath, null, deleteResponse, SC_OK);
+ dropResponse = new DropResponse(false, false);
+ buildMockResource(Method.DELETE, rolePath, null, dropResponse, SC_OK);
Assertions.assertFalse(gravitinoClient.deleteRole(roleName));
// test RuntimeException
diff --git
a/common/src/main/java/org/apache/gravitino/dto/responses/DeleteResponse.java
b/common/src/main/java/org/apache/gravitino/dto/responses/DeleteResponse.java
index 69204b47dd..77f312cc76 100644
---
a/common/src/main/java/org/apache/gravitino/dto/responses/DeleteResponse.java
+++
b/common/src/main/java/org/apache/gravitino/dto/responses/DeleteResponse.java
@@ -22,7 +22,11 @@ import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.EqualsAndHashCode;
import lombok.ToString;
-/** Represents a response for a delete operation. */
+/**
+ * Represents a response for a delete operation. This class is deprecated and
will be removed in
+ * future versions, please use {@link DropResponse} instead.
+ */
+@Deprecated(since = "1.0.0")
@ToString
@EqualsAndHashCode(callSuper = true)
public class DeleteResponse extends BaseResponse {
diff --git
a/common/src/main/java/org/apache/gravitino/dto/responses/DropResponse.java
b/common/src/main/java/org/apache/gravitino/dto/responses/DropResponse.java
index db4f0031b4..5c6db9be88 100644
--- a/common/src/main/java/org/apache/gravitino/dto/responses/DropResponse.java
+++ b/common/src/main/java/org/apache/gravitino/dto/responses/DropResponse.java
@@ -19,6 +19,8 @@
package org.apache.gravitino.dto.responses;
import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
import lombok.EqualsAndHashCode;
import lombok.ToString;
@@ -28,22 +30,44 @@ import lombok.ToString;
public class DropResponse extends BaseResponse {
@JsonProperty("dropped")
- private final boolean dropped;
+ private final Boolean dropped;
+
+ /**
+ * This field is added to be compatible with DeleteResponse. This field will
only be set for
+ * deleteRole operation. For other drop operations, it will be null. This
field will be leveraged
+ * by the old client, for new client it will never be leveraged.
+ */
+ @JsonProperty("deleted")
+ private final Boolean deleted;
/**
* Constructor for DropResponse.
*
* @param dropped Whether the drop operation was successful.
*/
- public DropResponse(boolean dropped) {
+ public DropResponse(Boolean dropped) {
super(0);
this.dropped = dropped;
+ this.deleted = null;
+ }
+
+ /**
+ * Constructor for DropResponse with both dropped and deleted fields.
+ *
+ * @param dropped Whether the drop operation was successful.
+ * @param deleted Whether the delete operation was successful (used for
backward compatibility).
+ */
+ public DropResponse(Boolean dropped, Boolean deleted) {
+ super(0);
+ this.dropped = dropped;
+ this.deleted = deleted;
}
/** Default constructor for DropResponse (used by Jackson deserializer). */
public DropResponse() {
super();
- this.dropped = false;
+ this.dropped = null;
+ this.deleted = null;
}
/**
@@ -51,7 +75,29 @@ public class DropResponse extends BaseResponse {
*
* @return True if the drop operation was successful, otherwise false.
*/
- public boolean dropped() {
- return dropped;
+ public Boolean dropped() {
+ // If the dropped field is null, it means we use the new client to handle
the old server
+ // `DeleteResponse` response, so we should return the deleted field.
+ return dropped != null ? dropped : deleted;
+ }
+
+ /**
+ * Returns whether the delete operation was successful. This method will
only be called in test.
+ *
+ * @return True if the delete operation was successful, otherwise false.
+ */
+ @VisibleForTesting
+ public Boolean deleted() {
+ return deleted;
+ }
+
+ @Override
+ public void validate() {
+ super.validate();
+
+ // Ensure that at least one of dropped or deleted is set
+ Preconditions.checkArgument(
+ dropped != null || deleted != null,
+ "Either 'dropped' or 'deleted' must be set in DropResponse");
}
}
diff --git
a/common/src/test/java/org/apache/gravitino/dto/requests/TestDropResponse.java
b/common/src/test/java/org/apache/gravitino/dto/requests/TestDropResponse.java
new file mode 100644
index 0000000000..14ee74c3da
--- /dev/null
+++
b/common/src/test/java/org/apache/gravitino/dto/requests/TestDropResponse.java
@@ -0,0 +1,87 @@
+/*
+ * 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.gravitino.dto.requests;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import org.apache.gravitino.dto.responses.DropResponse;
+import org.apache.gravitino.json.JsonUtils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestDropResponse {
+
+ @Test
+ public void testDropResponseSerDe() throws JsonProcessingException {
+ DropResponse resp = new DropResponse(true);
+ String serJson = JsonUtils.objectMapper().writeValueAsString(resp);
+ DropResponse deserResp = JsonUtils.objectMapper().readValue(serJson,
DropResponse.class);
+ Assertions.assertTrue(deserResp.dropped());
+ Assertions.assertNull(deserResp.deleted());
+
+ resp = new DropResponse(true, true);
+ serJson = JsonUtils.objectMapper().writeValueAsString(resp);
+ deserResp = JsonUtils.objectMapper().readValue(serJson,
DropResponse.class);
+ Assertions.assertTrue(deserResp.dropped());
+ Assertions.assertTrue(deserResp.deleted() != null && deserResp.deleted());
+
+ resp = new DropResponse(false, false);
+ serJson = JsonUtils.objectMapper().writeValueAsString(resp);
+ deserResp = JsonUtils.objectMapper().readValue(serJson,
DropResponse.class);
+ Assertions.assertFalse(deserResp.dropped());
+ Assertions.assertTrue(deserResp.deleted() != null && !deserResp.deleted());
+ }
+
+ @Test
+ public void deserializeFromString() throws JsonProcessingException {
+ String json = "{\"dropped\":true,\"deleted\":null}";
+ DropResponse response = JsonUtils.objectMapper().readValue(json,
DropResponse.class);
+ Assertions.assertTrue(response.dropped());
+ Assertions.assertNull(response.deleted());
+
+ json = "{\"dropped\":true,\"deleted\":true}";
+ response = JsonUtils.objectMapper().readValue(json, DropResponse.class);
+ Assertions.assertTrue(response.dropped());
+ Assertions.assertTrue(response.deleted() != null && response.deleted());
+
+ json = "{\"dropped\":false,\"deleted\":false}";
+ response = JsonUtils.objectMapper().readValue(json, DropResponse.class);
+ Assertions.assertFalse(response.dropped());
+ Assertions.assertTrue(response.deleted() != null && !response.deleted());
+
+ json = "{\"dropped\":false}";
+ response = JsonUtils.objectMapper().readValue(json, DropResponse.class);
+ Assertions.assertFalse(response.dropped());
+ Assertions.assertNull(response.deleted());
+
+ json = "{\"dropped\":true}";
+ response = JsonUtils.objectMapper().readValue(json, DropResponse.class);
+ Assertions.assertTrue(response.dropped());
+ Assertions.assertNull(response.deleted());
+
+ json = "{\"deleted\":true}";
+ response = JsonUtils.objectMapper().readValue(json, DropResponse.class);
+ Assertions.assertTrue(response.dropped());
+ Assertions.assertTrue(response.deleted() != null && response.deleted());
+
+ json = "{\"deleted\":false}";
+ response = JsonUtils.objectMapper().readValue(json, DropResponse.class);
+ Assertions.assertFalse(response.dropped());
+ Assertions.assertTrue(response.deleted() != null && !response.deleted());
+ }
+}
diff --git
a/common/src/test/java/org/apache/gravitino/dto/responses/TestResponses.java
b/common/src/test/java/org/apache/gravitino/dto/responses/TestResponses.java
index 9e3b1904c0..c3385cb9e8 100644
--- a/common/src/test/java/org/apache/gravitino/dto/responses/TestResponses.java
+++ b/common/src/test/java/org/apache/gravitino/dto/responses/TestResponses.java
@@ -21,7 +21,7 @@ package org.apache.gravitino.dto.responses;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -77,7 +77,7 @@ public class TestResponses {
void testDropped() throws IllegalArgumentException {
DropResponse drop = new DropResponse();
- assertFalse(drop.dropped());
+ assertNull(drop.dropped());
}
@Test
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java
index c54a6be497..934478c0dd 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java
@@ -47,7 +47,7 @@ import org.apache.gravitino.authorization.SecurableObjects;
import org.apache.gravitino.dto.authorization.PrivilegeDTO;
import org.apache.gravitino.dto.authorization.SecurableObjectDTO;
import org.apache.gravitino.dto.requests.RoleCreateRequest;
-import org.apache.gravitino.dto.responses.DeleteResponse;
+import org.apache.gravitino.dto.responses.DropResponse;
import org.apache.gravitino.dto.responses.NameListResponse;
import org.apache.gravitino.dto.responses.RoleResponse;
import org.apache.gravitino.dto.util.DTOConverters;
@@ -220,7 +220,7 @@ public class RoleOperations {
if (!deleted) {
LOG.warn("Failed to delete role {} under metalake {}", role,
metalake);
}
- return Utils.ok(new DeleteResponse(deleted));
+ return Utils.ok(new DropResponse(deleted, deleted));
});
} catch (Exception e) {
return ExceptionHandlers.handleRoleException(OperationType.DELETE, role,
metalake, e);
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
index 708c8603cf..c2436cc284 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
@@ -57,7 +57,7 @@ import org.apache.gravitino.dto.authorization.PrivilegeDTO;
import org.apache.gravitino.dto.authorization.RoleDTO;
import org.apache.gravitino.dto.authorization.SecurableObjectDTO;
import org.apache.gravitino.dto.requests.RoleCreateRequest;
-import org.apache.gravitino.dto.responses.DeleteResponse;
+import org.apache.gravitino.dto.responses.DropResponse;
import org.apache.gravitino.dto.responses.ErrorConstants;
import org.apache.gravitino.dto.responses.ErrorResponse;
import org.apache.gravitino.dto.responses.NameListResponse;
@@ -454,9 +454,9 @@ public class TestRoleOperations extends BaseOperationsTest {
.delete();
Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
- DeleteResponse deleteResponse = resp.readEntity(DeleteResponse.class);
- Assertions.assertEquals(0, deleteResponse.getCode());
- Assertions.assertTrue(deleteResponse.deleted());
+ DropResponse dropResponse = resp.readEntity(DropResponse.class);
+ Assertions.assertEquals(0, dropResponse.getCode());
+ Assertions.assertTrue(dropResponse.dropped());
// Test when failed to delete role
when(manager.deleteRole(any(), any())).thenReturn(false);
@@ -467,9 +467,9 @@ public class TestRoleOperations extends BaseOperationsTest {
.delete();
Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp2.getStatus());
- DeleteResponse deleteResponse2 = resp2.readEntity(DeleteResponse.class);
- Assertions.assertEquals(0, deleteResponse2.getCode());
- Assertions.assertFalse(deleteResponse2.deleted());
+ DropResponse dropResponse2 = resp2.readEntity(DropResponse.class);
+ Assertions.assertEquals(0, dropResponse2.getCode());
+ Assertions.assertFalse(dropResponse2.dropped());
doThrow(new RuntimeException("mock
error")).when(manager).deleteRole(any(), any());
Response resp3 =
