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

Unmarshal struct attributes #163

Closed
wants to merge 5 commits into from

Conversation

henrmota
Copy link

@henrmota henrmota commented Oct 12, 2018

Add the ability to marhsal and unmarhsal custom struct attributes as well custom struct slices, either pointers or not

@mkarlesky
Copy link

@shwoodard What needs to happen for this PR to be merged? It appears to fix issues #156 and #164. I confess that there are subtle issues involved that I don't fully grasp. But, I believe I want to use the fixes of this PR instead of relying on a commit from March before the unmarshaling issues were introduced.

@fabrepe
Copy link

fabrepe commented Apr 24, 2019

Any news on this PR ? Why cannot it be merged ?
While this is not merged we need to use @henrmota repository as go module.

@mellena1
Copy link

I would also like to see this merged if possible. I am in agreement with @mkarlesky in the sense that I don't grasp some of the subtle issues, but I think this is an important feature to have.

@codyaray
Copy link

codyaray commented Nov 6, 2019

@shwoodard @aren55555 @Slemgrim can one of you provide a status update on this issue? What are the blockers to merging? What's missing / doesn't work as expected / etc?

EDIT:

I just tested this and it didn't really do what I hoped/expected. I have a struct generated by gogo proto that looks like

type Environment struct {
	Id                   string           `protobuf:"bytes,1,opt,name=id,proto3" json:"id,omitempty" jsonapi:"primary,Environment" db:"id,omitempty" url:"id,omitempty"`
	Name                 string           `protobuf:"bytes,2,opt,name=name,proto3" json:"name,omitempty" jsonapi:"attr,name" db:"name,omitempty" url:"name,omitempty"`
	Created              *types.Timestamp `protobuf:"bytes,6,opt,name=created,proto3" json:"created,omitempty" jsonapi:"attr,created_at" db:"created,omitempty" url:"created,omitempty"`
       ...
}

Normally protobuf's jsonpb marshaller will automatically serialize *types.Timestamp as a nice ISO datetime string. But the current version of jsonapi marshals it as its underlying components like normal encoding/json does (that is, "created": {"seconds": 234234, "nanos": 12342}). And the Payloader object returned from the jsonapi marshaller isn't a proto.Message so you can't just use jsonpb anymore.

So I tried writing a custom marshaller like


func (m *Environment) MarshalJSON() ([]byte, error) {
	s := jsonpb.Marshaler{}
	created, err := s.MarshalToString(m.Created)
	if err != nil {
		return nil, err
	}
	modified, err := s.MarshalToString(m.Modified)
	if err != nil {
		return nil, err
	}
	type Alias Environment
	return json.Marshal(&struct {
		Created  string `json:"created"  jsonapi:"attr,created_at"`
		Modified string `json:"modified" jsonapi:"attr,modified_at"`
		*Alias
	}{
		Created:  created,
		Modified: modified,
		Alias:    (*Alias)(m),
	})
}

But that's never even called by the current version of jsonapi. So I tried this PR hoping it was going to use my custom marshallers for these fields, but instead the output is now {"created_at": null}.

I stepped through the debugger and see that its now trying to treat *types.Timestamp as a relationship-type field to be sideloaded. Specifically, we're now hitting this line of code: https://github.com/google/jsonapi/pull/163/files#diff-b01558cabf60ed66d8a2c08b2bfaba31R381 and nested is coming back as a completely empty Node struct. And because Attributes is nil, created_at is set to nil by node.Attributes[args[1]] = nested.Attributes. (Remember, created_at is the value of args[1] and args[0] is attr) .

I ended up on this PR because of #156 but this doesn't seem to be related, or the PR doesn't really work, or I'm doing something wrong, or all of the above. But I thought I'd share what I was trying/looking for while I'm here. :)

@henrmota henrmota closed this Apr 11, 2024
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.

5 participants