[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6629: GEODE-9384: Fix exception messages in PdxToJSON and JsonWriter

[GitBox <gi...@apache.org>]

dschneider-pivotal commented on a change in pull request #6629:
URL: https://github.com/apache/geode/pull/6629#discussion_r655724164



##########
File path: geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxToJSON.java
##########
@@ -135,17 +135,16 @@ private void writeValue(JsonGenerator jg, Object value, String pf)
       jg.writeString(value.toString());
     } else if (value.getClass().equals(PdxInstanceEnumInfo.class)) {
       jg.writeString(value.toString());
+    } else if (value instanceof PdxInstance) {
+      getJSONString(jg, (PdxInstance) value);
+    } else if (value instanceof Collection) {
+      getJSONStringFromCollection(jg, (Collection<?>) value, pf);
+    } else if (value instanceof Map) {
+      getJSONStringFromMap(jg, (Map) value, pf);
     } else {
-      if (value instanceof PdxInstance) {
-        getJSONString(jg, (PdxInstance) value);
-      } else if (value instanceof Collection) {
-        getJSONStringFromCollection(jg, (Collection<?>) value, pf);
-      } else if (value instanceof Map) {
-        getJSONStringFromMap(jg, (Map) value, pf);
-      } else {
-        throw new IllegalStateException(
-            "PdxInstance returns unknwon pdxfield " + pf + " for type " + value);
-      }
+      throw new IllegalStateException(
+          "PdxInstance returns unsupported type " + value.getClass()

Review comment:
       I think this message should make clear what we mean by "unsupported". Pdx supports it but we don't support converting it to JSON. So something like: "The pdx field 'FIELD_NAME' has a value whose type 'TYPE' can not be converted to JSON. The value is 'VALUE'."
   You need to add a unit test for this message

##########
File path: geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxToJSON.java
##########
@@ -226,7 +225,8 @@ private void getJSONStringFromArray(JsonGenerator jg, Object value, String pf)
       jg.writeEndArray();
     } else {
       throw new IllegalStateException(
-          "PdxInstance returns unknown pdxfield " + pf + " for type " + value);
+          "PdxInstance returns unsupported type " + value.getClass()

Review comment:
       The caller already made sure value.getClass().isArray() was true. So this message should say something about the array element type not being supported by JSON. Given a Class instance that isArray() you can call getComponentType() it to get the class that describes the element type. The message could say that the componentType is not supported for JSON. Also keep in mind that "value" is an array and toString on an array is not very helpful (just prints out the class name and its hashCode but not the elements of the array).
   Also you need a unit test for this one

##########
File path: geode-web-api/src/test/java/org/apache/geode/rest/internal/web/util/JsonWriterTest.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.geode.rest.internal.web.util;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Fail.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.IOException;
+import java.util.Date;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.test.junit.categories.RestAPITest;
+
+@Category({RestAPITest.class})
+public class JsonWriterTest {
+  @Test
+  public void testWriteValueAsJsonException() throws IOException {
+    try {
+      JsonWriter.writeArrayAsJson(mock(JsonGenerator.class), new Date(), "test");
+      fail("should not succeed");
+    } catch (IllegalStateException e) {
+      assertThat(e.getMessage()).contains("java.util.Date");

Review comment:
       you should be more specific about the message since that is the main thing you are testing here

##########
File path: geode-web-api/src/main/java/org/apache/geode/rest/internal/web/util/JsonWriter.java
##########
@@ -216,7 +216,8 @@ public static void writeArrayAsJson(JsonGenerator generator, Object value, Strin
       writeObjectArrayAsJson(generator, (Object[]) value, pdxField);
     } else {
       throw new IllegalStateException(
-          "PdxInstance returns unknwon pdxfield " + pdxField + " for type " + value);
+          "PdxInstance returns unsupported type " + value.getClass()

Review comment:
       pdxField can be null. Are you okay with this message just saying "for field null" in that case? I think this message should say that it is trying to write an array as JSON and found a "value" whose "type" can not be written as an array. The only caller I found of it already checked that value.getClass().isArray() is true so we just have an array that does not match all the checks this code does. I think you should test this with an array that will fail instead of a Date. Try a Date[]. 
   By the way, the last "else if" on line 215 looks buggy to me. If you look at writeObjectArrayAsJson you will see it just iterates the array and delegates to writeValueAsJson which supports many more types than are check for here (for example an array of Link or an array of Map). If your array was declared as Object[], you could then put Link instances into it and this code would call writeObjectArrayAsJson. But if instead it declared it as Link[] I think this code would throw. It seems like it should work with any type of array as long as it contains values that writeValueAsJson can handle. But I'm not sure how the other direction works (going from JSON back to a java array). 




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org