Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Nov 20, 2024

Conversation

luigirende
Copy link
Contributor

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 #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: #3575

Checklist

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

@luigirende luigirende requested review from a team as code owners October 21, 2024 16:48
@luigirende luigirende force-pushed the fix/mongo-transational-multi branch from 776fbe3 to b1bfd26 Compare October 24, 2024 12:35
@berndverst
Copy link
Member

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 .

Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@luigirende
Copy link
Contributor Author

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 luigirende force-pushed the fix/mongo-transational-multi branch from b1bfd26 to a49eee8 Compare October 26, 2024 09:43
@luigirende
Copy link
Contributor Author

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

Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conformance test breaks Redis state store. This is a good catch - that's what conformance tests are for after all.

If you have bandwidth, could you try to debug why this test fails for redis? I'm sure the fix for Redis 6 addresses this also for Redis 7.

@luigirende luigirende force-pushed the fix/mongo-transational-multi branch from a49eee8 to af8bd78 Compare October 28, 2024 18:35
@luigirende
Copy link
Contributor Author

luigirende commented Oct 28, 2024

@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
Copy link
Contributor Author

@berndverst any feedback about the PR?

@yaron2 yaron2 merged commit f3bd794 into dapr:main Nov 20, 2024
90 checks passed
@marcduiker
Copy link
Contributor

@holopin-bot @luigirende Thank you!

Copy link

holopin-bot bot commented Dec 3, 2024

Congratulations @luigirende, the maintainer of this repository has issued you a badge! Here it is: https://holopin.io/claim/cm487ck8q29050cjmqh36kpvg

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants