Added new output binding for Cosmos Graph DB - Gremlin #885

hokandil · 2021-05-25T19:25:03Z

Description

I added a new output binding to support graph DB in Azure Cosmos via the Gremlin API.
unfortunately there no issue reference for this PR , although I found it useful to share this with you.

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]

ghost · 2021-05-25T19:25:15Z

All CLA requirements met.

yaron2 · 2021-06-04T02:55:28Z

@beiwei30 can you please review?

hokandil · 2021-06-04T16:13:07Z

@beiwei30 I fixed the issue causing the golangci-lint to fail.

halspang

Couple sections of commented code, let's remove it if we don't need it or put it back if we do.

halspang · 2021-06-04T21:09:26Z

bindings/azure/cosmosgraphdb/cosmosgraphdb.go

+	// NumMaxActiveConnections string `json:"NumMaxActiveConnections"`
+	// ConnectionIdleTimeout   string `json:"ConnectionIdleTimeout"`


Do we not want to use these and give defaults if they aren't provided?

yes, @halspang you are right , some comments need to be removed along with these values (NumMaxActiveConnections, ConnectionIdleTimeout) , I felt like it is over complicating the component YAML file , so I decided to remove it . any way the gremcos package assign there values to a default values which is more than enough for now.

All unwanted comment removed.

bindings/azure/cosmosgraphdb/cosmosgraphdb.go

yaron2 · 2021-06-27T15:43:26Z

@hokandil please resolve the conflicts and we'll assign a reviewer.

hokandil · 2021-06-27T16:05:34Z

@yaron2 Done, conflicts resolved , thank you

yaron2 · 2021-06-27T16:06:11Z

@halspang can you please review this?

halspang

Changes look good to me, just some small last things.

@yaron2 is passing the query though metadata the standard? If so, what's the reasoning behind it? I'm fine with it if that's the case, just curious.

bindings/azure/cosmosgraphdb/cosmosgraphdb.go

bindings/azure/cosmosgraphdb/cosmosgraphdb_test.go

bindings/azure/cosmosgraphdb/cosmosgraphdb.go

yaron2 · 2021-07-08T17:34:43Z

@hokandil I left two comments. kindly fix those, and resolve the linter issue:

Check failure on line 53 in bindings/azure/cosmosgraphdb/cosmosgraphdb.go

GitHub Actions
/ Build linux_amd64 binaries
bindings/azure/cosmosgraphdb/cosmosgraphdb.go#L53
File is not `gofmt`-ed with `-s` (gofmt)

Then we can merge :)

yaron2 · 2021-07-12T18:39:05Z

@hokandil ping

hokandil · 2021-07-12T18:42:35Z

@yaron2 I am on testing phase, I will commit shortly , maybe tomorrow .

hokandil · 2021-07-13T18:09:52Z

@yaron2 Done

pkedy

Just pointing out a few potential improvements.

pkedy · 2021-07-13T19:55:36Z

bindings/azure/cosmosgraphdb/cosmosgraphdb.go

+}
+
+func (c *CosmosGraphDB) parseMetadata(metadata bindings.Metadata) (*cosmosGraphDBCredentials, error) {
+	b, err := json.Marshal(metadata.Properties)


Nit: You can use the configuration decoding pattern we have instead of Marshaling/Unmarshaling JSON.

Example:
https://github.com/dapr/components-contrib/blob/master/bindings/alicloud/nacos/nacos.go#L59-L66
https://github.com/dapr/components-contrib/blob/master/bindings/alicloud/nacos/settings.go

pkedy · 2021-07-13T20:00:32Z

bindings/azure/cosmosgraphdb/cosmosgraphdb.go

+		return nil, errors.New("CosmosGraphDB Error: Cannot convert request data")
+	}
+
+	gq := fmt.Sprintf("%s", jsonPoint[commandGremlinKey])


Nit: It might more sense to unmarshal into an inline struct instead of map[string]interface{} since you know the field you expect.

type gremlinData struct { Gremlin string `json:"gremlin"` // Note sure if this is always a string, use `interface{}` otherwise. } var jsonPoint gremlinData

Also, using %v might make more since for an interface{} going in. But again, if the value is always going to be a string then the fmt.Sprintf is unnecessary.

gq := fmt.Sprintf("%v", jsonPoint[commandGremlinKey])

codecov · 2021-07-14T15:48:57Z

Codecov Report

Merging #885 (83e9716) into master (01147e5) will increase coverage by 3.61%.
The diff coverage is 37.41%.

❗ Current head 83e9716 differs from pull request most recent head b366e1a. Consider uploading reports for the commit b366e1a to get more accurate results

@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
+ Coverage   30.59%   34.21%   +3.61%     
==========================================
  Files          85      133      +48     
  Lines        7148    10595    +3447     
==========================================
+ Hits         2187     3625    +1438     
- Misses       4724     6591    +1867     
- Partials      237      379     +142

Impacted Files	Coverage Δ
bindings/alicloud/oss/oss.go	`11.11% <ø> (ø)`
bindings/alicloud/rocketmq/rocketmq.go	`0.00% <0.00%> (ø)`
bindings/apns/apns.go	`88.00% <ø> (ø)`
bindings/aws/dynamodb/dynamodb.go	`10.52% <ø> (ø)`
bindings/aws/kinesis/kinesis.go	`2.61% <ø> (ø)`
bindings/aws/s3/s3.go	`10.81% <ø> (ø)`
bindings/aws/sns/sns.go	`10.52% <ø> (ø)`
bindings/aws/sqs/sqs.go	`6.55% <0.00%> (ø)`
bindings/azure/blobstorage/blobstorage.go	`7.54% <0.00%> (-0.30%)`	⬇️
bindings/azure/cosmosdb/cosmosdb.go	`25.71% <ø> (ø)`
... and 173 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 3730993...b366e1a. Read the comment docs.

* master: (23 commits) Adds support for TTL in Redis State Store (dapr#990) Adds support for TTL in Memcached State Store (PR Fixup) (dapr#1011) Adds support for TTL in Cassandra State Store (dapr#996) Return blob metadata and add list operation (dapr#999) Added new output binding for Cosmos Graph DB - Gremlin (dapr#885) Adds support for TTL in CosmosDB State Store (dapr#991) Delete unused code and update test (dapr#924) Return SystemProperties on events from EventHubs bindings and pubsub (dapr#1009) Implement E2E tests for zeebe (dapr#973) rebase (dapr#982) Update CODEOWNERS (dapr#987) ci: add test skip case (dapr#922) fix: Mysql should support more data types. dapr#923 (dapr#926) fix: Dapr runtime panic when handle Pub/Sub (dapr#3281) (dapr#967) switch to golang-jwt (dapr#993) unmarshal getBlobRetryCount as int (dapr#919) secretstores: support more format of azure key vault (dapr#944) Optimize vault secret component error output (dapr#909) Convert AZURE_KEYVAULT in conformance.yml a GitHub secret (dapr#1002) Use AzureKeyVaultName param for Azure Keyvault conformance test (dapr#975) ... # Conflicts: # secretstores/hashicorp/vault/vault.go

Added new output binding for Cosmos Graph DB - Gremlin

b822ee1

Merge branch 'master' into master

5200a95

yaron2 requested a review from beiwei30 June 4, 2021 02:55

hokandil added 3 commits June 4, 2021 15:15

applying gofumpt on cosmosdbgraphdb

a400ae1

Merge branch 'master' of https://github.com/hokandil/components-contrib

3cda4a6

removed unnecessary conversion from cosmosgraphdb

a012243

halspang reviewed Jun 4, 2021

View reviewed changes

hokandil added 3 commits June 5, 2021 12:54

removed unwanted comments from cosmosgraphdb

8b9f1f2

Merge branch 'master' into master

265e796

Merge branch 'master' into master

9bd6b3b

hokandil requested a review from halspang June 9, 2021 09:14

hokandil added 5 commits June 27, 2021 19:56

resolving gomod confilicts

3fa4e13

Merge branch 'master' of https://github.com/hokandil/components-contrib

e34578d

Merge branch 'master' into master

ef97191

resolving go mod conflict

c986c74

Merge branch 'master' of https://github.com/hokandil/components-contrib

3987e4d

halspang reviewed Jun 28, 2021

View reviewed changes

bindings/azure/cosmosgraphdb/cosmosgraphdb.go Outdated Show resolved Hide resolved

bindings/azure/cosmosgraphdb/cosmosgraphdb.go Outdated Show resolved Hide resolved

bindings/azure/cosmosgraphdb/cosmosgraphdb_test.go Show resolved Hide resolved

hokandil requested a review from halspang June 29, 2021 16:44

hokandil and others added 4 commits July 6, 2021 22:44

resolve conflicting error messages

6043763

Merge branch 'master' into master

55a4ece

Merge branch 'master' into master

3d250e8

Merge branch 'master' into master

0216684

yaron2 requested changes Jul 8, 2021

View reviewed changes

bindings/azure/cosmosgraphdb/cosmosgraphdb.go Outdated Show resolved Hide resolved

bindings/azure/cosmosgraphdb/cosmosgraphdb.go Outdated Show resolved Hide resolved

hokandil added 2 commits July 13, 2021 21:46

change Gremlin query to req.data.

606a964

Merge branch 'master' of https://github.com/hokandil/components-contrib

305eb25

hokandil requested review from a team as code owners July 13, 2021 17:46

Merge branch 'master' into master

83e9716

pkedy reviewed Jul 13, 2021

View reviewed changes

hokandil requested review from yaron2 and removed request for a team July 14, 2021 09:55

yaron2 mentioned this pull request Jul 14, 2021

GraphQL Binding Doc dapr/docs#1630

Merged

6 tasks

Merge branch 'master' into master

b366e1a

yaron2 approved these changes Jul 14, 2021

View reviewed changes

yaron2 merged commit 13f96c4 into dapr:master Jul 14, 2021

greenie-msft mentioned this pull request Oct 28, 2021

Add content for Azure CosmosDBGremlinAPI dapr#1849 V1.5 dapr/docs#1883

Merged

6 tasks

artursouza modified the milestone: v1.5 Nov 10, 2021

		// NumMaxActiveConnections string `json:"NumMaxActiveConnections"`
		// ConnectionIdleTimeout string `json:"ConnectionIdleTimeout"`

Added new output binding for Cosmos Graph DB - Gremlin #885

Added new output binding for Cosmos Graph DB - Gremlin #885

Uh oh!

Conversation

hokandil commented May 25, 2021

Description

Uh oh!

ghost commented May 25, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaron2 commented Jun 4, 2021

Uh oh!

hokandil commented Jun 4, 2021

Uh oh!

halspang left a comment

Choose a reason for hiding this comment

Uh oh!

halspang Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

hokandil Jun 5, 2021

Choose a reason for hiding this comment

Uh oh!

hokandil Jun 5, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yaron2 commented Jun 27, 2021

Uh oh!

hokandil commented Jun 27, 2021

Uh oh!

yaron2 commented Jun 27, 2021

Uh oh!

halspang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yaron2 commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaron2 commented Jul 12, 2021

Uh oh!

hokandil commented Jul 12, 2021

Uh oh!

hokandil commented Jul 13, 2021

Uh oh!

pkedy left a comment

Choose a reason for hiding this comment

Uh oh!

pkedy Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

pkedy Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 14, 2021

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ghost commented May 25, 2021 •

edited by ghost

Loading

yaron2 commented Jul 8, 2021 •

edited

Loading