-
Notifications
You must be signed in to change notification settings - Fork 10
allow jsonapi type customization #67
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
Conversation
Kaycell
left a comment
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.
The implementation looks solid and non-breaking, but I’m not fully convinced this is solving a real limitation yet.
Could you share a concrete example of a scenario that can’t be achieved today without these new interfaces?
From the PR description it seems like this mostly helps avoid defining multiple relationship structs (e.g. UserRelationship, TeamRelationship), but those are usually small and explicit. I’m wondering if there’s a production use case where the type really needs to be decided dynamically at runtime rather than just defined statically via tags.
jsonapi.go
Outdated
| // 2. Compare against the type provided in the jsonapi tag on the primary field | ||
| // 3. Fail | ||
| type UnmarshalType interface { | ||
| UnmarshalType(jsonapiTpe string) error |
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.
Typo
unmarshal.go
Outdated
| if vut, ok := v.(UnmarshalType); ok { | ||
| err := vut.UnmarshalType(ro.Type) | ||
| if err != nil { | ||
| return &TypeError{Actual: ro.Type, Expected: []string{jsonapiTag.resourceType}} |
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.
We are losing the error info that the implementer might have provided in the returned error.
We could:
errors.Join(
&TypeError{Actual: ro.Type, Expected: []string{jsonapiTag.resourceType}},
err,
)
Currently, the jsonapi type must be specified as a tag on the struct.
This makes it impossible to dynamically decide the type, requiring a
unique struct per needed relationship:
```go
type User struct {
ID string `jsonapi:"primary,user"
// attributes
}
type Team struct {
ID string `jsonapi:"primary,user"
// attributes
}
type UserRelationship struct {
ID string `jsonapi:"primary,user"
// no attributes
}
type TeamRelationship struct {
ID string `jsonapi:"primary,team"
// no attributes
}
```
With this change, we could instead do:
```
type Relationship[T any] struct {
ID string `jsonapi:"primary,unused"`
}
func (r *Relationship[T]) MarshalType() string {
// custom logic for specifying type, e.g. based on a method on T
}
// Can now use Relationship[User], Relationship[Team]
```
We have many types that can be relationships, let's say: type User struct {
ID string `jsonapi:"primary,users" json:"id"`
Email string `jsonapi:"attribute" json:"email"`
Name string `jsonapi:"attribute" json:"name"`
}Oftentimes however we only have a reference to the type, so we also need: type UserReference struct {
ID string `jsonapi:"primary,users" json:"id"`
}And this is repeated for every type that can appear as a relationship. Moreover, it's not obvious what the full included version of type Post struct {
ID string `jsonapi:"primary,posts" json:"id"`
Author UserReference `jsonapi:"attribute" json:"author"` // what does this look like in `included`?
}This change would allow us to define a It would also make it easier to detect such references for automatic handling of includes, as currently we do it by scanning objects at runtime and trying to guess if they are just references or also contain the full object. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
Currently, the jsonapi type must be specified as a tag on the struct. This makes it impossible to dynamically decide the type, requiring a unique struct per needed relationship:
With this change, we could instead do: