decentralized-identity / confidential-storage

Confidential Storage Specification and Implementation
https://identity.foundation/confidential-storage/
Apache License 2.0
78 stars 23 forks source link

Add option to get full documents from queries #151

Open DRK3 opened 3 years ago

DRK3 commented 3 years ago

closes #137

DRK3 commented 3 years ago

Should I be adding myself to the list of editors at the top of the page?

OR13 commented 3 years ago

Should I be adding myself to the list of editors at the top of the page?

only if you intend to regularly attend WG calls, create and merge PRs based on WG consensus and generally be active, and even then the chairs / editors will have to approve the decision to add another editor, and I don't know DIFs process for this.

If there is an authors section, I think that may be safer... ping @tplooker @msporny @csuwildcat @dmitrizagidulin

dlongley commented 3 years ago

We should discuss what problem this is trying to solve and whether or not simpler mechanisms (HTTP keep-alive, HTTP/2) solve it more elegantly. We may want to discuss some kind of cursor/query-continuation mechanism as well, if that's desired.

I'm not necessarily opposed to additional interfaces, but I'd rather us not put cart before horse. The fewer primitives we require the better, provided the use cases are appropriately covered (including high performance use cases).

OR13 commented 3 years ago

@dlongley you are not requesting changes on this PR, but appear to be advocating for us to do some other work, its hard for me to know what to do with a PR like this without more critical feedback :)

DRK3 commented 3 years ago

@OR13 @dlongley Thanks for reviewing my PR!

only if you intend to regularly attend WG calls, create and merge PRs based on WG consensus and generally be active, and even then the chairs / editors will have to approve the decision to add another editor, and I don't know DIFs process for this.

If there is an authors section, I think that may be safer...

It sound like an "Authors" section would be a better fit for me, then.

I suppose the only concern here is that you can't use returnFullDocuments if your result set will be large than some reasonable threshold.... not sure how this would or would not interact with streams. I generally like the idea of a single query response interface for getting documents though.... most developers will expect something like this.

We definitely need some sort of pagination syntax... but note that the existing query endpoint already has this potential problem, as it could potentially return a huge list of document locations. I would like to work on some sort of pagination feature in the near future (unless someone else is already looking into this!)

We should discuss what problem this is trying to solve and whether or not simpler mechanisms (HTTP keep-alive, HTTP/2) solve it more elegantly. We may want to discuss some kind of cursor/query-continuation mechanism as well, if that's desired.

I'm not necessarily opposed to additional interfaces, but I'd rather us not put cart before horse. The fewer primitives we require the better, provided the use cases are appropriately covered (including high performance use cases).

Basically the problem is one of performance. By returning only document locations, clients are forced to do separate requests for each document, which can be painfully slow. This is a problem for us in Aries-Framework-Go, where we have an EDV storage provider that conforms to a general storage interface, and certain types of operations become very expensive. By returning full documents, we prevent having to make extra calls. It also ends up being useful for "bulk get" types of operations.

The exact way I implemented this could also be discussed... perhaps it should be a separate endpoint? The way I have it in this PR just matches the way I did it in our TrustBloc implementation, but I'm not sure if this is really the best way.

dlongley commented 3 years ago

@OR13,

...its hard for me to know what to do with a PR like this without more critical feedback

We should discuss this in the group before merging.

@DRK3,

We definitely need some sort of pagination syntax... but note that the existing query endpoint already has this potential problem, as it could potentially return a huge list of document locations. I would like to work on some sort of pagination feature in the near future (unless someone else is already looking into this!)

Yes, we should discuss this in the group. It's true that it's already possible to have too many values come back from a query than should be acceptable -- and we need to figure out what we want to do about it (i.e., how do we want to do pagination, etc.).

Basically the problem is one of performance. By returning only document locations, clients are forced to do separate requests for each document, which can be painfully slow. This is a problem for us in Aries-Framework-Go, where we have an EDV storage provider that conforms to a general storage interface, and certain types of operations become very expensive. By returning full documents, we prevent having to make extra calls. It also ends up being useful for "bulk get" types of operations.

Yeah, we should discuss in the group how we want to handle this. There may also be some impacts/differences in the abstract API vs the HTTP one here.

The exact way I implemented this could also be discussed... perhaps it should be a separate endpoint? The way I have it in this PR just matches the way I did it in our TrustBloc implementation, but I'm not sure if this is really the best way.

Right -- I think it's a good subject for discussion in the group before heading in a particular direction in the spec.

OR13 commented 3 years ago

ping @tplooker @dmitrizagidulin can we get a special topic call on scalable API design and pagination on the books?

DRK3 commented 3 years ago

Hi @OR13 , @tplooker , and @dmitrizagidulin!

I wanted to check in and see if we've had a change to discuss scalable API design and pagination? I see in @OR13's last message you requested a special topic call. Is that still upcoming, or has it already happened (and perhaps I missed it)? Thanks!

OR13 commented 3 years ago

@DRK3 yes, we are working with chairs to setup a special topic call, sorry this is taking so long.

DRK3 commented 3 years ago

(moved to https://github.com/decentralized-identity/edv-spec/pull/2)