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

Provide PATCH API call for scaffolded (users) API #112

Open
NoTuxNoBux opened this issue May 22, 2023 · 4 comments
Open

Provide PATCH API call for scaffolded (users) API #112

NoTuxNoBux opened this issue May 22, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@NoTuxNoBux
Copy link

NoTuxNoBux commented May 22, 2023

When scaffolding users, you get a bunch of common API calls, but PATCH seems to be missing:

afbeelding

For my use case specifically, I'm extending users to support local user management in a first phase of a project, so fields such as the (new) plain text password cannot be repeated every time, only when it needs to be updated, at which point it is hashed and permanently discarded.

Ideally a PATCH API call would exist similar to the PUT one that allows omitting fields if they don't need to be changed, and fields that are explicitly set (null` or otherwise) will only be written to.

This might also apply to scaffolded API calls for entities in general, but I noticed it specifically for users.

(I haven't tested yet if PUT supports this behaviour, but even if it does, typically both are separate actions in RESTful APIs which shouldn't be conflated.)

Not a biggy, as it is straightforward to add, but just wanted to mention it.

@NoTuxNoBux NoTuxNoBux added the enhancement New feature or request label May 22, 2023
@NoTuxNoBux NoTuxNoBux changed the title No PATCH API call for scaffolded (users) API Provide PATCH API call for scaffolded (users) API May 22, 2023
@pdevito3
Copy link
Owner

Yeah, so I had a patch at one point but it was adding a lot of overhead and complexity. It was partially due to the implementation using Patchdoc which has a few gaps still.

I might add something in as an alternative but would need to figure out a good convention for it apart from patchdoc.

If you have any thoughts on a good common structure feel free to toss it up for consideration.

@pdevito3
Copy link
Owner

Fwiw on users though, there wouldn't be the password use case like this since we're not working with an IDP in the boundary or managing passwords here. With the scaffolded setup that is.

But point stands on coming up with a potential patch option

@NoTuxNoBux
Copy link
Author

NoTuxNoBux commented May 23, 2023

Yes, I understand 🙂 . It's a lot like PUT, but with the additional complexity that a field not being present is not the same as a field being present and null (i.e. don't change field as opposed to change field but try to clear its value).

Fwiw on users though, there wouldn't be the password use case like this since we're not working with an IDP in the boundary or managing passwords here. With the scaffolded setup that is.

Indeed, out of the box there are no passwords anyway, so it doesn't apply. Might still be useful to generate a PATCH call for users once it's implemented for 'standard' entities as well, since there might be management interfaces that prefer to either only send changed data from forms, or there might be clients that would just like to change a single field such as the email without having to retrieve the existing 'body' using a GET call first in order to repeat the data.

@NoTuxNoBux
Copy link
Author

NoTuxNoBux commented May 25, 2023

If you have any thoughts on a good common structure feel free to toss it up for consideration.

FWIW for the time being I followed something like https://overflow.adminforge.de/questions/72910769/placeholder to differentiate between fields not being present and null in DTOs. It's kind of weird now, though, since it not only applies to the DTO, but also to the UserForModification class that is used in the entity to perform the update, as it also needs to be able to differentiate between the two.

Another idea was to have a wrapper type using generics such as ExplicitlySet<string> MyField, but it probably makes things more complex than this regarding JSON (de)serialization, so I didn't opt for that solution right now (just mentioning it for completeness' sake).

It's probably possible to let the PUT code reuse most of the same logic for the new PATCH call since it's essentially a PATCH where all fields are present. Doing it all the way through is probably not entirely correct since the PUT DTO ideally validates all fields are present as it's a full replacement. However, currently, at least for users, there are no validation attributes such as Required or the new required keyword in the DTOs, so leaving fields out in PUT seems to behave the same as setting them to be null, which I must admit is rather dangerous, so I changed this behaviour in my code.

Other ideas I had might involve auto-generating one DTO based on the other, but removing Required attributes using source generators or something, with the option to override them if you want something else. This still gives you full control, but most of the time all these DTO's will be very similar and it's boilerplate you're maintaining for little gain beyond being architecturally better, I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants