huma icon indicating copy to clipboard operation
huma copied to clipboard

Autopatch does not work with nullable fields

Open betaprior opened this issue 1 year ago • 5 comments

Consider a simple struct with two nullable string fields:

type Foo struct {
	A *string `json:"a"`
	B *string `json:"b"`
}

It is evidently impossible to set these fields to nil via a PATCH with application/merge-patch+json content type. For instance, the following request will throw a 422 with a message "expected required property a to be present".

curl --request PATCH \
  --url http://localhost:8888/foo/1 \
  --header 'Accept: application/json, application/problem+json' \
  --header 'Content-Type: application/merge-patch+json' \
  --data '{
  "a": null
}'

This seems like a bug in merge patch computation: the PUT body in the autogenerated GET/PUT sequence omits the a field altogether instead of setting a: null as expected.

The only way to set a to nil appears to be via a PUT of the full struct or via a json-patch (not merge-patch):

curl --request PATCH \
  --url http://localhost:8888/foo/1 \
  --header 'Accept: application/json, application/problem+json' \
  --header 'Content-Type: application/json-patch+json' \
  --data '[
  { "op": "replace", "path": "/a", "value": null }
]

This came as an unfortunate surprise as our front-end client started making use of PATCH requests.

betaprior avatar Nov 01 '24 06:11 betaprior

@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:

define MergePatch(Target, Patch):
  if Patch is an Object:
    if Target is not an Object:
  Target = {} # Ignore the contents and set it to an empty Object
    for each Name/Value pair in Patch:
  if Value is null:
    if Name exists in Target:
      remove the Name/Value pair from Target
  else:
    Target[Name] = MergePatch(Target[Name], Value)
    return Target
  else:
    return Patch

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 PUT endpoint indeed has the a field removed, which in turn causes the validation error because the field is required.

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

danielgtaylor avatar Dec 05 '24 06:12 danielgtaylor

I'm curious what you think the correct behavior would be in this case.

That's a tough one. For POST or PUT requests, we can make the following choices for API behavior vis-a-vis null and/or missing fields: (a) implicitly set omitted fields to NULL (i.e. with json:"omitempty" or required:"false") (b) disallow omitted fields (they have to be explicitly set to NULL) (with required:"true" or without json:"omitempty")

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 POST and PUT requests with the brevity of partial updates via a merge patch, but I don't see a way of achieving this with the merge patch. Do you?

betaprior avatar Dec 08 '24 01:12 betaprior

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 null values in place for the PUT operation. You don't have to exactly follow JSON merge patch - just make it useful for you and your clients.

Edit: you could also consider a shorthand patch, which differentiates between null and undefined https://github.com/danielgtaylor/shorthand?tab=readme-ov-file#patch-partial-update. This format is actually already built-in to Huma https://github.com/danielgtaylor/huma/blob/main/autopatch/autopatch.go#L249-L271.

danielgtaylor avatar Dec 09 '24 16:12 danielgtaylor

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 null values in place for the PUT operation.

I was hesitant to mess with the behavior of a fairly popular standard (something related to the principle of least surprise) but for our use case this seems like the best approach. Would you be open to adding optional behavior to autopatch that would go something like the following:

  • parse the PATCH body and convert null values to strings, e.g. __null__ (but probably user customizable, since if you intentionally choose to go the "stringly-typed" route, you should have control over magic string values)
  • run the json merge-patch as the autopatch currently does
  • convert the resultant magic strings (__null__ in this example) back to null
  • make this code path strictly optional so as not to have any risk of breakage or unexpected behavior

I quickly prototyped this and it seems fairly straightforward and works as expected for my use case.

betaprior avatar Apr 02 '25 00:04 betaprior

See https://github.com/danielgtaylor/huma/pull/791 for our implementation of the alternative autopatch behavior.

betaprior avatar Apr 08 '25 00:04 betaprior