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

feat: custom marshaling take 2 #20

Closed
wants to merge 1 commit into from

Conversation

ctrombley
Copy link

@ctrombley ctrombley commented Jan 4, 2024

This PR demonstrates how custom marshaling can be achieved without any updates to the jsonapi code.

@ctrombley ctrombley self-assigned this Jan 4, 2024
@brandonc
Copy link
Collaborator

brandonc commented Jan 5, 2024

@nfagerlund So now we have two pretty good approaches. I'm going to try and list all the tradeoffs between them as I understand it.

Approach #18 Allow any attribute type to implement AttributeMarshaler or AttributeUnmarshaler interfaces and customize what is sent and received.

  • allows most flexibility in an explicit interface as an escape hatch
  • adds some new reflection conditions as well as the potential for jsonapi abuse

Approach #20 Rely on the fact that "Marshaling" (transforming jsonapi annotated structs as payloadable) already works when a type implements MarshalJSON

  • no change necessary for jsonapi
  • doesn't work when using the custom type for decoding response bodies as jsonapi annotated structs

I like #20 (this PR) because literally sending JSON null is the only workaround we've ever needed and this addresses that in the downstream without introducing any new complexity or uncertainty to jsonapi. If we need to introduce "custom unmarshaling", which may not even be necessary, we can figure that out separately.

@nfagerlund
Copy link
Member

@brandonc I like this PR's approach. 👍🏼 I just talked through this with Chris on zoom, and yeah, my prior approaches and research were kind of blinded by my desire for symmetry... but if we restrict ourselves to ONLY making (null || absent) distinctions in outgoing json (i.e. UpdateOptions structs), then yeah, I can totally cope with the remaining asymmetry. ("This field uses a weird type in WorkspaceUpdateOptions despite being a *Time in Workspace" is orders of magnitude nicer than "this ONE scalar field needs a dedicated set/unset method".)

@nfagerlund
Copy link
Member

We might need to revisit the question later, but if we can avoid making atlas APIs where we need to distinguish null from absent in incoming json, the day might never arrive.

@ctrombley
Copy link
Author

superceded by #21

@ctrombley ctrombley closed this Jan 30, 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.

3 participants