Is it necessary to do strong check on fastjson?

[jasonjoo2010]

Issue Description

Rules' deserialization will be interrupted with old version of fastjson due to the bug on inherited bean classes. Should we add some kinds of force check on it?

[sczyh30]

Yes, it's really necessary as the deserialization problem of old fastjson versions often makes users confusing. Maybe we can add checking logic in relevant command handler and carry the error info in the command response, so that dashboard can get the error status. Any suggestions?

[jasonjoo2010]

The issue doesn't only occur during operations in dashboard but also loading rules from storages into application. I think putting it into sentinel initializing process is efficient because there're no such common suitable place in sentinel-datasource-extension etc. To prevent unused dependency we can use reflecting.

What do you think about?

And more i lock the minor version by tests fastjson-1.2.12

[sczyh30]

The issue doesn't only occur during operations in dashboard but also loading rules from storages into application. I think putting it into sentinel initializing process is efficient because there're no such common suitable place in sentinel-datasource-extension etc. To prevent unused dependency we can use reflecting.

What do you think about?

And more i lock the minor version by tests fastjson-1.2.12

That's okay, but just warn in the log might not be enough. We may also need to modify the rule command handlers in transport module to carry the error info in the command response.

PS: fastjson is introduced in transport common module, so maybe we can put the InitFunc to this module? (this cannot cover the users that don't use transport module, but i think it's okay, as the JSON library and related logic are not necessary in sentinel-core).

[jasonjoo2010]

The issue doesn't only occur during operations in dashboard but also loading rules from storages into application. I think putting it into sentinel initializing process is efficient because there're no such common suitable place in sentinel-datasource-extension etc. To prevent unused dependency we can use reflecting. What do you think about? And more i lock the minor version by tests fastjson-1.2.12

That's okay, but just warn in the log might not be enough. We may also need to modify the rule command handlers in transport module to carry the error info in the command response.

PS: fastjson is introduced in transport common module, so maybe we can put the InitFunc to this module? (this cannot cover the users that don't use transport module, but i think it's okay, as the JSON library and related logic are not necessary in sentinel-core).

Maybe it's still not enough. Fastjson is used in several places including transport and datasource which are important components. I may show you PR #742 for proposal. An exception will be thrown during doInit() which covers all scenarios that have sentinel. And it will skip the test automatically when you don't need a fastjson at all.

[jasonjoo2010]

The issue doesn't only occur during operations in dashboard but also loading rules from storages into application. I think putting it into sentinel initializing process is efficient because there're no such common suitable place in sentinel-datasource-extension etc. To prevent unused dependency we can use reflecting. What do you think about? And more i lock the minor version by tests fastjson-1.2.12

That's okay, but just warn in the log might not be enough. We may also need to modify the rule command handlers in transport module to carry the error info in the command response.

PS: fastjson is introduced in transport common module, so maybe we can put the InitFunc to this module? (this cannot cover the users that don't use transport module, but i think it's okay, as the JSON library and related logic are not necessary in sentinel-core).

Indeed sentinel-core doesn't need fastjson library. The bug occurs when do deserialization as figure:

And as we know the effected functions are listed below:

Which are pointed out in red.

Considering there maybe other problems not mentioned we can choose:

1. Do necessary check in datasource/transport modules and maybe package the check logic into sentinel-core. Implement it once but call it other places.
2. Introducing the check into doInit() through reflection to prevent introducing new dependency.

More i should point out is the bug will effects the deserializing of all the class having one or more parents. That means there will be some problems hiding except we consist on POJO class.

What's your opinion?

[jasonjoo2010]

@shxz130

Any further opinion?

Like the flow shown in last post i still think the check should be done in sentinel-core when initializing. But both strong or logging level are fine for me. Or some eye-catching logging content.

Certainly we can also change the beans which need serializing and deserializing into simple POJO with no inherited properties. But maybe it's a little complex and prompting the issued fastjson to user is still good for them because they may suffer it in other place.

If you have difference or better suggestions please let me know.

[shxz130]

i think you need @sczyh30 ‘s reply，not me。

[jasonjoo2010]

i think you need @sczyh30 ‘s reply，not me。

Sorry for my negligence and I apologize for it.

@sczyh30 And sorry for quoting the wrong name. You can read my previous reply and give me your suggestions.

Thank you all.

[sczyh30]

Sorry for the late reply.

In fact, transport and extension modules are optional (where the deserialization will occur), so I don't think it's appropriate to check for fastjson in sentinel-core (though it's useful).

And as we've discussed in another PR, maybe we could deprecate the AbstractRule in later versions and make every rule type individual. Some legacy logic in AbstractRule (e.g. helper methods to compare limitApp) should be tackled carefully. What do you think of it?

[jasonjoo2010]

Sorry for the late reply.

In fact, transport and extension modules are optional (where the deserialization will occur), so I don't think it's appropriate to check for fastjson in sentinel-core (though it's useful).

And as we've discussed in another PR, maybe we could deprecate the AbstractRule in later versions and make every rule type individual. Some legacy logic in AbstractRule (e.g. helper methods to compare limitApp) should be tackled carefully. What do you think of it?

Indeed it can be degraded to a warning indicating the bad fastjson. And i think it can be move to process of initializing of datasource rather than transport if we don't do it in core module because datasource module is more necessary than it. Using transport but without datasource maybe rarely seen. What about with it?

[jasonjoo2010]

Sorry for the late reply. In fact, transport and extension modules are optional (where the deserialization will occur), so I don't think it's appropriate to check for fastjson in sentinel-core (though it's useful). And as we've discussed in another PR, maybe we could deprecate the AbstractRule in later versions and make every rule type individual. Some legacy logic in AbstractRule (e.g. helper methods to compare limitApp) should be tackled carefully. What do you think of it?

Indeed it can be degraded to a warning indicating the bad fastjson. And i think it can be move to process of initializing of datasource rather than transport if we don't do it in core module because datasource module is more necessary than it. Using transport but without datasource maybe rarely seen. What about with it?

Certainly why it can be degraded into warning one important reason is that we may implement kind of SPI interface on logging so something like SCA will merge the log into the main application log. By this users could find the warning or error log obviously.