这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@betaprior
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings April 7, 2025 23:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

@betaprior betaprior force-pushed the leo/led-1075-modify-huma-autopatch-behavior branch from 195cb11 to b38e79f Compare April 7, 2025 23:58
@betaprior betaprior changed the title Leo/led 1075 modify huma autopatch behavior Make autopatch merge-patch work with nullable fields Apr 8, 2025
@betaprior betaprior force-pushed the leo/led-1075-modify-huma-autopatch-behavior branch from 7621a1c to 167cb85 Compare April 8, 2025 00:08
@danielgtaylor
Copy link
Owner

@betaprior can you maybe describe why this is needed? I've looked at it a couple times now and am struggling to fully understand this PR. It doesn't comply with https://datatracker.ietf.org/doc/html/rfc7386 and seems to set a sentinel value, but I wonder if this would better be handled using a middleware of some sorts or by using a different patch mechanism than JSON Merge-Patch. For example:

I think the JSON Merge-Patch implementation should probably stick to the RFC standard. What do you think?

@betaprior
Copy link
Contributor Author

betaprior commented Apr 27, 2025

@danielgtaylor we discussed this in #634. In that thread, you said

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.

That's exactly what this PR implements. Notably, it's not an approach that I originally wanted to take for the exact reason that you mentioned -- I don't love the idea of going against RFC standards, and we experimented with using the shorthand patch format, but at the end decided that sending JSON bodies with undefined is even worse with respect to established standards.

Note that the behavior in this PR isn't changing JSON Merge-Patch implementation -- it's provided by the 3rd party library -- but rather mutates the input and the output to the merge patch function so that it results in desired behavior with respect to nulls (i.e., preserving them instead of dropping them); needless to say the non-RFC compliant behavior is disabled by default.

If you think this can be done in middleware, could you outline how? The current approach needs the input and output of the patch function, and offhand I don't think it's possible to access in middleware -- unless I suppose the middleware itself somehow intercepts patch requests and provides its own patch implementation.

@danielgtaylor
Copy link
Owner

Going through this again, I see what you are saying about using a middleware. Any other way would be less efficient than doing this right in the patch handler. I think this is alright to merge as an optional extension and it's a well understood concept to use sentinel values with a special meaning. I don't love it but I can see the value, it provides an alternative people can use, and the implementation is clean. Thanks!

@danielgtaylor danielgtaylor merged commit c7ead6f into danielgtaylor:main May 9, 2025
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