Skip to content

Use state serializer content type when saving actor states. #1033

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

Conversation

antoniomaria
Copy link
Contributor

@antoniomaria antoniomaria commented Apr 10, 2024

Description

Use state serializer content type when saving dapr states.

Issue reference

Please reference the issue this PR will close: #1031

Checklist

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

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@antoniomaria antoniomaria requested review from a team as code owners April 10, 2024 10:45
@antoniomaria antoniomaria force-pushed the use-state-serializer-contenttype-when-saving-actor-state branch from 4be8853 to 0dac76a Compare April 10, 2024 11:50
@antoniomaria antoniomaria changed the title [WIP] Use state serializer content type when saving actor states. Use state serializer content type when saving actor states. Apr 15, 2024
artursouza
artursouza previously approved these changes Apr 16, 2024
@artursouza
Copy link
Contributor

I am want to take this PR, although it is a breaking change.

@artursouza artursouza added this to the v1.12 milestone Apr 16, 2024
@artursouza artursouza self-assigned this Apr 16, 2024
@antoniomaria
Copy link
Contributor Author

Be my guest. Thanks for take it.

@antoniomaria
Copy link
Contributor Author

Did I understand correctly that @artursouza will take this PR to the goal?
Or Can I still do something? like fixing Integration tests, or documentation?

@mukundansundar
Copy link
Contributor

@artursouza Qq if state serializer is used when serializing, shouldn't there be an option during desirialization as well?
Also do you think serializer option could be part of StateOptions? useCustomSerializer or useStateSerializer, and that could be used during both Save and Get State also?

@artursouza
Copy link
Contributor

@artursouza Qq if state serializer is used when serializing, shouldn't there be an option during desirialization as well? Also do you think serializer option could be part of StateOptions? useCustomSerializer or useStateSerializer, and that could be used during both Save and Get State also?

Can you elaborate because this PR is just about setting the content type.

@artursouza
Copy link
Contributor

Did I understand correctly that @artursouza will take this PR to the goal? Or Can I still do something? like fixing Integration tests, or documentation?

Sorry for the ambiguity. By taking this PR, I meant accepting it. But the validation is failing.

@antoniomaria
Copy link
Contributor Author

Did I understand correctly that @artursouza will take this PR to the goal? Or Can I still do something? like fixing Integration tests, or documentation?

Sorry for the ambiguity. By taking this PR, I meant accepting it. But the validation is failing.

Ah sorry, I missed your answer. Perhaps some NPE exception somewhere, I can have a look

@antoniomaria antoniomaria force-pushed the use-state-serializer-contenttype-when-saving-actor-state branch from 9263698 to 7c5f139 Compare May 23, 2024 13:23
@antoniomaria antoniomaria force-pushed the use-state-serializer-contenttype-when-saving-actor-state branch 2 times, most recently from 8193db5 to 74d944a Compare May 23, 2024 14:15
@antoniomaria
Copy link
Contributor Author

Dapr side car doesn't seem to like the content type if the state is null. I checked for it. Please review @artursouza

@antoniomaria
Copy link
Contributor Author

@mukundansundar did you have time to review this MR?

Copy link

codecov bot commented Jun 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.66%. Comparing base (d759c53) to head (0dac76a).
Report is 27 commits behind head on master.

Current head 0dac76a differs from pull request most recent head 6425151

Please upload reports for the commit 6425151 to get more accurate results.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1033      +/-   ##
============================================
+ Coverage     76.91%   77.66%   +0.74%     
+ Complexity     1592     1572      -20     
============================================
  Files           145      144       -1     
  Lines          4843     4768      -75     
  Branches        562      554       -8     
============================================
- Hits           3725     3703      -22     
+ Misses          821      780      -41     
+ Partials        297      285      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@antoniomaria
Copy link
Contributor Author

I don't see the button "Merge PR", is it because the branch is out of date? or because I don't have access to merge a PR ?

@cicoyle
Copy link
Contributor

cicoyle commented Jul 2, 2024

Only maintainers have the ability to merge - cc: @artursouza plz take another pass

@artursouza artursouza merged commit 3dadc0b into dapr:master Jul 3, 2024
8 checks passed
@marcduiker
Copy link
Contributor

@holopin-bot @antoniomaria Thank you!

Copy link

holopin-bot bot commented Aug 15, 2024

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

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dapr client.saveStore(..) doesn't honour stateSerializer contentType
5 participants