Fix 4529: Ignore Subscribe/Get wrong redis configuration type keys. #1693

LaurenceLiZhixin · 2022-04-27T08:49:30Z

Description

Please explain the changes you've made

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.

fix: dapr/dapr#4529

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 / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

Signed-off-by: LaurenceLiZhixin <[email protected]>

LaurenceLiZhixin · 2022-04-27T08:55:43Z

@daixiang0
/cc @beiwei30

configuration/redis/redis.go

berndverst

@LaurenceLiZhixin please add the additional test scenario mentioned by @shubham1172

A test that inserts an incompatible data type and then calls GetConfiguration. The test needs to verify no error was returned and the invalid data type was indeed skipped.

… case of redis configuration Signed-off-by: LaurenceLiZhixin <[email protected]>

LaurenceLiZhixin · 2022-05-05T04:05:09Z

Thanks for your advice, I have updated the code
@berndverst @shubham1172

shubham1172 · 2022-05-05T04:55:21Z

configuration/redis/redis_test.go

+		{
+			name: "test does not throw error for wrong type during get all",
+			prepare: func(client *redis.Client) {
+				client.HSet(context.Background(), "notSupportedType", []string{"key1", "value1", "key2", "value2"})


What does this line do?

This line writes an unsupported value to redis. After that, the test case tries to get all values, and skip returning this unsupported value. @shubham1172

Got it. Since string slice is an unsupported type, should we change it to this to avoid confusion with keys and values?

Suggested change

client.HSet(context.Background(), "notSupportedType", []string{"key1", "value1", "key2", "value2"})

client.HSet(context.Background(), "testKeyUnsupported", []string{"value1", "value2", "value3"})

https://redis.io/docs/manual/data-types/ although it looks like lists are supported by Redis.

codecov · 2022-05-07T06:42:58Z

Codecov Report

Merging #1693 (7009aef) into master (bfd87eb) will increase coverage by 0.03%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #1693      +/-   ##
==========================================
+ Coverage   36.37%   36.40%   +0.03%     
==========================================
  Files         166      166              
  Lines       15488    15520      +32     
==========================================
+ Hits         5633     5650      +17     
- Misses       9228     9239      +11     
- Partials      627      631       +4

Impacted Files	Coverage Δ
bindings/azure/cosmosdb/cosmosdb.go	`18.00% <ø> (ø)`
bindings/cron/cron.go	`87.23% <ø> (ø)`
bindings/http/http.go	`87.87% <ø> (ø)`
bindings/kubernetes/kubernetes.go	`17.14% <ø> (ø)`
bindings/localstorage/localstorage.go	`4.95% <0.00%> (-0.17%)`	⬇️
pubsub/azure/servicebus/message.go	`85.84% <ø> (ø)`
state/azure/cosmosdb/cosmosdb.go	`10.91% <0.00%> (ø)`
state/postgresql/postgresdbaccess.go	`38.60% <0.00%> (ø)`
state/redis/redis.go	`35.00% <ø> (ø)`
pubsub/pulsar/pulsar.go	`28.32% <51.85%> (+4.67%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a51de3b...7009aef. Read the comment docs.

beiwei30

LGTM too.

…apr#1693) * fix: 4529 Signed-off-by: LaurenceLiZhixin <[email protected]> * Fix: add test does not throw error for wrong type during get all test case of redis configuration Signed-off-by: LaurenceLiZhixin <[email protected]> Co-authored-by: Yaron Schneider <[email protected]> Co-authored-by: Ian Luo <[email protected]> Signed-off-by: Eddie <[email protected]>

@msfussell

* Update readme of bindings (#1690) Signed-off-by: pigletfly <[email protected]> Co-authored-by: Looong Dai <[email protected]> Signed-off-by: Eddie <[email protected]> * Fixing includedHeaders problem with spaces (#1610) Signed-off-by: Ben Kotvis <[email protected]> Co-authored-by: Bernd Verst <[email protected]> Co-authored-by: Looong Dai <[email protected]> Signed-off-by: Eddie <[email protected]> * Simplify vault token read (#1560) * Simplify vault token get Signed-off-by: zhangchao <[email protected]> * fix lint Signed-off-by: zhangchao <[email protected]> * update tests Signed-off-by: zhangchao <[email protected]> Co-authored-by: Looong Dai <[email protected]> Co-authored-by: Yaron Schneider <[email protected]> Signed-off-by: Eddie <[email protected]> * GH-1609 : Fix for MongoDB Atlas conn strings Added recommended fix in the issue Signed-off-by: Eddie <[email protected]> * updating the comment based on PR feedback Signed-off-by: Eddie <[email protected]> * Initial Certification test for eventhubs binding [incomplete] (#1670) * certification test for eventhubs binding Signed-off-by: tanvigour <[email protected]> * modified go.mod and go.sum Signed-off-by: tanvigour <[email protected]> * Add connection string testing Signed-off-by: tanvigour <[email protected]> * iothub testing Signed-off-by: tanvigour <[email protected]> * address feedback and run test Signed-off-by: tanvigour <[email protected]> * Install Azure CLI IOT hub extension Signed-off-by: Bernd Verst <[email protected]> * make modtidy-all Signed-off-by: Bernd Verst <[email protected]> * covering all eventhubs test cases Signed-off-by: tanvigour <[email protected]> * dependency changes after go modtidy-all Signed-off-by: tanvigour <[email protected]> Co-authored-by: Bernd Verst <[email protected]> Co-authored-by: Yaron Schneider <[email protected]> Co-authored-by: Looong Dai <[email protected]> Signed-off-by: Eddie <[email protected]> * Use revive instead of golint (#1685) Signed-off-by: pigletfly <[email protected]> Co-authored-by: Yaron Schneider <[email protected]> Signed-off-by: Eddie <[email protected]> * Updated to Go 1.18 (#1697) * Updated to Go 1.18 Signed-off-by: Alessandro (Ale) Segala <[email protected]> * Added go.work file With Go 1.18, this allows gopls (the Go language server used for example in VS Code) to work inside test apps too. See: https://go.dev/doc/tutorial/workspaces Signed-off-by: ItalyPaleAle <[email protected]> Signed-off-by: ItalyPaleAle <[email protected]> * Removed go.work Signed-off-by: ItalyPaleAle <[email protected]> * 💄 Signed-off-by: ItalyPaleAle <[email protected]> Co-authored-by: Bernd Verst <[email protected]> Signed-off-by: Eddie <[email protected]> * Add metadata property to configure Batching in Pulsar (#1707) * Add metadata property to configure BatchingMaxSize&batchingMaxMessages in Pulsar Signed-off-by: saberwang <[email protected]> * sort field Signed-off-by: saberwang <[email protected]> * [pubsub]fix unit test bug Signed-off-by: saberwang <[email protected]> * remove unrelated changes Signed-off-by: saberwang <[email protected]> * Delete hard coded Metadata Signed-off-by: saberwang <[email protected]> * remove .history Signed-off-by: saberwang <[email protected]> * restore .gitignore Signed-off-by: saberwang <[email protected]> * Hard coding default values and adding 'BatchingMaxPublishDelay' metadata Signed-off-by: saberwang <[email protected]> * fix code format Signed-off-by: saberwang <[email protected]> * formatting code Signed-off-by: saberwang <[email protected]> Co-authored-by: Looong Dai <[email protected]> Co-authored-by: Bernd Verst <[email protected]> Signed-off-by: Eddie <[email protected]> * Fix 4529: Ignore Subscribe/Get wrong redis configuration type keys. (#1693) * fix: 4529 Signed-off-by: LaurenceLiZhixin <[email protected]> * Fix: add test does not throw error for wrong type during get all test case of redis configuration Signed-off-by: LaurenceLiZhixin <[email protected]> Co-authored-by: Yaron Schneider <[email protected]> Co-authored-by: Ian Luo <[email protected]> Signed-off-by: Eddie <[email protected]> * Signed-off-by: Eddie Wassef <[email protected]> Fixing leading newline in linter error Signed-off-by: Eddie <[email protected]> * Implment a secret store based on Huawei CSMS (#1710) Signed-off-by: Chen Cong <[email protected]> Co-authored-by: Chen Cong <[email protected]> Signed-off-by: Eddie <[email protected]> * Add yet another missing secret to eventhub binding cert test (#1713) Signed-off-by: Bernd Verst <[email protected]> Signed-off-by: Eddie <[email protected]> * Support custom queueEndpoint in Azure Storage Queues (#1692) * Support custom queueEndpoint in Azure Storage Queues Signed-off-by: Janusz Dziurzynski <[email protected]> * run gofmt Signed-off-by: Janusz Dziurzynski <[email protected]> * Add "Url" to JSON field name for clarity Suggested by @msfussell in dapr/docs#2424 Signed-off-by: Janusz Dziurzynski <[email protected]> Signed-off-by: Eddie <[email protected]> * Refactory kafka binding to reuse the kafka common code extracting from kafka pubsub component (#1696) * refactory kafka pubsub code to extract common kafka code for reuse Signed-off-by: Sky Ao <[email protected]> * fix lint;add unit test for subscribeAdapter Signed-off-by: Sky Ao <[email protected]> * move topics filed from internal kafak struct to pubsub kafka struct, since in input binding the topics will confiured in metadata Signed-off-by: Sky Ao <[email protected]> * reuse internal kafka code for bindings Signed-off-by: Sky Ao <[email protected]> * add redis standalone_test back which is delete by mistaken Signed-off-by: Sky Ao <[email protected]> * small code improvement to trigger test Signed-off-by: Sky Ao <[email protected]> * add license headers Signed-off-by: Sky Ao <[email protected]> * try to set disbaleTls to true to verify the kafka connection fail Signed-off-by: Sky Ao <[email protected]> * don't enable consum retry in kafka binding component;if authenticaion is disabled, need not set TLSDisable at the same time; Signed-off-by: Sky Ao <[email protected]> * fix lint Signed-off-by: Sky Ao <[email protected]> Co-authored-by: Loong Dai <[email protected]> Signed-off-by: Eddie <[email protected]> * Add topic metadata for mqtt input binding and support user defined topic for mqtt output binding (#1674) * feat(bindings/mqtt): add data incoming topic to metadata Signed-off-by: lotuc <[email protected]> * feat(bindings/mqtt): support user defined topic on create action Signed-off-by: lotuc <[email protected]> * chore(bindings/mqtt): add integration test and topic response check test Signed-off-by: lotuc <[email protected]> * fix(bindings/mqtt): ignore misspell linting error for word mosquitto Signed-off-by: lotuc <[email protected]> Co-authored-by: Yaron Schneider <[email protected]> Signed-off-by: Eddie <[email protected]> * Expire -> ExpiryInSeconds (#1721) Signed-off-by: seeflood <[email protected]> Signed-off-by: Eddie <[email protected]> * running gofmt -s -w state/mongodb/mongodb.go Signed-off-by: Eddie <[email protected]> * Update mongodb.go Co-authored-by: Wang Bing <[email protected]> Co-authored-by: Looong Dai <[email protected]> Co-authored-by: Ben Kotvis <[email protected]> Co-authored-by: Bernd Verst <[email protected]> Co-authored-by: Taction <[email protected]> Co-authored-by: Yaron Schneider <[email protected]> Co-authored-by: tanvigour <[email protected]> Co-authored-by: Alessandro (Ale) Segala <[email protected]> Co-authored-by: saber-wang <[email protected]> Co-authored-by: Laurence <[email protected]> Co-authored-by: Ian Luo <[email protected]> Co-authored-by: Chock Chen <[email protected]> Co-authored-by: Chen Cong <[email protected]> Co-authored-by: Janusz Dziurzynski <[email protected]> Co-authored-by: Sky Ao <[email protected]> Co-authored-by: lotuc <[email protected]> Co-authored-by: seeflood <[email protected]>

LaurenceLiZhixin requested review from a team as code owners April 27, 2022 08:49

LaurenceLiZhixin force-pushed the fix-4529 branch from 84a0c02 to 4879e6c Compare April 27, 2022 08:50

fix: 4529

72ec98c

Signed-off-by: LaurenceLiZhixin <[email protected]>

LaurenceLiZhixin force-pushed the fix-4529 branch from 4879e6c to 72ec98c Compare April 27, 2022 08:52

LaurenceLiZhixin changed the title ~~Fix 4529: Ignore Subscribe/Get wrong type keys.~~ Fix 4529: Ignore Subscribe/Get wrong redis configuration type keys. Apr 27, 2022

daixiang0 previously approved these changes Apr 28, 2022

View reviewed changes

shubham1172 reviewed Apr 28, 2022

View reviewed changes

configuration/redis/redis.go Show resolved Hide resolved

berndverst requested changes May 4, 2022

View reviewed changes

yaron2 and others added 2 commits May 4, 2022 16:17

Merge branch 'master' into fix-4529

83c22b6

Fix: add test does not throw error for wrong type during get all test…

518a05e

… case of redis configuration Signed-off-by: LaurenceLiZhixin <[email protected]>

LaurenceLiZhixin dismissed daixiang0’s stale review via 518a05e May 5, 2022 04:03

shubham1172 reviewed May 5, 2022

View reviewed changes

yaron2 and others added 2 commits May 6, 2022 12:17

Merge branch 'master' into fix-4529

c196e36

Merge branch 'master' into fix-4529

7009aef

daixiang0 approved these changes May 7, 2022

View reviewed changes

beiwei30 approved these changes May 7, 2022

View reviewed changes

yaron2 merged commit e0ea27f into dapr:master May 7, 2022

berndverst added this to the v1.8 milestone May 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 4529: Ignore Subscribe/Get wrong redis configuration type keys. #1693

Fix 4529: Ignore Subscribe/Get wrong redis configuration type keys. #1693

LaurenceLiZhixin commented Apr 27, 2022

LaurenceLiZhixin commented Apr 27, 2022 •

edited

Loading

berndverst left a comment

LaurenceLiZhixin commented May 5, 2022

shubham1172 May 5, 2022

LaurenceLiZhixin May 5, 2022 •

edited

Loading

shubham1172 May 5, 2022

shubham1172 May 5, 2022

codecov bot commented May 7, 2022

beiwei30 left a comment

	client.HSet(context.Background(), "notSupportedType", []string{"key1", "value1", "key2", "value2"})
	client.HSet(context.Background(), "testKeyUnsupported", []string{"value1", "value2", "value3"})

Fix 4529: Ignore Subscribe/Get wrong redis configuration type keys. #1693

Fix 4529: Ignore Subscribe/Get wrong redis configuration type keys. #1693

Conversation

LaurenceLiZhixin commented Apr 27, 2022

Description

Issue reference

Checklist

LaurenceLiZhixin commented Apr 27, 2022 • edited Loading

berndverst left a comment

Choose a reason for hiding this comment

LaurenceLiZhixin commented May 5, 2022

shubham1172 May 5, 2022

Choose a reason for hiding this comment

LaurenceLiZhixin May 5, 2022 • edited Loading

Choose a reason for hiding this comment

shubham1172 May 5, 2022

Choose a reason for hiding this comment

shubham1172 May 5, 2022

Choose a reason for hiding this comment

codecov bot commented May 7, 2022

Codecov Report

beiwei30 left a comment

Choose a reason for hiding this comment

LaurenceLiZhixin commented Apr 27, 2022 •

edited

Loading

LaurenceLiZhixin May 5, 2022 •

edited

Loading