-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
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]) |
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 |
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 |
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)
} |
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! |
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:
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