search

ipfs / go-cid

Content ID v1 implemented in go
MIT License
156 stars 47 forks source link

refactor: remove invalid string2code mappings #137

Closed lidel closed 2 years ago

lidel commented 2 years ago

Problem: string2codec maps are a mess

The current state is not sustainable, string mappings are invalid or missing, and we don't have correct ones for core IPLD formats like dag-pb, dag-json and dag-cbor :see_no_evil:

Solution: cleanup

Problem 2: cleanup may introduce silent bugs into people's codebases

https://github.com/ipfs/go-cid/pull/137/commits/da5524780a4ae73d6b2ca765f1df8e7e8b7b0bcd demonstrates the map changes we would have to do to restore sanity: fixes mapping, and adds IPLD formats along with non-dag generic variants for Protobuf, JSON and CBOR, so go-cid finally match the source of truth.

This approach would change the meaning of code 0x70 and 0x71 and strings cbor and protobuf:

Pretty risky – people dont read release notes, this could cause silent errors in people's software. Also, we have proper code2string mappings in go-multicodec (generated, and not written by hand)

Solution

Remove invalid/redundant string2code mappings from go-ipfs, point ecosystem at using go-multicodec directly.

BREAKING CHANGE

Implemented Approach B: Remove invalid mappings entirely :green_heart:

I applied this in https://github.com/ipfs/go-cid/pull/137/commits/6ca2617460d553e1aa10d4fdced1dcfedd300197

We already have go-multicodec which is generated from the source of truth.

We remove hardcoded maps, then people will be forced to refactor their code to use go-multicodec (https://github.com/multiformats/go-multicodec/issues/65) – forcing people to refactor makes it less likely they will just blindly bump the dependency without reading release notes.

Refactor guide

Converting code to string and vice versa will no longer be done with go-cid, go-multicodec should be used instead

Old way (go-cid)


cid "github.com/ipfs/go-cid"

// string → code
code := cid.Codecs["libp2p-key"]

// code → string
string := cid.CodecToStr[code]

New way (go-multicodec)

mc "github.com/multiformats/go-multicodec"

// string → code
var mc.Code code 
code.Set("libp2p-key")

// code → string
code.String()
BigLep commented 2 years ago

2022-03-24 conversation:

  1. We want to delete the table. We generate this programmatically already in https://github.com/multiformats/go-multicodec
lidel commented 2 years ago

I went with Approach B (Remove mappings). Filled https://github.com/multiformats/go-multicodec/issues/65 to figure out the best way existing users could migrate, will ship this as soon we figure that out.

lidel commented 2 years ago

Based on https://github.com/multiformats/go-multicodec/issues/65#issuecomment-1078919431 I wrote migration guide (will include it in release notes).

This is now ready to merge as soon I get some reviews.

aschmahmann commented 2 years ago

@mvdan the reason to break users with the maps but not the constants is because the maps are wrong. The most egregious issue is that "cbor" is mapped to dag-cbor (0x71) instead of to cbor (0x51). There's also the protobuf vs dag-pb issue, but that's less terrible. If we just fixed the maps then people would silently break which is IMO the worst of the options.

If you're breaking users, have you thought about removing the constants and pointing users towards go-multicodec directly?

Why would you choose to break users when you could just not at trivial cost? We could add Deprecated markers pointing people at go-multicodec though.

It might be a bit too much in terms of breakage, but if we're already replacing the from/to string APIs, replacing the constants as well would seem consistent.

With some basic searching:

So breaking the maps seems to cause much less trouble for people than also breaking the constants.

mvdan commented 2 years ago

ACK, I hadn't caught the detail about the bad map entries.

lidel commented 2 years ago

note to self: before I merge this, I'll test this in go-ipfs, see how painful refactor is etc (I'll do that as part of review of https://github.com/ipfs/go-ipfs/pull/8568)

lidel commented 2 years ago
BigLep commented 2 years ago

2022-03-31 conversation: the work isn't blocked, but it won't get merged and released until the libp2p resource manager work for ipfs.

lidel commented 2 years ago

This is unblocked and ok to merge:

TLDR: if you've found this PR because you need to map CODE to STRING or STRING TO CODE, see usage https://github.com/multiformats/go-multicodec#usage

github-actions[bot] commented 2 years ago

Suggested version: v0.2.0 Comparing to: v0.1.0 (diff)

Changes in go.mod file(s):

diff --git a/go.mod b/go.mod
index e307ff9..6e3b580 100644
--- a/go.mod
+++ b/go.mod
@@ -4,7 +4,17 @@ require (
    github.com/multiformats/go-multibase v0.0.3
    github.com/multiformats/go-multihash v0.0.15
    github.com/multiformats/go-varint v0.0.6
+)
+
+require (
+   github.com/klauspost/cpuid/v2 v2.0.4 // indirect
+   github.com/minio/blake2b-simd v0.0.0-20160723061019-3f5f724cb5b1 // indirect
+   github.com/minio/sha256-simd v1.0.0 // indirect
+   github.com/mr-tron/base58 v1.2.0 // indirect
+   github.com/multiformats/go-base32 v0.0.3 // indirect
+   github.com/multiformats/go-base36 v0.1.0 // indirect
    golang.org/x/crypto v0.0.0-20210506145944-38f3c27a63bf // indirect
+   golang.org/x/sys v0.0.0-20210309074719-68d13333faf2 // indirect
 )

-go 1.15
+go 1.17

gorelease says:

# github.com/ipfs/go-cid
## incompatible changes
CodecToStr: removed
Codecs: removed
## compatible changes
DagJSON: added

# summary
Suggested version: v0.2.0

gocompat says:

(empty)