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

Do not type convert data to string for transaction API. #2010

Merged
merged 6 commits into from
Sep 15, 2020

Conversation

mukundansundar
Copy link
Contributor

@mukundansundar mukundansundar commented Sep 8, 2020

Description

Found an issue for Transaction API via Java Integration Tests.

When the state is stored via an upsert call made and the got back via the normal getState API call, it can be observed that there is an extra Json Marshal happening in the code.

Example:

data: "\"{\\\"propertyA\\\":\\\"data in property A\\\",\\\"propertyB\\\":\\\"data in property B\\\"}\""

Instead of

data: "{\"propertyA\":\"data in property A\",\"propertyB\":\"data in property B\"}"

The second output is what we have when we have a normal save and get state call done via Dapr.

The code change here, checks if the incoming request value is a byte array or not and then properly serializes the value.

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

Checklist

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

@mukundansundar
Copy link
Contributor Author

Code still needs the change from components contrib to be merged first and ref updated in this repo.

@mukundansundar
Copy link
Contributor Author

/ok-to-test

@dapr-bot
Copy link
Collaborator

Found the available test cluster - dapr-aks-e2e-07 for linux. Please wait until test is done.

@dapr-bot
Copy link
Collaborator

Found the available test cluster - dapr-aks-e2e-08 for windows. Please wait until test is done.

@dapr-bot
Copy link
Collaborator

End-to-end tests failed on linux.

@dapr-bot
Copy link
Collaborator

End-to-end tests cancelled on windows.

@mukundansundar
Copy link
Contributor Author

Need to run the E2E tests only after the new components contrib is merged in. Will be updating this PR once that code is merged in.

@mukundansundar
Copy link
Contributor Author

/ok-to-test

@dapr-bot
Copy link
Collaborator

Found the available test cluster - dapr-aks-e2e-08 for linux. Please wait until test is done.

@dapr-bot
Copy link
Collaborator

End-to-end tests cancelled on linux.

@mukundansundar
Copy link
Contributor Author

/ok-to-test

@dapr-bot
Copy link
Collaborator

Found the available test cluster - dapr-aks-e2e-05 for linux. Please wait until test is done.

@dapr-bot
Copy link
Collaborator

End-to-end tests cancelled on linux.

@mukundansundar
Copy link
Contributor Author

/ok-to-test

@dapr-bot
Copy link
Collaborator

Found the available test cluster - dapr-aks-e2e-05 for linux. Please wait until test is done.

@dapr-bot
Copy link
Collaborator

Found the available test cluster - dapr-aks-e2e-06 for windows. Please wait until test is done.

@dapr-bot
Copy link
Collaborator

Congrats! All end-to-end tests have passed on linux. Thanks for your contribution!

@mukundansundar mukundansundar marked this pull request as ready for review September 15, 2020 21:13
@dapr-bot
Copy link
Collaborator

Congrats! All end-to-end tests have passed on windows. Thanks for your contribution!

Copy link
Member

@mchmarny mchmarny left a comment

Choose a reason for hiding this comment

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

/lgtm

@mukundansundar mukundansundar merged commit afbe726 into dapr:master Sep 15, 2020
@mukundansundar mukundansundar deleted the fix-serde-transsaction-api branch September 15, 2020 22:30
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.

Serialization issues with Transaction upsert operation
7 participants