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

Autopatch does not work with nullable fields #634

Open
betaprior opened this issue Nov 1, 2024 · 3 comments
Open

Autopatch does not work with nullable fields #634

betaprior opened this issue Nov 1, 2024 · 3 comments
Labels
question Further information is requested

Comments

@betaprior
Copy link

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.

@danielgtaylor danielgtaylor added the bug Something isn't working label Nov 7, 2024
@danielgtaylor
Copy link
Owner

@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 danielgtaylor added question Further information is requested and removed bug Something isn't working labels Dec 5, 2024
@betaprior
Copy link
Author

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?

@danielgtaylor
Copy link
Owner

danielgtaylor commented Dec 9, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants