You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@helix.apache.org by ji...@apache.org on 2022/01/07 22:21:54 UTC
[helix] branch master updated: Avoid NPE when getting property store through Helix-rest API. (#1929)
This is an automated email from the ASF dual-hosted git repository.
jiajunwang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git
The following commit(s) were added to refs/heads/master by this push:
new f6aad0d Avoid NPE when getting property store through Helix-rest API. (#1929)
f6aad0d is described below
commit f6aad0db356716dc4647a2bedf4a65b6e75f77bb
Author: Jiajun Wang <jj...@linkedin.com>
AuthorDate: Fri Jan 7 14:21:45 2022 -0800
Avoid NPE when getting property store through Helix-rest API. (#1929)
This PR aims to fix the ambiguous error return message when user request to get an empty ZK node through the Helix-rest property store access API.
This PR changes the server behavior to response NO_CONTENT instead of internal_server_error in the scenarios described above.
---
.../resources/helix/PropertyStoreAccessor.java | 29 +++++++++++++---------
.../rest/server/TestPropertyStoreAccessor.java | 16 +++++++++---
.../rest/server/util/JerseyUriRequestBuilder.java | 16 ++++++------
3 files changed, 39 insertions(+), 22 deletions(-)
diff --git a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PropertyStoreAccessor.java b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PropertyStoreAccessor.java
index 78ed112..3dbb273 100644
--- a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PropertyStoreAccessor.java
+++ b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PropertyStoreAccessor.java
@@ -73,19 +73,24 @@ public class PropertyStoreAccessor extends AbstractHelixResource {
}
final String recordPath = PropertyPathBuilder.propertyStore(clusterId) + path;
BaseDataAccessor<byte[]> propertyStoreDataAccessor = getByteArrayDataAccessor();
- if (propertyStoreDataAccessor.exists(recordPath, AccessOption.PERSISTENT)) {
- byte[] bytes = propertyStoreDataAccessor.get(recordPath, null, AccessOption.PERSISTENT);
- ZNRecord znRecord = (ZNRecord) ZN_RECORD_SERIALIZER.deserialize(bytes);
- // The ZNRecordSerializer returns null when exception occurs in deserialization method
- if (znRecord == null) {
- ObjectNode jsonNode = OBJECT_MAPPER.createObjectNode();
- jsonNode.put(CONTENT_KEY, new String(bytes));
- return JSONRepresentation(jsonNode);
- }
- return JSONRepresentation(znRecord);
- } else {
+ if (!propertyStoreDataAccessor.exists(recordPath, AccessOption.PERSISTENT)) {
throw new WebApplicationException(Response.status(Response.Status.NOT_FOUND)
- .entity(String.format("The property store path %s doesn't exist", recordPath)).build());
+ .entity(String.format("The property store path %s doesn't exist", recordPath))
+ .build());
+ }
+ byte[] bytes = propertyStoreDataAccessor.get(recordPath, null, AccessOption.PERSISTENT);
+ if (bytes == null) {
+ throw new WebApplicationException(Response.status(Response.Status.NO_CONTENT).build());
+ }
+ ZNRecord znRecord = (ZNRecord) ZN_RECORD_SERIALIZER.deserialize(bytes);
+ // The ZNRecordSerializer returns null when exception occurs in deserialization method
+ if (znRecord == null) {
+ // If the zk node cannot be deserialized, return the content directly.
+ ObjectNode jsonNode = OBJECT_MAPPER.createObjectNode();
+ jsonNode.put(CONTENT_KEY, new String(bytes));
+ return JSONRepresentation(jsonNode);
+ } else {
+ return JSONRepresentation(znRecord);
}
}
diff --git a/helix-rest/src/test/java/org/apache/helix/rest/server/TestPropertyStoreAccessor.java b/helix-rest/src/test/java/org/apache/helix/rest/server/TestPropertyStoreAccessor.java
index d891989..36ae59b 100644
--- a/helix-rest/src/test/java/org/apache/helix/rest/server/TestPropertyStoreAccessor.java
+++ b/helix-rest/src/test/java/org/apache/helix/rest/server/TestPropertyStoreAccessor.java
@@ -49,6 +49,8 @@ public class TestPropertyStoreAccessor extends AbstractTestClass {
private static final ZNRecord TEST_ZNRECORD = new ZNRecord("TestContent");
private static final String CUSTOM_PATH =
PropertyPathBuilder.propertyStore(TEST_CLUSTER) + "/NonZnRecord";
+ private static final String EMPTY_PATH =
+ PropertyPathBuilder.propertyStore(TEST_CLUSTER) + "/EmptyNode";
private static final String TEST_CONTENT = "TestContent";
private static final String CONTENT_KEY = "content";
@@ -71,6 +73,7 @@ public class TestPropertyStoreAccessor extends AbstractTestClass {
Assert
.assertTrue(_customDataAccessor.create(CUSTOM_PATH, TEST_CONTENT, AccessOption.PERSISTENT));
Assert.assertTrue(_baseAccessor.create(ZNRECORD_PATH, TEST_ZNRECORD, AccessOption.PERSISTENT));
+ Assert.assertTrue(_baseAccessor.create(EMPTY_PATH, null, AccessOption.EPHEMERAL));
}
@AfterClass
@@ -81,6 +84,13 @@ public class TestPropertyStoreAccessor extends AbstractTestClass {
}
@Test
+ public void testGetPropertyStoreWithEmptyContent() {
+ String data = new JerseyUriRequestBuilder("clusters/{}/propertyStore/EmptyNode").format(TEST_CLUSTER)
+ .expectedReturnStatusCode(Response.Status.NO_CONTENT.getStatusCode()).get(this);
+ Assert.assertTrue(data.isEmpty());
+ }
+
+ @Test
public void testGetPropertyStoreWithZNRecordData() throws IOException {
String data =
new JerseyUriRequestBuilder("clusters/{}/propertyStore/ZnRecord").format(TEST_CLUSTER)
@@ -91,9 +101,9 @@ public class TestPropertyStoreAccessor extends AbstractTestClass {
@Test
public void testGetPropertyStoreWithTestStringData() throws IOException {
- String actual =
- new JerseyUriRequestBuilder("clusters/{}/propertyStore/NonZnRecord").format(TEST_CLUSTER)
- .isBodyReturnExpected(true).get(this);
+ String actual = new JerseyUriRequestBuilder("clusters/{}/propertyStore/NonZnRecord").format(TEST_CLUSTER)
+ .isBodyReturnExpected(true)
+ .get(this);
JsonNode jsonNode = OBJECT_MAPPER.readTree(actual);
String payLoad = jsonNode.get(CONTENT_KEY).textValue();
diff --git a/helix-rest/src/test/java/org/apache/helix/rest/server/util/JerseyUriRequestBuilder.java b/helix-rest/src/test/java/org/apache/helix/rest/server/util/JerseyUriRequestBuilder.java
index 456ecaf..b1597e6 100644
--- a/helix-rest/src/test/java/org/apache/helix/rest/server/util/JerseyUriRequestBuilder.java
+++ b/helix-rest/src/test/java/org/apache/helix/rest/server/util/JerseyUriRequestBuilder.java
@@ -94,13 +94,15 @@ public class JerseyUriRequestBuilder {
Assert.assertEquals(response.getStatus(), _expectedStatusCode);
- // NOT_FOUND will throw text based html
- if (_expectedStatusCode != Response.Status.NOT_FOUND.getStatusCode()
- && _expectedStatusCode != Response.Status.BAD_REQUEST.getStatusCode()) {
- Assert.assertEquals(response.getMediaType().getType(), "application");
- } else {
- Assert.assertEquals(response.getMediaType().getType(), "text");
- }
+ if (_expectedStatusCode != Response.Status.NO_CONTENT.getStatusCode()) {
+ // NOT_FOUND will throw text based html
+ if (_expectedStatusCode != Response.Status.NOT_FOUND.getStatusCode()
+ && _expectedStatusCode != Response.Status.BAD_REQUEST.getStatusCode()) {
+ Assert.assertEquals(response.getMediaType().getType(), "application");
+ } else {
+ Assert.assertEquals(response.getMediaType().getType(), "text");
+ }
+ } // else, NO_CONTENT should not return any content, so no need to check the type.
String body = response.readEntity(String.class);
if (_isBodyReturnExpected) {