-
-
Notifications
You must be signed in to change notification settings - Fork 172
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
Autopatch does not work with nullable fields #634
Comments
@betaprior I dug into this a bit. I originally thought this might be a bug but now I'm not so sure. I re-read RFC 7386 JSON Merge Patch, specifically:
https://datatracker.ietf.org/doc/html/rfc7386#section-2 This matches my understanding of Huma's autopatch implementation, and adding in some debug statements I'm able to see that the resulting request made to the I'm curious what you think the correct behavior would be in this case. One thing that you can do is make the field optional. For example this works fine: https://go.dev/play/p/EJmLwkLJ4ci See also https://huma.rocks/features/request-validation/#optional-required |
That's a tough one. For In my APIs, I tend to favor (b) since it forces the client to be explicit about their intent, and rules out the situation where they neglect to send a field thereby unintentionally nulling it out. However, the semantics of merge patch is, as we discovered, incompatible with this choice, since NULL means "omit this field", thus for nullable fields, only choice (a) will work. This is unfortunate, because I was hoping to combine the safety of explicitly requiring nullable fields for |
I tend to agree this doesn't seem possible with the standard merge-patch, however if you always require these fields consistently in your APIs then I would consider copying the autopatch code and modifying it slightly to leave the Edit: you could also consider a shorthand patch, which differentiates between |
Consider a simple struct with two nullable string fields:
It is evidently impossible to set these fields to
nil
via aPATCH
withapplication/merge-patch+json
content type. For instance, the following request will throw a 422 with a message "expected required property a to be present".This seems like a bug in merge patch computation: the
PUT
body in the autogeneratedGET
/PUT
sequence omits thea
field altogether instead of settinga: null
as expected.The only way to set
a
tonil
appears to be via aPUT
of the full struct or via ajson-patch
(notmerge-patch
):This came as an unfortunate surprise as our front-end client started making use of PATCH requests.
The text was updated successfully, but these errors were encountered: