Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

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
Collaborator

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 moved this to 🏗 In progress in bifrost-gateway Apr 6, 2023
@lidel lidel self-assigned this Apr 6, 2023
@lidel
Copy link
Collaborator 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
Collaborator 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
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in bifrost-gateway Jun 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant