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

Document.Context mixed types #104

Closed
wants to merge 4 commits into from

Conversation

KendallWeihe
Copy link
Contributor

Closes #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
Copy link
Contributor Author

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])

@KendallWeihe KendallWeihe mentioned this pull request Mar 1, 2024
@mistermoe
Copy link
Contributor

mistermoe commented Mar 1, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

We have a direction on this w/ the web5 spec broadly, under active development here decentralized-identity/web5-spec#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!

@KendallWeihe KendallWeihe deleted the kendallw/did-doc-contexts-mixed-types branch April 4, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

didcore.Document @context non-string types
3 participants