[serialization]Abstract JSON serialization and use gson as default #1739

jasonjoo2010 · 2020-09-16T17:20:58Z

Describe what this PR does / why we need it

Though sentinel-core doesn't need json serialization, but in most sentinel modules (adaption, datasource, etc.) they use fastjson for serializing/deserializing json strings.

Unfortunately there are more and more versions of fastjson which were found vulnerable and users must follow the latest patches or versions. If not their application may become weak and easy to attack.

In the other hand JSON serializing is a common need. So most projects have their own way to do that. In this scene it's not necessary to introduce a new one if sentinel can use existing, compatible serializing library in project.

This PR will enable user letting sentinel to use gson / fastjson / jackson or their own implementation through SPI protocol.

Does this pull request fix one issue?

#1522

Describe how you did it

Introduce a new module sentinel-serialization and take gson as default json serializing/deserializing library.

Add new SPI interface JsonSerializer and JsonDeserializer
The default implementation adapts to gson and fastjson (gson prior)
gson can be excluded manually from dependency tree if use custom implementation
Add a popular implementation sentinel-serialization-jackson

Describe how to verify it

Depends.

Special notes for reviews

A wide regressing test may be necessary.

…fault json serializing/deserializing library. - Add new SPI interface `JsonSerializer` and `JsonDeserializer` - The default implementation adapts to `gson` and `fastjson` (gson prior) - `gson` can be excluded manually from dependency tree if use custom implementation - Add a popular implementation `sentinel-serialization-jackson` Signed-off-by: Jason Joo <[email protected]>

…ds) in gson - Update sentinel-demo-cluster-server-alone to demonstrate an example using fastjson in serialization

zhengpengGitHub · 2021-01-24T14:13:53Z

hello，when does this fix will release?

cdfive · 2021-01-28T07:02:01Z

...src/main/java/com/alibaba/csp/sentinel/serialization/common/fallback/DefaultTransformer.java

+    @Override
+    public String serialize(Object obj) {
+        if (obj == null) {
+            return "null";
+        }
+        try {
+            return this.adaption.serialize(obj);
+        } catch (Exception e) {
+        }
+        return null;
+    }
+
+    @Override
+    public <T> T deserialize(String str, Type type) {
+        if (StringUtil.isBlank(str)) {
+            return null;
+        }
+        try {
+            return this.adaption.deserialize(str, type);
+        } catch (Exception e) {
+        }
+        return null;
+    }


When serialize and deserialize the exception is ignored, do we need to throw an exception to remind the user to know or record the error log? The same is in JacksonTransformer class.

Here just keep the dealing strategy for backward compatibility. (null when failed in most scenarios)
So some logs are needed or a runtime exception should be introduced?

I suggest both throw runtime exception and record error log. Although the exception will be occur in rare case, maybe never, but since we catch the Exception, why not do nothing. If exception occurred, user may want to know what happened. If it's sure that we don't need to deal with it here, suggest adding some comments.

If throw runtime exception, it will affect some test cases.Or just record log? I feel acceptable to ignore the exception,just a thought of it for reference, it's up to you.

cdfive · 2021-01-30T15:23:54Z

...n/src/main/java/com/alibaba/csp/sentinel/serialization/common/fallback/FastjsonAdaption.java

+    public String serialize(Object obj) throws Exception {
+        return (String)METHOD_TO_JSON_STRING.invoke(CLASS, obj);
+    }
+
+    @SuppressWarnings("unchecked")
+    @Override
+    public <T> T deserialize(String str, Type type) throws Exception {
+        return (T)METHOD_PARSE_OBJECT_BY_TYPE.invoke(CLASS, str, type, null);
+    }


Serialization and deserialization methods called by reflection in FastjsonAdaption and GsonAdaption. Another way is that, the fastjson and gson depenency can be with provided scope, and we can use their class and method calls directly. Which way do you think is better? @jasonjoo2010 @sczyh30

cdfive · 2021-01-30T15:30:56Z

...src/main/java/com/alibaba/csp/sentinel/serialization/common/fallback/DefaultTransformer.java

+    public DefaultTransformer() {
+        List<Adaption> adaptions = Arrays.asList(
+            new GsonAdaption(), 
+            new FastjsonAdaption()
+        );


Jackson seems more widely used, Gson as the default first choice may need to be discussed.
sentinel-serialization-jackson is a separate module, why not JasksonAdaption, will it be more convenient to use? Or GsonAdaption, FastjsonAdaption -> sentinel-serialization-gson, sentinel-serialization-fastjson module, it may be a special consideration, but it seems that there are not uniform implementation.

cdfive · 2021-01-30T15:40:41Z

...on/src/main/java/com/alibaba/csp/sentinel/serialization/common/fallback/DateTypeAdapter.java

+import com.google.gson.JsonSerializationContext;
+import com.google.gson.JsonSerializer;
+
+class DateTypeAdapter implements JsonSerializer<Date>, JsonDeserializer<Date> {


GsonDateTypeAdapter may be a better name, or add separate package for gson and fastjson fallback.

brotherlu-xcq · 2021-06-03T09:14:04Z

Hi, @jasonjoo2010. I have some question about this design.

I find that you split deserialize and serialize to two interface. but in our scene，the relationship bewteen deserialize and serialize should be like the front and back of a coin. It maybe better to put them in an interface.
serialize type is not only json, I think design of this module shoud cover other serialize type like hession,etc.

jasonjoo2010 · 2021-06-03T15:17:35Z

Hi, @jasonjoo2010. I have some question about this design.

I find that you split deserialize and serialize to two interface. but in our scene，the relationship bewteen deserialize and serialize should be like the front and back of a coin. It maybe better to put them in an interface.

serialize type is not only json, I think design of this module shoud cover other serialize type like hession,etc.

Hi Lu

First let's skip the first point you mentioned and go over the second point directly.
The purpose of this PR is aiming to decouple the serialising/deserialising on json string against fastjson library because of more and more versions of that have security risks. That's why it focuses on dealing json only.
But if open our minds to other general purpose serialisations it will be too flexible. Which will make things complicated especially the data will be submitted through http transports while hessian/proto/igbinary is binary serialisation. So it's not a RPC case.
So we want to keep json as the standard encoding in transports.

Come back to the first point not in java but in many languages they are two interfaces(eg. golang). You even can only customize one of them instead of both which make more sense. In the other hand you must name it something new if you want to combine them together. So it's not a big deal I think.
Let me use a more detailed example to make it clear. If you use gson but you are not satisfied with the provided implementation because you want all float type to keep up to 2 decimal places. So if you just want to truncate the inputs from outside you only need to implement a new deserialiser.

CLAassistant · 2022-01-26T02:39:13Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

jasonjoo2010 added the dependencies Pull requests that update a dependency file label Sep 16, 2020

jasonjoo2010 mentioned this pull request Sep 17, 2020

fixd #1661: fastjson deserialization problem #1663

Closed

jasonjoo2010 added 7 commits September 17, 2020 20:47

Make ParamFlowThrottleRateLimitingCheckerTest more stable

1d046f8

Compatible with older gson

b334e14

Add README for sentinel-serialization

f74dd3d

Polish README for sentinel-serialization

370e489

Remove trailing space from README of sentinel-serialization

43195da

- Make java.util.Date/java.sql.Date is serialized as long (millisecon…

3614396

…ds) in gson - Update sentinel-demo-cluster-server-alone to demonstrate an example using fastjson in serialization

jasonjoo2010 force-pushed the serialization branch from 6c0345c to 3614396 Compare September 17, 2020 12:48

sczyh30 requested review from linlinisme, cdfive and a team September 17, 2020 13:18

jasonjoo2010 changed the title ~~[serialization]Abstract JSON serialization and default to use gson~~ [serialization]Abstract JSON serialization and use gson as default Sep 18, 2020

sczyh30 added kind/refactor Issue related to functional refactoring. size/XXL Indicate a PR that changes 1000+ lines. to-review To review labels Sep 19, 2020

This was referenced Oct 27, 2020

1.8.0 sentinel-datasource-zookeeper的测试类中,Json转对象会丢失resource字段，切换成jackson后正常 #1824

Closed

[bugfixed] fixes issue #1065, modified sentinel-dubbo-adapter module … #1794

Merged

cdfive reviewed Jan 28, 2021

View reviewed changes

cdfive reviewed Jan 30, 2021

View reviewed changes

sczyh30 requested a review from zhaoyuguang July 5, 2021 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[serialization]Abstract JSON serialization and use gson as default #1739

[serialization]Abstract JSON serialization and use gson as default #1739

jasonjoo2010 commented Sep 16, 2020

zhengpengGitHub commented Jan 24, 2021

cdfive Jan 28, 2021

jasonjoo2010 Jan 28, 2021

cdfive Jan 30, 2021

cdfive Jan 30, 2021 •

edited

Loading

cdfive Jan 30, 2021

cdfive Jan 30, 2021 •

edited

Loading

cdfive Jan 30, 2021

brotherlu-xcq commented Jun 3, 2021

jasonjoo2010 commented Jun 3, 2021

CLAassistant commented Jan 26, 2022

[serialization]Abstract JSON serialization and use gson as default #1739

Are you sure you want to change the base?

[serialization]Abstract JSON serialization and use gson as default #1739

Conversation

jasonjoo2010 commented Sep 16, 2020

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

zhengpengGitHub commented Jan 24, 2021

cdfive Jan 28, 2021

Choose a reason for hiding this comment

jasonjoo2010 Jan 28, 2021

Choose a reason for hiding this comment

cdfive Jan 30, 2021

Choose a reason for hiding this comment

cdfive Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

cdfive Jan 30, 2021

Choose a reason for hiding this comment

cdfive Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

cdfive Jan 30, 2021

Choose a reason for hiding this comment

brotherlu-xcq commented Jun 3, 2021

jasonjoo2010 commented Jun 3, 2021

CLAassistant commented Jan 26, 2022

cdfive Jan 30, 2021 •

edited

Loading

cdfive Jan 30, 2021 •

edited

Loading