Update ipld prime, use proper code-gen #5
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,8 +7,8 @@ import ( | |
|
|
||
| "github.com/ipfs/go-cid" | ||
| merkledag_pb "github.com/ipfs/go-merkledag/pb" | ||
| "github.com/ipld/go-ipld-prime/fluent" | ||
| cidlink "github.com/ipld/go-ipld-prime/linking/cid" | ||
| ) | ||
|
|
||
| // byteAccessor is a reader interface that can access underlying bytes | ||
|
|
@@ -17,8 +17,8 @@ type byteAccesor interface { | |
| } | ||
|
|
||
| // DecodeDagProto is a fast path decoding to protobuf | ||
| // from PBNode__Builders | ||
| func (nb *_PBNode__Builder) DecodeDagProto(r io.Reader) error { | ||
| var pbn merkledag_pb.PBNode | ||
| var encoded []byte | ||
| var err error | ||
|
|
@@ -34,64 +34,63 @@ func (nb _PBNode__NodeBuilder) DecodeDagProto(r io.Reader) error { | |
| if err := pbn.Unmarshal(encoded); err != nil { | ||
| return fmt.Errorf("unmarshal failed. %v", err) | ||
| } | ||
| return fluent.Recover(func() { | ||
| fb := fluent.WrapAssembler(nb) | ||
| fb.CreateMap(0, func(fmb fluent.MapAssembler) { | ||
| fmb.AssembleEntry("Links").CreateList(len(pbn.Links), func(flb fluent.ListAssembler) { | ||
|
There was a problem hiding this comment. So I think @rvagg 's comment about the links list here suggests there should be a branch on I can't decide if I think that's weird or not, though, now that I think about it. I'm hedging on if I think we should update our IPLD Schema describing this to just say that the list is required... and then if it's zero-length, sure, we can encode it to the proto wire format as missing. That's a discussion that probably @rvagg and I should hash out on our specs documents about it though. If this is the behavior this code has had already, I think it's fine to keep it today. (I don't want to force you through testing a change like this in either direction today...) |
||
| for _, link := range pbn.Links { | ||
| hash, err := cid.Cast(link.GetHash()) | ||
|
|
||
| if err != nil { | ||
| panic(fluent.Error{Err: fmt.Errorf("unmarshal failed. %v", err)}) | ||
|
There was a problem hiding this comment. @rvagg : I went deep on exactly what happens here if the protobuf has a nil value for hash, because of its relevance to our other discussions about documenting this behavior: tl;dr: a lack of hash here is not actually tolerated by this code: we get an error returned here. (In detail: So I think this is a pretty solid argument that we don't have to consider |
||
| } | ||
| flb.AssembleValue().CreateMap(0, func(fmb fluent.MapAssembler) { | ||
| fmb.AssembleEntry("Hash").AssignLink(cidlink.Link{Cid: hash}) | ||
| fmb.AssembleEntry("Name").AssignString(link.GetName()) | ||
|
There was a problem hiding this comment. I'm moderately confused what the protobuf library is doing here -- we had to hand the protobuf lib a pointer to the name string elsewhere, because it's option in protobuf's mind, right? But here it's returning a string (no pointer). Does this panic if the string is missing? Or silently coerce it to an empty string? There was a problem hiding this comment. yea it silently coerces it tp empty string. Which is weird behavior except -- that's exactly what go-merkledag uses in in IPFS. My basic approach is follow as close as possible so as to not break things. There was a problem hiding this comment. Okay, cool. I appreciate that, thanks for talking through it with us so we have it also as input to the other standardization quests @rvagg mentioned :) |
||
| fmb.AssembleEntry("Tsize").AssignInt(int(link.GetTsize())) | ||
|
There was a problem hiding this comment. Same question as above about name strings. Does this panic if the field isn't in the protobuf? Or silently coerce it to a zero? |
||
| }) | ||
| } | ||
| }) | ||
| fmb.AssembleEntry("Data").AssignBytes(pbn.GetData()) | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| // EncodeDagProto is a fast path encoding to protobuf | ||
| // for PBNode types | ||
| func (nd PBNode) EncodeDagProto(w io.Writer) error { | ||
| pbn := new(merkledag_pb.PBNode) | ||
| pbn.Links = make([]*merkledag_pb.PBLink, 0, nd.FieldLinks().Length()) | ||
| linksIter := nd.FieldLinks().ListIterator() | ||
| for !linksIter.Done() { | ||
| _, nlink, err := linksIter.Next() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| link := nlink.(PBLink) | ||
| var hash []byte | ||
| if link.FieldHash().Exists() { | ||
| cid := link.FieldHash().Must().Link().(cidlink.Link).Cid | ||
|
There was a problem hiding this comment.
|
||
| if cid.Defined() { | ||
| hash = cid.Bytes() | ||
| } | ||
| } | ||
| var name *string | ||
| if link.FieldName().Exists() { | ||
| tmp := link.FieldName().Must().String() | ||
|
There was a problem hiding this comment. I'd write a review about how all this causes unfortunate allocations, but... it's unavoidable in interacting with protobufs, as I understand it. Ah well. (Fun fact if we ever come back to optimize this though, without just tearing deep through and doing the parse protobuf ourselves completely: I think we could bump these |
||
| name = &tmp | ||
| } | ||
| var tsize *uint64 | ||
| if link.FieldTsize().Exists() { | ||
| tmp := uint64(link.FieldTsize().Must().Int()) | ||
| tsize = &tmp | ||
| } | ||
| pbn.Links = append(pbn.Links, &merkledag_pb.PBLink{ | ||
| Hash: hash, | ||
| Name: name, | ||
| Tsize: tsize}) | ||
| } | ||
| pbn.Data = nd.FieldData().Bytes() | ||
| data, err := pbn.Marshal() | ||
| if err != nil { | ||
| return fmt.Errorf("marshal failed. %v", err) | ||
|
|
@@ -104,25 +103,27 @@ func (nd PBNode) EncodeDagProto(w io.Writer) error { | |
| } | ||
|
|
||
| // DecodeDagRaw is a fast path decoding to protobuf | ||
| // from RawNode__Builders | ||
| func (nb *_RawNode__Builder) DecodeDagRaw(r io.Reader) error { | ||
| return fluent.Recover(func() { | ||
| fnb := fluent.WrapAssembler(nb) | ||
|
There was a problem hiding this comment. Will definitely cause at least one extra alloc, and idk if it's paying for itself in simplicity added in this case. But I'm not complaining enough to demand a change, either. Probably not a big issue unless we profile it and find out that it is. |
||
| byteBuf, ok := r.(byteAccesor) | ||
| if ok { | ||
| fnb.AssignBytes(byteBuf.Bytes()) | ||
| return | ||
| } | ||
| data, err := ioutil.ReadAll(r) | ||
| if err != nil { | ||
| panic(fluent.Error{Err: fmt.Errorf("io error during unmarshal. %v", err)}) | ||
| } | ||
| fnb.AssignBytes(data) | ||
| }) | ||
| } | ||
|
|
||
| // EncodeDagRaw is a fast path encoding to protobuf | ||
| // for RawNode types | ||
| func (nd RawNode) EncodeDagRaw(w io.Writer) error { | ||
| _, err := w.Write(nd.Bytes()) | ||
| if err != nil { | ||
| return fmt.Errorf(" error during marshal. %v", err) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,83 +1,42 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "os/exec" | ||
|
|
||
| "github.com/ipld/go-ipld-prime/schema" | ||
| gengo "github.com/ipld/go-ipld-prime/schema/gen/go" | ||
| ) | ||
|
|
||
| func main() { | ||
|
|
||
| ts := schema.TypeSystem{} | ||
| ts.Init() | ||
| adjCfg := &gengo.AdjunctCfg{} | ||
|
|
||
| pkgName := "dagpb" | ||
|
|
||
| ts.Accumulate(schema.SpawnString("String")) | ||
| ts.Accumulate(schema.SpawnInt("Int")) | ||
| ts.Accumulate(schema.SpawnLink("Link")) | ||
| ts.Accumulate(schema.SpawnBytes("Bytes")) | ||
|
|
||
| ts.Accumulate(schema.SpawnStruct("PBLink", | ||
| []schema.StructField{ | ||
| schema.SpawnStructField("Hash", "Link", true, false), | ||
| schema.SpawnStructField("Name", "String", true, false), | ||
| schema.SpawnStructField("Tsize", "Int", true, false), | ||
| }, | ||
| schema.SpawnStructRepresentationMap(nil), | ||
| )) | ||
| ts.Accumulate(schema.SpawnList("PBLinks", "PBLink", false)) | ||
| ts.Accumulate(schema.SpawnStruct("PBNode", | ||
| []schema.StructField{ | ||
| schema.SpawnStructField("Links", "PBLinks", false, false), | ||
| schema.SpawnStructField("Data", "Bytes", false, false), | ||
| }, | ||
| schema.SpawnStructRepresentationMap(nil), | ||
| )) | ||
| ts.Accumulate(schema.SpawnBytes("RawNode")) | ||
| gengo.Generate(".", pkgName, ts, adjCfg) | ||
| exec.Command("go", "fmt").Run() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd mildly rather that these be -1 when they're saying "shrug idk deal with it". The contract about this isn't strict, but there are some logic branches that look at 0 as a "ah, expect 0" and look at -1 as "ah, take a guess". It's also helpful for reading.
... but none of those codepaths happen apply here in a codegen'd struct, so, this is a nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to improve the godoc on this, I've realized. Mea culpa.