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

Renaming a member via the Rest-API reset optional arguments (activated & weight) #1259

Open
Turtle6665 opened this issue Nov 11, 2023 · 3 comments
Labels

Comments

@Turtle6665
Copy link
Contributor

Hello,

When a member name is updated without filling the optional argument, it resets them to a weight of one and a deactivation. So in short, any optional argument not given will be reset.
For example, with this code:

#if we give the activated field & the weight along side the name
curl --basic -u demo:demo -X PUT https://ihatemoney.org/api/projects/demo/members/36807 -d 'name=yeaaaaah&activated=true&weight=2'
#> {"id": 36807, "name": "yeaaaaah", "weight": 2.0, "activated": true}

#if we only give the name
curl --basic -u demo:demo -X PUT https://ihatemoney.org/api/projects/demo/members/36807 -d 'name=yeaaaaah'
#> {"id": 36807, "name": "yeaaaaah", "weight": 1.0, "activated": false}

What I assumed by reading the API documentation is that those values are optional and, when not given, will not change their value. I therefore assume it's a bug, but it may be the expected behaviour ?

Have a nice day

@almet
Copy link
Member

almet commented Nov 11, 2023

Hi, thanks for opening this issue. I think you're correct : this is a bug. We should look in the database before erasing the values with the defaults.

https://github.com/spiral-project/ihatemoney/blob/master/ihatemoney/api/common.py#L100-L110

@almet almet added the bug label Nov 11, 2023
@zorun
Copy link
Collaborator

zorun commented Nov 12, 2023

It's a general issue with the API, similar to #1205

That being said, that's probably how a "REST-compliant API" is supposed to behave for PUT requests with optional arguments. PUT is supposed to completely replace the resource, so it would theoretically make sense to reset optional values to their default. But this is usually not what we want, especially when we add new optional arguments that some clients may not know about yet.

So, I believe the "clean" REST way should be to implement PATCH instead.

@almet
Copy link
Member

almet commented Nov 13, 2023

I agree with you on the semantics, but that's not how it's implemented now unfortunately. We could change the API if we want :-)

But, in the meantime, I think we've used PUT as a PATCH, and I think the problem (in this issue and #1205 is a bug) : as I mentioned in #1261, WTForms considers that no value for a Boolean field means it should be set to False.

That's not how it should behave in an API, and I think that's what causes the problem here.

I've submitted a proposed solution in #1261, but changing the implementation of the BooleanField could be another proposal that would fix it for all the similar cases.

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

No branches or pull requests

3 participants