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

Rename ?format=car URL params to match IPIP-402 #80

Closed
7 of 14 tasks
lidel opened this issue Apr 6, 2023 · 2 comments · Fixed by #144
Closed
7 of 14 tasks

Rename ?format=car URL params to match IPIP-402 #80

lidel opened this issue Apr 6, 2023 · 2 comments · Fixed by #144
Assignees

Comments

@lidel
Copy link
Member

lidel commented Apr 6, 2023

This issue is based on discussions I had with @aschmahmann and with @hannahhoward (slack) about around depth=0|1|all.
cc ipfs/specs#348

Problem

The depth=0|1|all parameter was provisionally introduced as part of Rhea project and we quickly realized we need to improve if we want to upstream it to https://specs.ipfs.tech/http-gateways/trustless-gateway/

The name is unfortunate, because after all Graph API interations we've ended up with "depth" that lost original meaning ("how deep to fetch a DAG (or path)"), and ended up meaning "logical depth from the perspective of end user thinking in terms of a single block, a file or a directory enumeration".

In that framing, the name makes very little sense:

  • a block can be resolved only via depth=0
  • things that support byte-range seeking, like unixfs files, can have max depth=1
  • hypothetical depth=2 makes sense only for directories and DAGs built out of IPLD things like DAG-JSON/CBOR documents (but we don't use this for Rhea nor need for gateway atm).

I don’t think we should have an integer range where there are only two possible values with well-defined behavior.

I agree, we ended up with three states:

  • 0 - blocks for path + only the root block for terminal CID
    • Gateway uses this for IPFSBackend.GetBlock and for IPFSBackend.ResolvePath
  • 1 - blocks for path + all blocks for a file, or a minimal set of blocks to enumerate directory or HAMT
    • Gateway uses this for all IPFSBackend.Get requests, as we don't want to over-fetch directories, and we don't know which path ends with a dir
  • all - blocks for path + all blocks for entire DAG
    • Gateway uses this only for TAR (IPFSBackend.GetAll)and CAR (IPFSBackend.GetCAR) responses, but this is also the implicit default for ?format=car, so we don't really need this.

I think we both want to clean this up. and we agree "depth" is simply an invalid term here. I also agree with you, strings are more meaningful than numbers for this abstraction layer.

What we want

  • this is not "depth" → this is more a predefined "scope" with specific meaning
  • hard to reason what integers mean, or what the parameter is related to → include "car" in name, avoid magical IPLD/ADL terms, and other PL-specific words
  • we don't accept integers >1 → make it opaque string
  • we may want to use "depth" for selecting actual DAG depth in the future → rename

Solution

Based on my notes and discussion with Hannah, we agreed car-scope=block|file|all is a better framing:

  • depth=0car-scope=block
  • depth=1car-scope=file (non-files, like directories, and IPLD Maps only return blocks necessary for their enumeration)
    • we use file here to make more sense to non-PL user. "file" is a synonym for "the blocks needed to interact with the ADL in the transformed view", and the pathing on /ipfs/ defines the implicit ADLs to use.
  • depth=allcar-scope=all (implicit default when car-scope is unset, send everything)

This is way easier to reason about, no need to read docs, hard to make a mistake,
and a better fit for a future IPIP that updates https://specs.ipfs.tech/http-gateways/trustless-gateway/ which we want to do this year.

Transition plan.

Updated plan

lidel added a commit that referenced this issue Apr 6, 2023
This adds support for 'car-scope' URL param next to existing 'depth'
to allow Saturn L1/Lassie to migrake without breaking anything
during the transition.

Context: #80
lidel added a commit that referenced this issue Apr 6, 2023
This adds support for 'car-scope' URL parameter next to existing 'depth'
to allow Saturn L1/Lassie to migrate without breaking anything
during the transition.

Context: #80
@lidel lidel self-assigned this Apr 6, 2023
@lidel
Copy link
Member Author

lidel commented Apr 7, 2023

Eyeballed metrics on staging, unsurprisingly the file scope is requested the most often:

Screenshot 2023-04-07 at 02-02-48 bifrost-gw staging metrics - Project Rhea - Dashboards - Grafana

@lidel lidel changed the title Replace depth with car-scope parameter Rename BRAPH_BACKEND URL params to match IPIP-402 May 15, 2023
@lidel
Copy link
Member Author

lidel commented May 15, 2023

We had another round of renames in ipfs/specs#402 (comment):

  • name changes based on rough consensus. (Juan, Rod, and Adin all suggested removing ambiguity and name dependency on CAR and UnixFS formats, making things more future-proof):
    • car-scope=filedag-scope=entity
    • bytesentity-bytes

Repurposing this issue to align bifrost-gateway implementation with specs.

@lidel lidel changed the title Rename BRAPH_BACKEND URL params to match IPIP-402 Rename ?format=car URL params to match IPIP-402 May 16, 2023
lidel added a commit that referenced this issue Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant