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

Unexpected behaviour for d2's .update and .save #78

Open
ghost opened this issue Nov 4, 2019 · 3 comments
Open

Unexpected behaviour for d2's .update and .save #78

ghost opened this issue Nov 4, 2019 · 3 comments
Labels
backend frontend question Further information is requested

Comments

@ghost
Copy link

ghost commented Nov 4, 2019

Whilst working on an issue in the user-app @HendrikThePendric and I discovered that d2's update and save methods behave a little unexpectedly (for me at least): d2’s api.update uses a PUT and model.save also does a PUT if the model has an ID.

This caused a bug in the user-app. Because not all fields were included on an update it caused certain fields to change. We're not sure if there are other areas that assume d2's update to work like a PATCH. But if there are you might encounter similar things.

Core issue is that PUT is really not the expected HTTP verb for partial updates (PATCH would be more apt). So Hendrik and I thought it'd be good to open an issue in notes, make sure that this doesn't surprise anyone else. I assume that eventually the front-end side of this issue will be resolved by our new app-runtime.

Though we might run into the backend not being used to receiving PATCH updates, as Hendrik mentioned that certain endpoints weren't able to digest PATCHes that well (and I've encountered similar things with the jobs endpoint for the scheduler app).

@varl varl added backend frontend question Further information is requested labels Nov 18, 2019
@amcgee
Copy link
Member

amcgee commented Dec 11, 2019

This is indeed concerning, and also relates to how we implement partial updates and full replacements in the data engine.

@ismay I wasn't aware of the inconsistency in PATCH request handling... can we collect a list of the endpoints that behave abnormally? To start:

  • You mentioned the jobs endpoint, presumably that's /api/jobConfiguration? What behavior is invalid or inconsistent when sending a PATCH request there?

@ghost
Copy link
Author

ghost commented Dec 11, 2019

It's a while ago. I encountered it whilst working on the scheduler app: https://jira.dhis2.org/browse/DHIS2-7237. It was fixed, and just a minor bug, but I assume it wasn't caught because it works fine when using a PUT, which was probably all that was used before I tried out a PATCH.

@Birkbjo
Copy link
Member

Birkbjo commented Jan 3, 2020

PATCH requests has generally not been well supported by the API, and that is why PUT is used. I'm not too up to date how well the endpoints support PATCH right now, and it's something we should discuss with the backend team. It would also help a lot in the new Maintenance app, as we currently have to post complete objects instead of using PATCH, which would help save bandwidth and prevent bugs that you mention.

There's also an query-parameter mergeMode that can be set to REPLACE or MERGE. When set to merge it kinds of works like a PATCH, but you still have to pass required fields...

D2 was designed "around" this, and will generally PUT all fields that are needed. So this is more of an API-restriction than a unexpected behavior of D2.

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

No branches or pull requests

3 participants