search

bluesky-social / atproto

Social networking technology created by Bluesky
Other
6.37k stars 445 forks source link

Mismatch between protocol spec and implementation for "since" parameter in com.atproto.sync.getRepo #1560

Closed skopf closed 1 year ago

skopf commented 1 year ago

Describe the bug

The latest version of the atprotocol spec defines the optional "since" parameter for "com.atproto.sync.getRepo" to take a CID. See here in the lexicon https://github.com/bluesky-social/atproto/blob/main/lexicons/com/atproto/sync/getRepo.json#L17 and here on the website: https://atproto.com/lexicons/com-atproto-sync#comatprotosyncgetrepo

The actual implementation however is expecting this parameter to be a "refkey" and uses it to filter on the "repoRev" column: https://github.com/bluesky-social/atproto/blob/main/packages/pds/src/sql-repo-storage.ts#L292

To Reproduce

Steps to reproduce the behavior:

  1. Passing a valid DID and commit CID will return a car file with no records:
  2. Passing a valid DID and a commit "rev" will return the error "Error: since must be a cid string"

Expected behavior

Lexicon and implementation match.

Details

If expecting a CID, the implementation first needs to search for this CID, get the repoRev from this record, and then filters based on this repoRev. A potential problem could be that the record with the CID might have been deleted meanwhile. Another option would be to change the protocol spec and expect the RevKey of the commit rather than the CID for the "since" parameter.

dholms commented 1 year ago

Thanks for filing this. You're exactly right that this was not supposed to be cid formatted. That accidentally stuck around when doing the refactor.

I've put a fix up here & as well as a test to support it: https://github.com/bluesky-social/atproto/pull/1579