TBD54566975 / web5-go

Apache License 2.0
7 stars 6 forks source link

Document.Context mixed types #104

Closed KendallWeihe closed 4 months ago

KendallWeihe commented 4 months ago

Closes https://github.com/TBD54566975/web5-go/issues/92

When I implemented the did:web resolution work, I realized the @context property could be a string, an array of strings, or an array of strings and objects (and specifically, LinkedIn's DID Doc had the mixed types).

I implemented the work to support mixed types specifically for the @context property, but I also reviewed the spec and found other properties which are mixed type and need to be implemented (list below).

The work here is three-part:

  1. Unmarshal support
  2. Tests
  3. Example

I ventured into deeper Golang areas here, so @alecthomas @mihai-chiorean @mistermoe any go-specific feedback on this PR would be much appreciated. Thanks!

Created new tickets for the other areas in the DID Document where mixed types exist

KendallWeihe commented 4 months ago

Honestly, I'm not totally sold this is the ideal approach so please feel free to propose alternative DX's

Specifically, I don't love that users may have to make type assertions, like from the example...

context, ok := doc.Context.([]didcore.Context)
if !ok {
    panic(errors.New("error unmarshalling Document"))
}
fmt.Printf("Document @context array string item: %s\n", context[0])
mistermoe commented 4 months ago

Will take a look on Monday morn! Thinking we may want to have a discussion to figure out where we want to land across all sdks. I think our strongly typed representations of did core data models that have mixed types, particularly in scenarios where it's "string or list of strings", should just use list of strings.

Like we'll unmarshal both and if it's a string we'll just make it a list of strings with 1 elem. then when marshaling we don't have to do anything special. This does come at the cost of us having to write our own unmarshal functions for did core data models but I'm thinking it may keep things simpler in application logic that uses these models. Also less cognitive overhead having to remember "oh ya this can be an x or a y or even a Z every 3rd Thursday of even years."

Lmk wyt

KendallWeihe commented 4 months ago

Also less cognitive overhead having to remember "oh ya this can be an x or a y or even a Z every 3rd Thursday of even years."

100% agree

I like your proposal of "just make them all a list of strings." Additional unmarshal code but that's fine. Won't hurt my feelings at all if we don't move forward with this PR; I'd rather keep the code base light if we don't have full-confidence in our approach. We can use things here as reference.

The situation of list of strings and/or maps is something we'll need to confront too... and it doesn't quite fit into place with your proposal of "always a list of strings." Situations where this may arise are the @context (here), serviceEndpoints and all of the various places where Verification Methods can be embedded instead of linked to (though w/ that one we could just always result in linking in the same vein as your proposal, support unmarshalling but unmarshal always into a link not an embed). serviceEndpoints being the one which may be a high enough priority to prompt a decision.

alecthomas commented 4 months ago

If the context type could only be a string or a list of strings, I think that would be the way to go. But the addition of "ordered maps" complicates things. I would probably suggest doing something like this:

type Context struct {
  String string
  StringList []string
  OrderedMap [][2]any
}

func (c *Context) UnmarshalJSON(data []byte) error {
  if err := json.Unmarshal(data, &c.String); err == nil {
    return nil
  }
  if err := json.Unmarshal(data, &c.StringList); err == nil {
    return nil
  }
  if err = json.Unmarshal(data, &c.OrderedMap); err == nil {
  }
  return  fmt.Errorf("invalid context %s: expected string, list of strings, or ordered map", data)
}
KendallWeihe commented 4 months ago

We have a direction on this w/ the web5 spec broadly, under active development here https://github.com/TBD54566975/web5-spec/pull/122

Tickets created in the PR description can probably be used for tracking the work going forward. Closing this PR, but will be good for use as reference!