State stores: conformace test to check special characters are not mangled #2910

JoshVanL · 2023-06-13T14:49:33Z

Conformance test for all state stores to ensure that all state stores are able to store and return keys who's values are string which contain special characters.

Adds new state/utils.JSONStringify function for converting a []byte into a jsonified []byte string. Update state stores which were incorrectly encoding special characters in string state values on set.

Question: Why are we doing anything to Values when Setting to state stores? Why do we not Set them as-is?

strings, written as is Signed-off-by: joshvanl <me@joshvanl.dev>

Signed-off-by: joshvanl <me@joshvanl.dev>

`JSONStringify` Signed-off-by: joshvanl <me@joshvanl.dev>

Signed-off-by: joshvanl <me@joshvanl.dev>

ItalyPaleAle

What I am concerned with: is this going to cause backwards-incompatibility?

What about reading values created with Dapr "vCurrent" using "vNext"?
What about reading values created with Dapr "vNext" into "vCurrent"?

state/utils/utils.go

ItalyPaleAle · 2023-06-14T19:39:35Z

state/utils/utils.go

+	}
+}
+
+// Deprecated: use JSONStringify instead.


Should this be deleted then?

It is still references in dapr/dapr. We can remove once we have merged this, updated go mod and migrated the caller in dapr/dapr

ItalyPaleAle · 2023-06-14T19:52:57Z

state/in-memory/in_memory.go

 	for _, o := range request.Operations {
 		switch req := o.(type) {
 		case *innerSetRequest:
-			store.doSet(ctx, req.req.Key, req.data, req.ttl)
+			err = errors.Join(err, store.doSet(ctx, req.req.Key, req.req.Value, req.ttl))


Why the errors.Join here?

We are inside a range, and want to catch all doSet errors.

However this is not really how errors.Join should be used, as the Go team told me too: golang/go#60209
Should collect the errors in a slice and then call errors.Join once

Thanks 👍

artursouza · 2023-06-14T20:51:10Z

What I am concerned with: is this going to cause backwards-incompatibility?

What about reading values created with Dapr "vCurrent" using "vNext"?

What about reading values created with Dapr "vNext" into "vCurrent"?

This is a common debate of fixing a bug but having customers relying on the bug. We do have a way to workaround that, it is to bump the component version and keep the old version untouched. It would require to copy and paste the code and deprecate v1 of each component that has a breaking change.

ItalyPaleAle · 2023-06-14T21:24:13Z

Even if we made a "v2", something I would consider a last-resort as it would double the maintenance burden on maintainers of this repo, we also need a solution for data that was already written. Otherwise, users (who, on average, never read docs, let alone changelogs ) will just switch "v1" to "v2" and then complain Dapr broke their data 🙃

@JoshVanL what are the "broken" components in 1.11.0?

ItalyPaleAle · 2023-06-14T22:28:44Z

Some additional interesting findings. The interface in components accepts data as any, so I was wondering what kind of data actually gets stored from Dapr.

When data comes from the gRPC API, it's always received by the component as a []byte. Some components base64-encode that before storing it.
When it comes from HTTP, it's whatever it is in the JSON that it's received. For example, if you pass a number, it's a float64 (because JavaScript...), strings are strings, bools are bools, and objects/arrays are received by the component as-is so are serialized as JSON.

This seems like it can cause a lot of inconsistencies also depending on how the data is stored and retrieved.

Perhaps the problem isn't really having a v2 of components, but a v2 of the state store APIs that removes all these inconsistencies...

state/utils/utils.go

Signed-off-by: joshvanl <me@joshvanl.dev>

JoshVanL · 2023-06-15T11:16:58Z

@ItalyPaleAle I believe these would have been the broken ones https://github.com/dapr/components-contrib/actions/runs/5257496485

Signed-off-by: joshvanl <me@joshvanl.dev>

…/issues/60209b Signed-off-by: joshvanl <me@joshvanl.dev>

ItalyPaleAle · 2023-06-15T14:22:06Z

List here:

Aside from in-memory which doesn't matter, I am very surprised to see MariaDB in the list but not MySQL, since they should be the same...

github-actions · 2023-07-15T23:16:57Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2023-07-22T23:24:58Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2023-08-21T23:43:36Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2023-08-28T23:53:44Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Add conformance test to ensure state stores return special characters in

f590f50

strings, written as is Signed-off-by: joshvanl <me@joshvanl.dev>

JoshVanL requested review from a team as code owners June 13, 2023 14:49

Expected returned value to have escaped double quotes

a7ccf78

Signed-off-by: joshvanl <me@joshvanl.dev>

JoshVanL force-pushed the test-conformance-state-specical-characters branch from 2848d3a to a7ccf78 Compare June 13, 2023 15:33

Merge branch 'master' into test-conformance-state-specical-characters

b22e320

JoshVanL mentioned this pull request Jun 14, 2023

1.10.8 Patch Release Checklist dapr/dapr#6511

Closed

17 tasks

JoshVanL and others added 9 commits June 14, 2023 14:07

Replace state/utils json marshaller, with JSONStringify

d53fbaf

Signed-off-by: joshvanl <me@joshvanl.dev>

Linting

673d226

Signed-off-by: joshvanl <me@joshvanl.dev>

Fix postgresql db set unit tests

ad0cbdc

Signed-off-by: joshvanl <me@joshvanl.dev>

Update state/{mysql,rethinkdb,sqlite} to use utils.JSONStringify

837e0fb

Signed-off-by: joshvanl <me@joshvanl.dev>

redis: use json.Marshal on JSON header content values

cc96622

Signed-off-by: joshvanl <me@joshvanl.dev>

tests/conformance/state: add bulk read/write test for special characters

178be0c

Signed-off-by: joshvanl <me@joshvanl.dev>

Merge branch 'master' into test-conformance-state-specical-characters

c381a85

Add deprecation warning to utils.Marshal. Consumers should use

1845629

`JSONStringify` Signed-off-by: joshvanl <me@joshvanl.dev>

Linting

bfbc394

Signed-off-by: joshvanl <me@joshvanl.dev>

ItalyPaleAle reviewed Jun 14, 2023

View reviewed changes

ItalyPaleAle reviewed Jun 15, 2023

View reviewed changes

state/utils/utils.go Outdated Show resolved Hide resolved

JoshVanL added 2 commits June 15, 2023 11:51

Use strconv over fmt.Sprintf for slightly better perf

b6c6a65

Signed-off-by: joshvanl <me@joshvanl.dev>

Use json.Marshal with HTLM escape disabled for string type

b29b83c

Signed-off-by: joshvanl <me@joshvanl.dev>

JoshVanL added 2 commits June 15, 2023 13:23

Add back string switch case to ensure previous behavior is maintained

ee549c2

Signed-off-by: joshvanl <me@joshvanl.dev>

Call errors.Join once, as according to https://github.com/golang/go…

f62d88d

…/issues/60209b Signed-off-by: joshvanl <me@joshvanl.dev>

ItalyPaleAle added the do-not-merge PR is not ready for merging label Jun 15, 2023

github-actions bot added the stale label Jul 15, 2023

github-actions bot closed this Jul 22, 2023

yaron2 reopened this Jul 22, 2023

github-actions bot removed the stale label Jul 22, 2023

github-actions bot added the stale label Aug 21, 2023

github-actions bot closed this Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

State stores: conformace test to check special characters are not mangled #2910

State stores: conformace test to check special characters are not mangled #2910

JoshVanL commented Jun 13, 2023 •

edited

Loading

ItalyPaleAle left a comment

ItalyPaleAle Jun 14, 2023

JoshVanL Jun 15, 2023

ItalyPaleAle Jun 14, 2023

JoshVanL Jun 15, 2023

ItalyPaleAle Jun 15, 2023

JoshVanL Jun 15, 2023

artursouza commented Jun 14, 2023

ItalyPaleAle commented Jun 14, 2023

ItalyPaleAle commented Jun 14, 2023

JoshVanL commented Jun 15, 2023

ItalyPaleAle commented Jun 15, 2023

github-actions bot commented Jul 15, 2023

github-actions bot commented Jul 22, 2023

github-actions bot commented Aug 21, 2023

github-actions bot commented Aug 28, 2023

State stores: conformace test to check special characters are not mangled #2910

State stores: conformace test to check special characters are not mangled #2910

Conversation

JoshVanL commented Jun 13, 2023 • edited Loading

ItalyPaleAle left a comment

Choose a reason for hiding this comment

ItalyPaleAle Jun 14, 2023

Choose a reason for hiding this comment

JoshVanL Jun 15, 2023

Choose a reason for hiding this comment

ItalyPaleAle Jun 14, 2023

Choose a reason for hiding this comment

JoshVanL Jun 15, 2023

Choose a reason for hiding this comment

ItalyPaleAle Jun 15, 2023

Choose a reason for hiding this comment

JoshVanL Jun 15, 2023

Choose a reason for hiding this comment

artursouza commented Jun 14, 2023

ItalyPaleAle commented Jun 14, 2023

ItalyPaleAle commented Jun 14, 2023

JoshVanL commented Jun 15, 2023

ItalyPaleAle commented Jun 15, 2023

github-actions bot commented Jul 15, 2023

github-actions bot commented Jul 22, 2023

github-actions bot commented Aug 21, 2023

github-actions bot commented Aug 28, 2023

JoshVanL commented Jun 13, 2023 •

edited

Loading