Copilot commented on code in PR #7760: URL: https://github.com/apache/incubator-seata/pull/7760#discussion_r2558237542
########## json-common/src/main/java/org/apache/seata/common/json/JsonSerializer.java: ########## @@ -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. + */ +package org.apache.seata.common.json; + +import java.lang.reflect.Type; + +/** + * The json serializer interface. + */ +public interface JsonSerializer { + + String toJSONString(Object object); + + <T> T parseObject(String text, Class<T> clazz); + + <T> T parseObject(String text, Type type); + + boolean useAutoType(String json); + + String toJSONString(Object o, boolean prettyPrint); + + String toJSONString(Object o, boolean ignoreAutoType, boolean prettyPrint); + Review Comment: The interface methods lack JavaDoc comments. Consider adding documentation for each method, especially for the overloaded methods with additional parameters like `prettyPrint` and `ignoreAutoType`, to clarify their purpose and usage. For example: - What does `useAutoType` check for and return? - What's the difference between `toJSONString(Object o)` and `toJSONString(Object o, boolean prettyPrint)`? - What does the `ignoreAutoType` parameter control in `parseObject` and `toJSONString` methods? ```suggestion /** * Serializes the specified object into its JSON string representation. * * @param object the object to serialize * @return the JSON string representation of the object */ String toJSONString(Object object); /** * Deserializes the specified JSON string into an object of the given class type. * * @param text the JSON string to parse * @param clazz the class of T * @param <T> the type of the desired object * @return the deserialized object of type T */ <T> T parseObject(String text, Class<T> clazz); /** * Deserializes the specified JSON string into an object of the given type. * * @param text the JSON string to parse * @param type the type to deserialize into * @param <T> the type of the desired object * @return the deserialized object of type T */ <T> T parseObject(String text, Type type); /** * Checks whether the given JSON string uses auto type features (such as type information for polymorphic deserialization). * * @param json the JSON string to check * @return true if auto type is used in the JSON, false otherwise */ boolean useAutoType(String json); /** * Serializes the specified object into its JSON string representation. * * @param o the object to serialize * @param prettyPrint whether to format the JSON string for readability * @return the JSON string representation of the object */ String toJSONString(Object o, boolean prettyPrint); /** * Serializes the specified object into its JSON string representation, with options to ignore auto type and pretty print. * * @param o the object to serialize * @param ignoreAutoType whether to ignore auto type information during serialization * @param prettyPrint whether to format the JSON string for readability * @return the JSON string representation of the object */ String toJSONString(Object o, boolean ignoreAutoType, boolean prettyPrint); /** * Deserializes the specified JSON string into an object of the given class type, with an option to ignore auto type information. * * @param json the JSON string to parse * @param type the class of T * @param ignoreAutoType whether to ignore auto type information during deserialization * @param <T> the type of the desired object * @return the deserialized object of type T */ ``` ########## saga/seata-saga-spring/src/main/java/org/apache/seata/saga/engine/invoker/impl/SpringBeanServiceInvoker.java: ########## @@ -311,16 +311,16 @@ protected Object toJavaObject(Object value, Class paramType) { } else if (isPrimitive(paramType)) { return value; } else { - JsonParser jsonParser = JsonParserFactory.getJsonParser(getSagaJsonParser()); - if (jsonParser == null) { + JsonSerializer jsonSerializer = JsonSerializerFactory.getSerializer(getSagaJsonParser()); + if (jsonSerializer == null) { throw new RuntimeException("Cannot get JsonParser by name : " + getSagaJsonParser()); Review Comment: The error message refers to "JsonParser" but should refer to "JsonSerializer" to match the actual class name being used and the variable name `jsonSerializer` on line 314. This inconsistency could confuse developers debugging issues. ```suggestion throw new RuntimeException("Cannot get JsonSerializer by name : " + getSagaJsonParser()); ``` ########## json-common/src/main/java/org/apache/seata/common/json/impl/FastjsonJsonSerializer.java: ########## @@ -0,0 +1,144 @@ +/* + * 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.seata.common.json.impl; + +import com.alibaba.fastjson.JSON; +import com.alibaba.fastjson.parser.Feature; +import com.alibaba.fastjson.serializer.SerializerFeature; +import org.apache.seata.common.exception.JsonParseException; +import org.apache.seata.common.json.JsonSerializer; +import org.apache.seata.common.loader.LoadLevel; + +import java.lang.reflect.Type; + +/** + * FastJSON implementation of JsonSerializer + */ +@LoadLevel(name = FastjsonJsonSerializer.NAME) +public class FastjsonJsonSerializer implements JsonSerializer { + + private static final SerializerFeature[] SERIALIZER_FEATURES = new SerializerFeature[] { + SerializerFeature.DisableCircularReferenceDetect, + SerializerFeature.WriteDateUseDateFormat, + SerializerFeature.WriteClassName + }; + + private static final SerializerFeature[] SERIALIZER_FEATURES_PRETTY = new SerializerFeature[] { + SerializerFeature.DisableCircularReferenceDetect, + SerializerFeature.WriteDateUseDateFormat, + SerializerFeature.WriteClassName, + SerializerFeature.PrettyFormat + }; + + private static final SerializerFeature[] FEATURES_PRETTY = new SerializerFeature[] { + SerializerFeature.DisableCircularReferenceDetect, + SerializerFeature.WriteDateUseDateFormat, + SerializerFeature.PrettyFormat + }; + + private static final Feature[] READER_FEATURES_SUPPORT_AUTO_TYPE = + new Feature[] {Feature.SupportAutoType, Feature.OrderedField}; + + private static final Feature[] READER_FEATURES_IGNORE_AUTO_TYPE = + new Feature[] {Feature.IgnoreAutoType, Feature.OrderedField}; + + public static final String NAME = "fastjson"; + + @Override + public String toJSONString(Object object) { + try { + return JSON.toJSONString(object); + } catch (Exception e) { + throw new JsonParseException("FastJSON serialize error", e); + } + } + + @Override + public <T> T parseObject(String text, Class<T> clazz) { + if (text == null || clazz == null) { + return null; + } + try { + return JSON.parseObject(text, clazz); + } catch (Exception e) { + throw new JsonParseException("FastJSON deserialize error", e); + } + } + + @Override + public <T> T parseObject(String text, Type type) { + if (text == null || type == null) { + return null; + } + try { + return JSON.parseObject(text, type); + } catch (Exception e) { + throw new JsonParseException("FastJSON deserialize error", e); + } + } + + // advanced methods for Saga + @Override + public boolean useAutoType(String json) { + return json != null && json.contains("\"@type\""); + } + + @Override + public String toJSONString(Object object, boolean prettyPrint) { + return toJSONString(object, false, prettyPrint); + } + + @Override + public String toJSONString(Object object, boolean ignoreAutoType, boolean prettyPrint) { + try { + if (prettyPrint) { + if (ignoreAutoType) { + return JSON.toJSONString(object, FEATURES_PRETTY); + } else { + return JSON.toJSONString(object, SERIALIZER_FEATURES_PRETTY); + } + } else { + if (ignoreAutoType) { + return JSON.toJSONString(object); + } else { + return JSON.toJSONString(object, SERIALIZER_FEATURES); + } + } + } catch (Exception e) { + throw new JsonParseException("FastJSON serialize error", e); + } + } + + @Override + public <T> T parseObject(String text, Class<T> type, boolean ignoreAutoType) { + if (text == null || type == null) { + return null; + } + try { + if ("[]".equals(text)) { + return (T) java.util.Collections.emptyList(); Review Comment: Inconsistent empty list handling across implementations. FastjsonJsonSerializer returns `Collections.emptyList()` (immutable) while JacksonJsonSerializer returns `new ArrayList<>(0)` (mutable) for the same input "[]". This inconsistency could cause runtime errors if code expects a mutable list. All implementations should return the same type of list for consistency. Consider standardizing on either mutable or immutable empty lists across all serializers. ```suggestion return (T) new java.util.ArrayList<>(); ``` ########## json-common/src/main/resources/META-INF.services/org.apache.seata.common.json.JsonSerializer: ########## @@ -0,0 +1,19 @@ +# +# 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. +# +org.apache.seata.common.json.impl.FastjsonJsonSerializer +org.apache.seata.common.json.impl.JacksonJsonSerializer +org.apache.seata.common.json.impl.GsonJsonSerializer Review Comment: The SPI service file path is incorrect. It should be `META-INF/services/` (with a forward slash) instead of `META-INF.services/` (with a dot). This incorrect path will prevent the Java ServiceLoader from discovering the implementations, causing the service loading to fail at runtime. ########## json-common/src/main/java/org/apache/seata/common/json/impl/FastjsonJsonSerializer.java: ########## @@ -0,0 +1,144 @@ +/* + * 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.seata.common.json.impl; + +import com.alibaba.fastjson.JSON; +import com.alibaba.fastjson.parser.Feature; +import com.alibaba.fastjson.serializer.SerializerFeature; +import org.apache.seata.common.exception.JsonParseException; +import org.apache.seata.common.json.JsonSerializer; +import org.apache.seata.common.loader.LoadLevel; + +import java.lang.reflect.Type; + +/** + * FastJSON implementation of JsonSerializer + */ +@LoadLevel(name = FastjsonJsonSerializer.NAME) +public class FastjsonJsonSerializer implements JsonSerializer { + + private static final SerializerFeature[] SERIALIZER_FEATURES = new SerializerFeature[] { + SerializerFeature.DisableCircularReferenceDetect, + SerializerFeature.WriteDateUseDateFormat, + SerializerFeature.WriteClassName + }; + + private static final SerializerFeature[] SERIALIZER_FEATURES_PRETTY = new SerializerFeature[] { + SerializerFeature.DisableCircularReferenceDetect, + SerializerFeature.WriteDateUseDateFormat, + SerializerFeature.WriteClassName, + SerializerFeature.PrettyFormat + }; + + private static final SerializerFeature[] FEATURES_PRETTY = new SerializerFeature[] { + SerializerFeature.DisableCircularReferenceDetect, + SerializerFeature.WriteDateUseDateFormat, + SerializerFeature.PrettyFormat + }; + + private static final Feature[] READER_FEATURES_SUPPORT_AUTO_TYPE = + new Feature[] {Feature.SupportAutoType, Feature.OrderedField}; Review Comment: Using `Feature.SupportAutoType` poses a security risk. Fastjson's autoType feature has well-documented security vulnerabilities (CVE-2017-18349, CVE-2019-10072, and others) that can lead to remote code execution. Consider: 1. Documenting the security implications of using `ignoreAutoType=false` 2. Defaulting to `ignoreAutoType=true` where possible 3. Adding validation or whitelisting for allowed types when autoType must be enabled 4. Warning users about the security risks in the API documentation ```suggestion // Removed READER_FEATURES_SUPPORT_AUTO_TYPE due to security risks associated with Feature.SupportAutoType. // See CVE-2017-18349, CVE-2019-10072, and Fastjson documentation for details. ``` ########## saga/seata-saga-statelang/src/main/java/org/apache/seata/saga/statelang/parser/impl/StateMachineParserImpl.java: ########## @@ -58,15 +58,15 @@ public StateMachineParserImpl(String jsonParserName) { @Override public StateMachine parse(String json) { - JsonParser jsonParser = JsonParserFactory.getJsonParser(jsonParserName); - if (jsonParser == null) { + JsonSerializer jsonSerializer = JsonSerializerFactory.getSerializer(jsonParserName); + if (jsonSerializer == null) { throw new RuntimeException("Cannot find JsonParer by name: " + jsonParserName); Review Comment: The error message contains a typo: "JsonParer" should be "JsonSerializer" to match the actual class name being used. ```suggestion throw new RuntimeException("Cannot find JsonSerializer by name: " + jsonParserName); ``` ########## common/src/main/resources/META-INF/services/org.apache.seata.common.json.JsonSerializer: ########## @@ -0,0 +1,19 @@ +# +# 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. +# +org.apache.seata.common.json.impl.FastjsonJsonSerializer +org.apache.seata.common.json.impl.JacksonJsonSerializer +org.apache.seata.common.json.impl.GsonJsonSerializer Review Comment: This SPI service file is placed in the wrong module. The implementations (FastjsonJsonSerializer, JacksonJsonSerializer, GsonJsonSerializer) are located in the `json-common` module, not the `common` module. This SPI file should only exist in the `json-common` module where the implementations actually reside. Having it in the `common` module will cause service loading issues. ```suggestion ``` ########## json-common/src/main/java/org/apache/seata/common/json/impl/GsonJsonSerializer.java: ########## @@ -0,0 +1,116 @@ +/* + * 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.seata.common.json.impl; + +import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import org.apache.seata.common.exception.JsonParseException; +import org.apache.seata.common.json.JsonSerializer; +import org.apache.seata.common.loader.LoadLevel; + +import java.lang.reflect.Modifier; +import java.lang.reflect.Type; + +/** + * Gson implementation of JsonSerializer + */ +@LoadLevel(name = GsonJsonSerializer.NAME) +public class GsonJsonSerializer implements JsonSerializer { + + private final Gson gson = new GsonBuilder() + .excludeFieldsWithModifiers(Modifier.STATIC, Modifier.TRANSIENT) + .create(); + + private final Gson gsonPretty = new GsonBuilder() + .excludeFieldsWithModifiers(Modifier.STATIC, Modifier.TRANSIENT) + .setPrettyPrinting() + .create(); + + public static final String NAME = "gson"; + + @Override + public String toJSONString(Object object) { + try { + return gson.toJson(object); + } catch (Exception e) { + throw new JsonParseException("Gson serialize error", e); + } + } + + @Override + public <T> T parseObject(String text, Class<T> clazz) { + if (text == null || clazz == null) { + return null; + } + try { + return gson.fromJson(text, clazz); + } catch (Exception e) { + throw new JsonParseException("Gson deserialize error", e); + } + } + + @Override + public <T> T parseObject(String text, Type type) { + if (text == null || type == null) { + return null; + } + try { + return gson.fromJson(text, type); + } catch (Exception e) { + throw new JsonParseException("Gson deserialize error", e); + } + } + + // advanced methods for Saga + @Override + public boolean useAutoType(String json) { + return false; + } + + @Override + public String toJSONString(Object object, boolean prettyPrint) { + return toJSONString(object, false, prettyPrint); + } + + @Override + public String toJSONString(Object object, boolean ignoreAutoType, boolean prettyPrint) { + try { + if (prettyPrint) { + return gsonPretty.toJson(object); + } else { + return gson.toJson(object); + } + } catch (Exception e) { + throw new JsonParseException("Gson serialize error", e); + } + } + + @Override + public <T> T parseObject(String text, Class<T> type, boolean ignoreAutoType) { + if (text == null || type == null) { + return null; + } + try { + if ("[]".equals(text)) { + return (T) java.util.Collections.emptyList(); Review Comment: Inconsistent empty list handling across implementations. GsonJsonSerializer returns `Collections.emptyList()` (immutable) while JacksonJsonSerializer returns `new ArrayList<>(0)` (mutable) for the same input "[]". This inconsistency could cause runtime errors if code expects a mutable list. All implementations should return the same type of list for consistency. Consider standardizing on either mutable or immutable empty lists across all serializers. ```suggestion return (T) new java.util.ArrayList<>(); ``` ########## json-common/src/main/java/org/apache/seata/common/json/JsonSerializer.java: ########## @@ -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. + */ +package org.apache.seata.common.json; + +import java.lang.reflect.Type; + +/** + * The json serializer interface. + */ +public interface JsonSerializer { + + String toJSONString(Object object); + + <T> T parseObject(String text, Class<T> clazz); + + <T> T parseObject(String text, Type type); Review Comment: Method JsonSerializer.parseObject(..) could be confused with overloaded method [parseObject](1), since dispatch depends on static types. ```suggestion <T> T parseObjectWithType(String text, Type type); ``` ########## json-common/src/test/java/org/apache/seata/common/json/GsonJsonSerializerTest.java: ########## @@ -0,0 +1,241 @@ +/* + * 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.seata.common.json; + +import org.apache.seata.common.exception.JsonParseException; +import org.apache.seata.common.json.impl.GsonJsonSerializer; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.ArrayList; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +public class GsonJsonSerializerTest { + + private JsonSerializer jsonSerializer; + + @BeforeEach + void setUp() { + // Use factory to get Gson serializer by name + jsonSerializer = JsonSerializerFactory.getSerializer("gson"); + } + + @Test + public void testToJSONString_basicObject() { + TestObject obj = new TestObject("test", 123); + String json = jsonSerializer.toJSONString(obj); + + assertThat(json).contains("\"name\":\"test\""); + assertThat(json).contains("\"value\":123"); + assertThat(json).doesNotContain("@type"); + } + + @Test + public void testParseObject_basicObject() { + String json = "{\"name\":\"test\",\"value\":123}"; + TestObject obj = jsonSerializer.parseObject(json, TestObject.class); + + assertThat(obj).isNotNull(); + assertThat(obj.getName()).isEqualTo("test"); + assertThat(obj.getValue()).isEqualTo(123); + } + + @Test + public void testToJSONString_and_parseObject() { + TestObject original = new TestObject("school", 456); + String json = jsonSerializer.toJSONString(original); + TestObject restored = jsonSerializer.parseObject(json, TestObject.class); + + assertThat(restored.getName()).isEqualTo(original.getName()); + assertThat(restored.getValue()).isEqualTo(original.getValue()); + } + + @Test + public void testUseAutoType_withType() { + String json = "{\"@type\":\"some.type\",\"name\":\"test\"}"; + boolean hasAutoType = ((GsonJsonSerializer) jsonSerializer).useAutoType(json); + assertThat(hasAutoType).isFalse(); + } + + @Test + public void testUseAutoType_withoutType() { + String json = "{\"name\":\"test\"}"; + boolean hasAutoType = ((GsonJsonSerializer) jsonSerializer).useAutoType(json); + assertThat(hasAutoType).isFalse(); + } + + @Test + public void testToJSONString_prettyPrint() { Review Comment: The method 'testToJSONString_prettyPrint' may be confused with [testToJsonString_prettyPrint](1). ```suggestion public void testToJsonString_prettyPrint() { ``` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
