Return blob metadata and add list operation #999

akkie · 2021-07-06T14:44:03Z

Description

This PR adds the possibility to return the user defined blob metadata and it returns the option to list blobs from a container.

I've started first to implement one PR for each feature, but during development of the second feature it turned out that code is shared between the both features. It therefore made sense to implement both features into one PR. I hope that is ok?

Fixes: #683
Fixes: #682

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: Add doc for blob storage metadata retrieval and blob listing docs#1623

akkie · 2021-07-08T15:44:22Z

@pkedy @artursouza There is some inconsistency in the blob storage binding:

The form of the metadata keys is inconsistent. For the blobName the camel case form is used and the others use pascal case. When I look at the other bindings, camel case is mainly used. Is there a common rule?
Is there a rule when metadata and when data should be used? For the create option it makes sense to send the blobName in the metadata, because the content is used for the data. But for all other options, the data could be used instead of the metadata. I have the list option also implemented with metadata, but I think it makes more sense to send the data as JSON in the data field.

What do you think about that? Maybe it makes sense to create a new version of the binding and clean the things up?

.devcontainer/devcontainer.json

bindings/azure/blobstorage/blobstorage.go

artursouza · 2021-07-09T03:44:03Z

@pkedy @artursouza There is some inconsistency in the blob storage binding:

The form of the metadata keys is inconsistent. For the blobName the camel case form is used and the others use pascal case. When I look at the other bindings, camel case is mainly used. Is there a common rule?

camel case should be used. As of now, this component is in Alpha and not GA, which means it can have breaking change. I would add support to camel case for the ones using pascal case to give consistency but keep backwards compatibility (alpha does not require backwards compatibility but it is nice to keep IMO). Then, add a TODO to remove the pascal case support when the component moves to GA. Adding a new version would be overkill now IMO.

Is there a rule when metadata and when data should be used? For the create option it makes sense to send the blobName in the metadata, because the content is used for the data. But for all other options, the data could be used instead of the metadata. I have the list option also implemented with metadata, but I think it makes more sense to send the data as JSON in the data field.

I like blobName in the metadata for all method for consistency.

What do you think about that? Maybe it makes sense to create a new version of the binding and clean the things up?

Because this is not in GA yet, it can have breaking changes. Like I said, I would try to bring consistency without breaking changes as much as possible and add a TODO to remove the "old way" when moving it to GA.

akkie · 2021-07-09T15:32:15Z

I followed your suggestion to make it backward compatible and use metadata where the blob name is involved. This means that the create, get and delete options will use metadata to submit data and the list option will use the data property. I need to update the documentation, but the PR can be reviewed.

artursouza

LGTM. Just a small readability improvement.

artursouza · 2021-07-12T18:30:08Z

bindings/azure/blobstorage/blobstorage.go

-	contentDisposition       = "ContentDisposition"
-	cacheControl             = "CacheControl"
-	deleteSnapshotOptions    = "DeleteSnapshotOptions"
+	marker                   = "marker"


a constant that just repeats the content is not so useful. Please, add more context in the name for improved readability. This feedback is for the other constants too. I know it was already like that but this is an opportunity for us to raise the bar here.

Do you have an example?

I have added a prefix metadataKey for each constant. Hope this is OK?

I think it would be great to add comment where define them and those meanings.

@daixiang0 Do you mean for each or only for the group?

@daixiang0 Just to be sure. You suggest to add a comment for each constant where the meaning is described?

const ( // Defines the name of the blob metadataKeyBlobName = "blobName" // Defines a string value that identifies the portion of the list to be returned with the next list operation. // The operation returns a marker value within the response body if the list returned was not complete. The marker // value may then be used in a subsequent call to request the next set of list items. metadataKeyMarker = "marker" ... )

Is there any doc for those? If so, add link here.

Defines the name of the blob

Comments like this make no sense.

I have also updated the doc: https://github.com/dapr/docs/pull/1623/files

Please don't get me wrong. But could you please be more precise with the changes you suggest? I think otherwise we are going around in circles here. Maybe you can add an example?

Is there a doc for "blobName", "marker" and so on? Add link here. If not, add comment for each if necessary.

Is this OK now?

yaron2 · 2021-07-14T16:03:12Z

@akkie please run go mod tidy and push the changes. we can then merge.

codecov · 2021-07-14T16:11:23Z

Codecov Report

Merging #999 (8012c72) into master (13f96c4) will decrease coverage by 0.12%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master     #999      +/-   ##
==========================================
- Coverage   34.16%   34.03%   -0.13%     
==========================================
  Files         133      133              
  Lines       10640    10750     +110     
==========================================
+ Hits         3635     3659      +24     
- Misses       6627     6710      +83     
- Partials      378      381       +3

Impacted Files	Coverage Δ
bindings/azure/blobstorage/blobstorage.go	`14.81% <16.66%> (+7.26%)`	⬆️

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 13f96c4...8012c72. 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

akkie changed the title ~~Return blob metadata if configured~~ Return blob metadata and add list operation Jul 7, 2021

akkie marked this pull request as draft July 7, 2021 13:36

artursouza suggested changes Jul 9, 2021

View reviewed changes

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved

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

akkie added 9 commits July 9, 2021 14:21

Return blob metadata if configured

790b6da

Enhance error msg

dc3f066

Add list functionality

a3e92a8

Fix linting issues

40f66ad

Finalize list implementation

c81a00d

Fix linting errors

365751c

Add backward compatibility and align code to be more consistent

77aafb6

Add tests

2e93912

Fix linting errors

6c5074a

akkie marked this pull request as ready for review July 9, 2021 15:32

akkie requested review from a team as code owners July 9, 2021 15:32

akkie added 3 commits July 12, 2021 12:24

Rename properties

2852caa

Fix max result logic

7c67bed

Fix linting error

8e059a2

artursouza suggested changes Jul 12, 2021

View reviewed changes

akkie and others added 4 commits July 13, 2021 09:32

Fix PR suggestions

2bdd8a2

Add comments for constants

1527148

Merge branch 'master' into get-blob-metadata

8068920

Merge branch 'master' into get-blob-metadata

23afe44

Run go mod tidy

8012c72

yaron2 approved these changes Jul 14, 2021

View reviewed changes

yaron2 merged commit 80f0110 into dapr:master Jul 14, 2021

yaron2 mentioned this pull request Jul 14, 2021

Add doc for blob storage metadata retrieval and blob listing dapr/docs#1623

Merged

Return blob metadata and add list operation #999

Return blob metadata and add list operation #999

Uh oh!

Conversation

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

Uh oh!

Description

Checklist

Uh oh!

akkie commented Jul 8, 2021

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

artursouza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaron2 commented Jul 14, 2021

Uh oh!

codecov bot commented Jul 14, 2021

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

akkie commented Jul 6, 2021 •

edited

Loading

artursouza commented Jul 9, 2021 •

edited

Loading

akkie commented Jul 9, 2021 •

edited

Loading

daixiang0 Jul 13, 2021 •

edited

Loading

akkie Jul 13, 2021 •

edited

Loading