Skip to content
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

Add an indxed column and simple query API for distinct sub-paths #1236

Merged
merged 3 commits into from
Mar 27, 2023

Conversation

peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Mar 26, 2023

Fixes #1227

  • Adds a blob_path column / blob.path JSON field for the path
  • Standardizes on / prefixing on these paths, without requiring the name to include a / prefix
  • Provides a simple API for querying the direct decendents of a path (no pagination or filtering, simple string return)

This only adds the column for newly created data objects, and it will be empty for all existing data

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@codecov-commenter
Copy link

codecov-commenter commented Mar 26, 2023

Codecov Report

Merging #1236 (9ba2c48) into main (7660d7d) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1236      +/-   ##
==========================================
+ Coverage   99.91%   99.97%   +0.05%     
==========================================
  Files         306      307       +1     
  Lines       20135    20189      +54     
==========================================
+ Hits        20117    20183      +66     
+ Misses         15        3      -12     
  Partials        3        3              
Impacted Files Coverage Δ
internal/apiserver/routes.go 100.00% <ø> (ø)
internal/orchestrator/orchestrator.go 100.00% <ø> (ø)
internal/apiserver/route_get_data_subpaths.go 100.00% <100.00%> (ø)
internal/database/sqlcommon/data_sql.go 100.00% <100.00%> (ø)
internal/orchestrator/data_query.go 100.00% <100.00%> (ø)
pkg/core/data.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@nguyer
Copy link
Contributor

nguyer commented Mar 27, 2023

One question on this... when I upload a blob today, I don't have the option to give it a name. It looks like we calculate the path based off the name, but I didn't see anything that adds the ability to set the name in this PR when doing a multi-part file upload. I could very well just be overlooking something though?

@peterbroadhurst
Copy link
Contributor Author

Let me find the semantics for setting that name, which as you say are unchanged in this PR...

@peterbroadhurst
Copy link
Contributor Author

Here we go: #381 (pointing to FIR 3)

@nguyer
Copy link
Contributor

nguyer commented Mar 27, 2023

Okay. I re-read over that FIR (thanks for digging that up). Maybe it's just that the Swagger doesn't explain that this is possible to set? If I want to specify the name, do I need to send JSON and the file? I'm not sure how one would do that?

There are a few fields exposed on the Swagger UI for things that can be set. Do we need to add value.name to that list as well?

@peterbroadhurst
Copy link
Contributor Author

peterbroadhurst commented Mar 27, 2023

The Swagger contains the details to the extent it can I think (without us building docs for a bigger Markdown/HTML multi-line documentation on complex APIs beyond Swagger's capabilities to document at the field level). I'm happy to work on something related to instructions for using the metadata JSON field or autometa fields in a PR if you think there's a gap in the docs / instructions there. I would like to decouple it from this PR though if that's ok.

@peterbroadhurst
Copy link
Contributor Author

peterbroadhurst commented Mar 27, 2023

These are the correct fields I believe - although I do see that these fields don't have descriptions - not sure if that's a Swagger UI bug, or a gap in the FF codebase:
image

@peterbroadhurst
Copy link
Contributor Author

I raised #1241 to decouple the investigation there

@nguyer
Copy link
Contributor

nguyer commented Mar 27, 2023

Yep, I'm happy for those enhancements to be a separate PR. I don't think it's urgent, so whenever we get to it. It just seems non-obvious how a user would use the new functionality here though, so as long as that gap gets closed at some point, I'm happy to merge this.

@peterbroadhurst peterbroadhurst merged commit 681908a into hyperledger:main Mar 27, 2023
@peterbroadhurst peterbroadhurst deleted the folderpaths branch March 27, 2023 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to list sub-paths for data
3 participants