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

feat: add pre caveat to store/list and upload/list #423

Merged
merged 6 commits into from
Feb 10, 2023
Merged

Conversation

travis
Copy link
Member

@travis travis commented Feb 6, 2023

Per storacha/w3ui#143 we'd like to give clients a way to get the previous page of results rather than the next page.

This change introduces a pre caveat which, when set to true, will return the page of results preceding cursor.

I considered calling this reverse or rev but I think that implies that results will be returned sorted in reverse, which is not the intent.

`store/list` and `upload/list` both accept paging parameters -
document them.
Per storacha/w3ui#143 we'd like to give
clients a way to get the previous page of results rather than the next
page.

This change introduces a `prev` caveat which, when set to true, will
return the page of results preceding `cursor`.

I considered calling this `reverse` or `rev` but I think that implies
that results will be returned sorted in reverse, which is not
the intent.
@travis travis marked this pull request as draft February 6, 2023 08:24
@travis travis changed the title feat: add prev caveat to store/list and upload/list proposed feat: add prev caveat to store/list and upload/list Feb 6, 2023
@travis travis changed the title proposed feat: add prev caveat to store/list and upload/list feat: add prev caveat to store/list and upload/list (proposal) Feb 6, 2023
travis added a commit to storacha/w3infra that referenced this pull request Feb 6, 2023
this implements support for the `prev` caveat proposed in storacha/w3up#423

I'm not sure the semantics are right yet, deploying to get a test env to play with
travis added a commit to storacha/w3infra that referenced this pull request Feb 6, 2023
this implements support for the `prev` caveat proposed in storacha/w3up#423

I'm not sure the semantics are right yet, deploying to get a test env to play with
@alanshaw
Copy link
Member

alanshaw commented Feb 6, 2023

@travis if I'm understanding this correctly, this would enable you to page in either direction, but would prevent switching direction without storing some state (the cursor for the current page).

I think I had imagined this as being a change to the return value, where you'd get 2 cursors previous and next instead of one cursor. Then you could use that to get the previous or next page without having to store any state.

@travis
Copy link
Member Author

travis commented Feb 6, 2023

ahh, I think that would be a better API for this yes - I'm going to close this for now and investigate that direction, thanks!

@travis travis closed this Feb 6, 2023
@travis travis reopened this Feb 7, 2023
@travis
Copy link
Member Author

travis commented Feb 7, 2023

ok I did some investigation into this today and as far as I can tell, returning a prev cursor as proposed above would require doing an extra DynamoDB query here:

https://github.com/web3-storage/w3infra/blob/main/upload-api/tables/upload.js#L150

something like:

      const prevCmd = new QueryCommand({
        TableName: tableName,
        Limit: options.size || 20,
        KeyConditions: {
          space: {
            ComparisonOperator: 'EQ',
            AttributeValueList: [{ S: space }],
          },
        },
        ExclusiveStartKey: exclusiveStartKey,
        AttributesToGet: ['root', 'shards'],
        ScanIndexForward: false
      })
      const response = await dynamoDb.send(prevCmd)
      const lastKey = response.LastEvaluatedKey && unmarshall(response.LastEvaluatedKey)
      const prevCursor = lastKey ? lastKey.root : undefined

(may need to tweak some things to avoid off-by-one errors)

I don't think it's possible to avoid doing two queries here, unless there's some way to scan forward and backward in a single DynamoDB query, so my question is: is adding an extra database query to every upload/list worth it to provide a UX convenience? I genuinely don't have good intuition for this at the moment, so would love your thoughts @alanshaw.

I'm going to hold off digging deeper on this until we can hash this out at a design level...

Base automatically changed from chore/document-paging to main February 7, 2023 05:05
@travis travis marked this pull request as ready for review February 8, 2023 06:47
@travis travis changed the title feat: add prev caveat to store/list and upload/list (proposal) feat: add prev caveat to store/list and upload/list Feb 8, 2023
@travis travis requested review from alanshaw, Gozala and gobengo February 8, 2023 06:53
@travis
Copy link
Member Author

travis commented Feb 8, 2023

OK - chatting out of band we decided this probably is the right way to go, since returning a cursor to the previous page would require an extra DynamoDB query. While I'm not a huge fan of driving protocol design with the constraints of a particular implementation, I do think that this is likely to be an issue in just about any database we implemented this with, and is probably appropriate in this case.

Given that, I think this PR is ready for review!

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good but maybe we should name this parameter differently? prev implies there's some kind of time element here - i.e. it was previously retrieved...but that's not necessarily the case.

Maybe this is weird but what about just pre?: boolean or precursor?: boolean.

@gobengo
Copy link
Contributor

gobengo commented Feb 8, 2023

prev implies there's some kind of time element here

IMO prev implies ordering but not necessary along the time dimension, and it makes sense here too in the same way it makes sense in:

I don't care very much what we call it, mostly jumping in to point out how other specs have distinguished (or not) between separating ordering along some dimension vs ordering along (one) time dimension

@travis
Copy link
Member Author

travis commented Feb 9, 2023

yep, fwiw it also doesn't feel time-related to me, and I think the existence of the html link rel makes it feel pretty intuitive.

though I suppose that's the name of a link relation, and this is a boolean that determines whether the index should be read forward or backward...

@alanshaw how would you feel about about rev: boolean, short for "reverse" - this option is specifically designed to communicate that the index should be read in reverse, so that feels pretty intuitive, plus: three letters (good for inclusion in a UCAN) and just one letter off of prev...

@travis
Copy link
Member Author

travis commented Feb 9, 2023

oh lol, reviewing this I realized I made the case against calling it rev initially, and the implications about sort order do still seem relevant...

I'm down to call it pre! short (ie, good for inclusion in UCANs) and vague enough that it doesn't feel like it implies much, but directional enough that it should read reasonably in code.

I'll get this PR updated.

@travis travis changed the title feat: add prev caveat to store/list and upload/list feat: add pre caveat to store/list and upload/list Feb 9, 2023
@travis travis requested a review from alanshaw February 9, 2023 09:02
@alanshaw
Copy link
Member

alanshaw commented Feb 9, 2023

@gobengo noted, thank you. Although, the things you mentioned (rel, AS2 etc.) are links sent by the server to the previous document in the sequence but here we're using it as an option (a boolean) from the client to enable a specific behaviour in the server. i.e. that we want the page of results preceeding the cursor we've sent. So I think this is a different enough situation to warrant a divergence, and as @travis pointed out, folks can think of pre as "previous" if that works better for them.

Ideally we'd have what I was originally advocating for - which would be previous and next, sent by the server...but we decided against due to limitations of our tech choices 🙈.

spec/capabilities.md Outdated Show resolved Hide resolved
spec/capabilities.md Outdated Show resolved Hide resolved
travis and others added 2 commits February 10, 2023 11:01
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
@travis travis merged commit a0f6d28 into main Feb 10, 2023
@travis travis deleted the feat/list-prev-page branch February 10, 2023 03:09
travis pushed a commit that referenced this pull request Feb 10, 2023
🤖 I have created a release *beep* *boop*
---


##
[2.3.0](capabilities-v2.2.0...capabilities-v2.3.0)
(2023-02-10)


### Features

* add `pre` caveat to `store/list` and `upload/list`
([#423](#423))
([a0f6d28](a0f6d28))
* add access/delegate capability parser exported from
@web3-storage/capabilities
([#420](#420))
([e8e2b1a](e8e2b1a))
* add support for access/authorize and update
([#392](#392))
([9c8ca0b](9c8ca0b)),
closes [#386](#386)
* define access/claim in @web3-storage/capabilities
([#409](#409))
([4d72ba3](4d72ba3))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
travis added a commit that referenced this pull request Feb 10, 2023
🤖 I have created a release *beep* *boop*
---


##
[5.5.0](upload-client-v5.4.0...upload-client-v5.5.0)
(2023-02-10)


### Features

* add `pre` caveat to `store/list` and `upload/list`
([#423](#423))
([a0f6d28](a0f6d28))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: Travis Vachon <travis@dag.house>
travis added a commit to storacha/specs that referenced this pull request Feb 10, 2023
travis added a commit to storacha/specs that referenced this pull request Feb 13, 2023
travis added a commit to storacha/w3infra that referenced this pull request Feb 16, 2023
Implement support for the `pre` caveat introduced in
storacha/w3up#423

Also introduce `startCursor` and `endCursor` with the intention to
deprecate `cursor`. By providing cursors for the beginning and end of
the range, we enable clients to page forward or backward without saving
state anywhere.

Example usage:

Given uploads like:

```bash
$ w3 ls
bafybeia7tr4dgyln7zeyyyzmkppkcts6azdssykuluwzmmswysieyadcbm
bafybeibvbxjeodaa6hdqlgbwmv4qzdp3bxnwdoukay4dpl7aemkiwc2eje
bafybeid2fbwlsb7euczxw7ig3q3fyezw7tfkcikrer75w4ucfawh5vqfdm
bafybeidibxjgswcu2q4gx7j5mzq26qtcczt3nczg6k4ybqct2rm567kpoy
bafybeie7vavzb2ojufppuaxer3kfu4yxi36z7ozdz7wnhtylnd7rl5p6ji
bafybeiefiddap3p3z3p3obatviipgda2us3umt4uxeh2trrni5pt6bfc6e
bafybeifohbynkellbberk3hi447dntungwgicmsvxuwdttk5k5k535mw2a
bafybeigkeedelu7fkqgeafzh7zrtwmcn747clncxuufl2dzu46mww735qq
bafybeign7hb7ynhc6o3sevas365xuzuqzrhpfzgksi4dzpcbphl4xrerye
bafybeignyyeaqn6b2fzqswrozt2txexjbwesrt4dbxlquoac4nn7mbi2gm
bafybeigz7o2ibtv6k5pnklfza3ebsl6jeveguoyxwvrmmtvp2lkvqqjyem
```

You can now page forward like:

```bash
$ w3 can upload ls --size 4
bafybeia7tr4dgyln7zeyyyzmkppkcts6azdssykuluwzmmswysieyadcbm
bafybeibvbxjeodaa6hdqlgbwmv4qzdp3bxnwdoukay4dpl7aemkiwc2eje
bafybeid2fbwlsb7euczxw7ig3q3fyezw7tfkcikrer75w4ucfawh5vqfdm
bafybeidibxjgswcu2q4gx7j5mzq26qtcczt3nczg6k4ybqct2rm567kpoy
```

```bash
$ w3 can upload ls --size 4 --cursor bafybeidibxjgswcu2q4gx7j5mzq26qtcczt3nczg6k4ybqct2rm567kpoy
bafybeie7vavzb2ojufppuaxer3kfu4yxi36z7ozdz7wnhtylnd7rl5p6ji
bafybeiefiddap3p3z3p3obatviipgda2us3umt4uxeh2trrni5pt6bfc6e
bafybeifohbynkellbberk3hi447dntungwgicmsvxuwdttk5k5k535mw2a
bafybeigkeedelu7fkqgeafzh7zrtwmcn747clncxuufl2dzu46mww735qq
```

```bash
$ w3 can upload ls --size 4 --cursor bafybeigkeedelu7fkqgeafzh7zrtwmcn747clncxuufl2dzu46mww735qq
bafybeign7hb7ynhc6o3sevas365xuzuqzrhpfzgksi4dzpcbphl4xrerye
bafybeignyyeaqn6b2fzqswrozt2txexjbwesrt4dbxlquoac4nn7mbi2gm
bafybeigz7o2ibtv6k5pnklfza3ebsl6jeveguoyxwvrmmtvp2lkvqqjyem
```

and page backward like:

```bash
$ w3 can upload ls --size 4 --cursor bafybeign7hb7ynhc6o3sevas365xuzuqzrhpfzgksi4dzpcbphl4xrerye --pre
bafybeigkeedelu7fkqgeafzh7zrtwmcn747clncxuufl2dzu46mww735qq
bafybeifohbynkellbberk3hi447dntungwgicmsvxuwdttk5k5k535mw2a
bafybeiefiddap3p3z3p3obatviipgda2us3umt4uxeh2trrni5pt6bfc6e
bafybeie7vavzb2ojufppuaxer3kfu4yxi36z7ozdz7wnhtylnd7rl5p6ji
```

```bash
$ w3 can upload ls --size 4 --cursor bafybeie7vavzb2ojufppuaxer3kfu4yxi36z7ozdz7wnhtylnd7rl5p6ji --pre
bafybeidibxjgswcu2q4gx7j5mzq26qtcczt3nczg6k4ybqct2rm567kpoy
bafybeid2fbwlsb7euczxw7ig3q3fyezw7tfkcikrer75w4ucfawh5vqfdm
bafybeibvbxjeodaa6hdqlgbwmv4qzdp3bxnwdoukay4dpl7aemkiwc2eje
bafybeia7tr4dgyln7zeyyyzmkppkcts6azdssykuluwzmmswysieyadcbm
```

Note that the cursor values here happen to match the CID strings - this
is an implementation detail and should not be relied upon. Instead,
cursor values should be taken from `startCursor` and `endCursor` in the
list response:

```bash
$ w3 can upload ls --size 4 --cursor bafybeidibxjgswcu2q4gx7j5mzq26qtcczt3nczg6k4ybqct2rm567kpoy --raw
{"size":4,"cursor":"bafybeigkeedelu7fkqgeafzh7zrtwmcn747clncxuufl2dzu46mww735qq","results":[{"root":{"/":"bafybeie7vavzb2ojufppuaxer3kfu4yxi36z7ozdz7wnhtylnd7rl5p6ji"},"shards":[{"/":"bagbaieracmkgwrw6rowsk5jse5eihyhszyrq5w23aqosajyckn2tfbotdcqq"}],"updatedAt":"2023-02-13T16:30:04.172Z","insertedAt":"2023-02-13T16:30:04.172Z"},{"root":{"/":"bafybeiefiddap3p3z3p3obatviipgda2us3umt4uxeh2trrni5pt6bfc6e"},"shards":[{"/":"bagbaieraz3iyicghnyzpjld7yxf7rmntf5cdbewo4pmuck6kkkc6f7z3sznq"}],"updatedAt":"2023-02-13T16:29:51.864Z","insertedAt":"2023-02-13T10:57:39.017Z"},{"root":{"/":"bafybeifohbynkellbberk3hi447dntungwgicmsvxuwdttk5k5k535mw2a"},"shards":[{"/":"bagbaierasu5cufqagk34dkmhszbwuhiydjjjp3pl4bacyvskljqtkcxhgbta"}],"updatedAt":"2023-02-13T16:30:11.648Z","insertedAt":"2023-02-13T10:57:16.264Z"},{"root":{"/":"bafybeigkeedelu7fkqgeafzh7zrtwmcn747clncxuufl2dzu46mww735qq"},"shards":[{"/":"bagbaieralqhbrq2mqupqijkoylagbams263jwq6helsl6kbdpivupjm7egxq"}],"updatedAt":"2023-02-13T16:30:18.403Z","insertedAt":"2023-02-13T10:57:49.695Z"}],
 "endCursor":"bafybeigkeedelu7fkqgeafzh7zrtwmcn747clncxuufl2dzu46mww735qq","startCursor":"bafybeie7vavzb2ojufppuaxer3kfu4yxi36z7ozdz7wnhtylnd7rl5p6ji"}
```

nb that these w3 commands require the changes I implemented in
storacha/w3cli#45

---------

Co-authored-by: Benjamin Goering <171782+gobengo@users.noreply.github.com>
travis added a commit to storacha/w3cli that referenced this pull request Mar 2, 2023
Implement `can store ls` and `can upload ls` commands, including support
for the `pre` caveat introduced in
storacha/w3up#423 and support for the
`size` and `cursor` caveats, set with options of the same name.

Also make some docs changes for more consistency and less speling erors.
gobengo pushed a commit that referenced this pull request Apr 11, 2023
Per storacha/w3ui#143 we'd like to give
clients a way to get the previous page of results rather than the next
page.

This change introduces a `pre` caveat which, when set to true, will
return the page of results preceding `cursor`.

I considered calling this `reverse` or `rev` but I think that implies
that results will be returned sorted in reverse, which is not the
intent.

---------

Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
gobengo pushed a commit that referenced this pull request Apr 11, 2023
🤖 I have created a release *beep* *boop*
---


##
[2.3.0](capabilities-v2.2.0...capabilities-v2.3.0)
(2023-02-10)


### Features

* add `pre` caveat to `store/list` and `upload/list`
([#423](#423))
([9cce414](9cce414))
* add access/delegate capability parser exported from
@web3-storage/capabilities
([#420](#420))
([7834cf2](7834cf2))
* add support for access/authorize and update
([#392](#392))
([bf41071](bf41071)),
closes [#386](#386)
* define access/claim in @web3-storage/capabilities
([#409](#409))
([2fb34dd](2fb34dd))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
gobengo pushed a commit that referenced this pull request Apr 11, 2023
🤖 I have created a release *beep* *boop*
---


##
[5.5.0](upload-client-v5.4.0...upload-client-v5.5.0)
(2023-02-10)


### Features

* add `pre` caveat to `store/list` and `upload/list`
([#423](#423))
([9cce414](9cce414))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: Travis Vachon <travis@dag.house>
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.

3 participants