dapr / components-contrib

Community driven, reusable components for distributed apps
Apache License 2.0
548 stars 480 forks source link

Mongo State: fix serialization value in the transaction method #3576

Closed luigirende closed 5 days ago

luigirende commented 1 month ago

Description

This PR to fix the issue about the serialization of the value in the transactional request when the contentType is equals to application/json as described in https://github.com/dapr/components-contrib/issues/3575

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: https://github.com/dapr/components-contrib/issues/3575

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

berndverst commented 1 month ago

I tried this many times but something was always broken with serialization and didn't have time to further investigate.

Mongo serialization is very very finnicky because BSON doesn't perfectly map to JSON and to Dapr's API.

Make sure to run both certification tests and conformance tests after making any changes!

go test -v -tags=conftests -count=1 ./tests/conformance -run="TestStateConformance/mongodb"

and also from the tests/certification/state/mongodb folder go test -v --count=1 .

luigirende commented 1 month ago

I tried this many times but something was always broken with serialization and didn't have time to further investigate.

Mongo serialization is very very finnicky because BSON doesn't perfectly map to JSON and to Dapr's API.

Make sure to run both certification tests and conformance tests after making any changes!

go test -v -tags=conftests -count=1 ./tests/conformance -run="TestStateConformance/mongodb"

and also from the tests/certification/state/mongodb folder go test -v --count=1 .

About the test I did and were passed, in fact you can see the job in actions passed the test about confromance https://github.com/dapr/components-contrib/actions/runs/11499402962/job/32020955182?pr=3576

luigirende commented 1 month ago

Please add a test case in conformance tests that can execute this particular scenario / new code path added here.

Make sure that test doesn't break other components however.

https://github.com/dapr/components-contrib/blob/main/tests/conformance/state/state.go

@berndverst added the test in conformance

luigirende commented 4 weeks ago

@berndverst I ran the tests locally and found that the issue with Redis 6 is related to the contentType I added in the Multi method. Specifically, the value stored in Redis is in Base64 format when the data request is represented as a byte array. I did a check in the response to decode the result and validate the content. As for Redis 7, when using the Multi method, the contentType is set to JSON; however, the version used in the conformance does not support this feature, in this case I have disabled the test and accept the test if the query feature is enabled. In fact in redis when the Json is a feature enabled the query api is active and however for Mongo this test is always done.

luigirende commented 2 weeks ago

@berndverst any feedback about the PR?