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:
[email protected]
