Skip to content

PUT in REST API does not upsert object with custom ID #7139

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

Open
3 tasks done
ArkeshGKalathiya opened this issue Jan 22, 2021 · 43 comments
Open
3 tasks done

PUT in REST API does not upsert object with custom ID #7139

ArkeshGKalathiya opened this issue Jan 22, 2021 · 43 comments

Comments

@ArkeshGKalathiya
Copy link

ArkeshGKalathiya commented Jan 22, 2021

New Feature / Enhancement Checklist

Current Limitation

When integrating third party apis with server, most of them already provide the unique ids for the returned data. So right now if I want to save object with custom id it requires restful api call from node environment, and to make restful api call ( batch request ), we need to check with the server if object with custom id exist or not. If object exist then batch array requires PUT otherwise it requires POST. So it is kind of little overhead and extra management of the code. And also in fast updating environment it can cause errors ( where multiple node in same cluster checks with the backend if certain object id exist, it can cause race conditions ). so It would be nice to have support for saving objects in node environment without rest apis, and maybe upsert option when saving parse object.

Feature / Enhancement Description

So It would be great to have feature where parse server sdk supports saving object based on custom object ids ( it doesn't support at the moment in cloud code and server sdk, it requires rest api cal in node environment to save such objects with custom object id ) and one flag with something like Parse.Object.save({},{upsert:true}), which updates existing documents if object id is already there in database.

Alternatives / Workarounds

Making redundant requests to check if object exist in server

@mtrezza
Copy link
Member

mtrezza commented Jan 28, 2021

Thanks for suggesting.

If I understood your description correctly, there are two separate suggestions:

a) Add setting custom object ID to Parse JS SDK
b) Add upsert operation when saving object with custom ID

Regarding the Parse JS SDK, there is already an open issue for setting a custom object ID, see parse-community/Parse-SDK-JS#1097. When adding the custom ID option to the Parse JS SDK, then the upsert option may makes sense there. This feature is related to the Parse JS SDK, therefore the issue discussion and PR would take place in that repo.

Regarding the REST API, the equivalent of a database upsert would be a http PUT operation. According to the http specification:

If the Request-URI refers to an already existing resource, the enclosed entity SHOULD be considered as a modified version of the one residing on the origin server. If the Request-URI does not point to an existing resource, and that URI is capable of being defined as a new resource by the requesting user agent, the origin server can create the resource with that URI.

If you save an object with a custom ID using the REST API, and a PUT operation does not create a new object if it didn't exist yet or update the existing object, then this issue should be classified as a bug. If you can verify this behavior, would you be willing to write a failing test case?

Note: I have renamed the title of this issue to limit its scope on the REST PUT operation, which is the "upsert" behavior for the REST API you are suggesting (b). For the upsert option for the Parse JS SDK (a) you could open a separate issue in the Parse JS SDK repo, but I think this really only makes sense after parse-community/Parse-SDK-JS#1097 has been solved.

Related:

@mtrezza mtrezza changed the title Upsert flag would be very useful PUT in REST API does not upsert object with custom ID Jan 28, 2021
@ArkeshGKalathiya
Copy link
Author

Hi @mtrezza, Thanks for the response.

I have confirmed that PUT request for non existent object is not working, it gives error.

https://docs.parseplatform.org/rest/guide/#updating-objects

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2021

Thanks for confirming, can you please create a Pull Request and write a test case for this?

@ArkeshGKalathiya
Copy link
Author

Can you please give me suggestion about where I can write test case? Do I have to pull parse-server repo and put test case there? or separate code would be sufficient?

@mtrezza
Copy link
Member

mtrezza commented Feb 4, 2021

Yes, you would

  • fork Parse Server
  • clone it locally
  • create a new branch
  • add a test to an already existing test file, such as ParseServerRESTController.spec.js (there may be another one that is more focused on schema changes); you can look for inspiration in the existing tests, there should be already existing tests for schema change via REST API.
  • push the branch to your fork on github
  • submit a PR to the Parse Server repo with your branch

You can find more details in the Contribution Guide. Don't get scared, the guide needs some rework and we're working on it. If you need any guidance please feel free to ask.

@mtrezza
Copy link
Member

mtrezza commented Feb 10, 2021

@ArkeshGKalathiya Do you need any help for creating a test case?

@ArkeshGKalathiya
Copy link
Author

@mtrezza No, I was just busy with other stuff. I will soon add test case and submit PR.

@dplewis
Copy link
Member

dplewis commented Mar 10, 2021

@ArkeshGKalathiya I added customID to the JS SDK. Feel free to provide feedback

parse-community/Parse-SDK-JS#1309

@dplewis dplewis closed this as completed Mar 15, 2021
@mstniy
Copy link
Contributor

mstniy commented Apr 20, 2021

Hi @mtrezza, Thanks for the response.

I have confirmed that PUT request for non existent object is not working, it gives error.

https://docs.parseplatform.org/rest/guide/#updating-objects

This issue with PUT requests still persists. I believe this issue should be re-opened.

@mtrezza
Copy link
Member

mtrezza commented Apr 20, 2021

I'm not sure why this was closed, seemingly by mistake, because the ref PR is about custom IDs, @dplewis? Maybe they are two different issues?

I think we can reopen if we have a failing test case.

@mstniy
Copy link
Contributor

mstniy commented Apr 21, 2021

I don't think there is a test case that covers this. ParseAPI.spec.js has a few test for PUT, but none of them cover the case of using custom object ids to create a new object.

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2021

@mstniy Would you want to add a test case?

@mstniy
Copy link
Contributor

mstniy commented Apr 22, 2021

Sure thing.

My understanding is that POST should create a new object. For requests with a custom object id, if the object already exists, it should fail. Whereas PUT should basically be an upsert. Please let me know if I am mistaken.

What should a pure (non-upsert) update look like?

Should the PR fix the issue with PUT as well as introducing corresponding regression tests?

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2021

What should a pure (non-upsert) update look like?

A PATCH request. Unlike PUT which is always a full representation of an object, a PATCH is a partial representation and contains instructions on how to modify a resource.

That is the HTTP standard. That being said, if Parse is currently violating that standard, we'd have to look into what a correction implies in terms of scope of changes internally and breaking changes for existing deployments.

@mstniy
Copy link
Contributor

mstniy commented Apr 22, 2021

It seems Parse is indeed violating the standard. From the docs about PUT requests:

Any keys you don’t specify will remain unchanged, so you can update just a subset of the object’s data.

So the current PUT is actually a PATCH: It is a strict update, not an upsert, and it takes as parameter a partial representation.
How about using query strings? For example, we can enable upsert semantics only on PUT requests of the form /classes/Foo/test_id?upsert=true

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2021

That sounds like a violation of http/1.1. A PUT must include a full object, not a partial object.

RFC 5789 says:

The PUT method is already defined to overwrite a resource with a complete new body, and cannot be reused to do partial changes.

and

In a PUT request, the enclosed entity is considered to be a modified version of the resource stored on the origin server, and the client is requesting that the stored version be replaced. With PATCH, however, the enclosed entity contains a set of instructions describing how a resource currently residing on the origin server should be modified to produce a new version.

What happens when making a PUT request for an object that doesn't exist?

@mstniy
Copy link
Contributor

mstniy commented Apr 22, 2021

Quoting the specification you have mentioned:

If the Request-URI does not point to an existing resource, and that URI is capable of being defined as a new resource by the requesting user agent, the origin server can create the resource with that URI. If a new resource is created, the origin server MUST inform the user agent via the 201 (Created) response.

Currently, Parse returns an OBJECT_NOT_FOUND

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2021

You get that response for PUT or PATCH?

Also, we should be careful with citing specs, because part of them may be outdated. Before PATCH has been introduced, servers used PUT to update objects, deliberately violating the spec. That led to the introduction of PATCH.

The first spec I posted is from 1999, the second from 2010 which modified parts of RFC 2616.

@mstniy
Copy link
Contributor

mstniy commented Apr 22, 2021

For PUT

I don't think Parse uses PATCH?

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2021

So if I understand correctly, there are 2 issues:

  • a) The main issue that PUT does not create an object if it doesn't exists yet, which it should according to RFC 5789.
  • b) The secondary issue that Parse does not support PATCH but uses PUT for partially updating an object, which is also a violation.

@mstniy
Copy link
Contributor

mstniy commented Apr 22, 2021

I agree
But I believe fixing it per the specification would likely break existing code

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2021

I suppose fixing (a) would be a breaking change, but possibly not have wide implications for Parse SDKs. It would only be a breaking change for mechanisms that are depending on PUT failing for non-existent objects. Making a PUT for a non-existent object is probably unintended in most cases anyway.

Fixing (b) would likely affect all Parse SDKs, and may not be worth the effort for now.

@mstniy
Copy link
Contributor

mstniy commented Apr 22, 2021

Introducing upserts as a query string option sounds like a good compromise.

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2021

Not sure about that, shouldn't we rather go into the direction of correcting the behavior instead of leaving the incorrect behavior in place and adding an option? Adding an option seems like adding a 5th wheel to a car because it has a flat tyre.

@mstniy
Copy link
Contributor

mstniy commented Apr 22, 2021

I would be happy to do that, as our use case wouldn't be negatively affected.
But it will likely be a breaking change for certain users.

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2021

A breaking change that is actually a correction (of the http spec violation) is generally acceptable.

I am just not sure what that change implies.

  • Do we need to make changes in Parse SDKs as well?
  • Are any other mechanisms in Parse Server affected?
  • In which scenarios would that be breaking change for existing deployments; any examples?

@mstniy
Copy link
Contributor

mstniy commented Apr 23, 2021

Here's my take:

  • We rename the existing PUT operation to PATCH.
  • We make the PUT use upsert and take as parameter full objects.
  • We switch ParseObject.save() from doing a PUT to a PATCH.
  • We need a new method for PUTs, maybe ParseObject.put()?

I am not sure as to whether any mechanism would be affected.
And of course, this could break any code base that uses the REST api directly. The fix would be to change from PUT to PATCH.

@mtrezza
Copy link
Member

mtrezza commented Apr 23, 2021

It seems what you are suggesting is to become fully spec compliant.

If that was easily achievable without major implications I would agree. If there are major implications, we may need to be a bit more pragmatic. Parse Server works after all, even if it is not fully spec compliant.

We could look at these changes:

  • a) new PATCH route for partial update functionality -> no breaking change, evaluating Parse Server internals needed, for example this requires changes in Idempotency feature
  • b) add upsert functionality to existing PUT while keeping partial update functionality -> breaking change, evaluating all Parse SDKs and Parse Server internals needed
  • c) remove partial update functionality from existing PUT -> breaking change with wide implications, across all Parse SDKs and custom code for most Parse Server deployments that use the REST API

Which of these changes we make depends on the implications for the product and developers. Thinking about timeline, we already have a lot of other breaking changes accrued for Parse Server 5, so I wouldn't expect fundamental breaking changes like (c) above to go into effect before Parse Server 7 (~Jan 2023).

@mtrezza mtrezza reopened this Apr 23, 2021
@mtrezza
Copy link
Member

mtrezza commented Apr 23, 2021

Reopened because issue still seems to exist.

@cbaker6
Copy link
Contributor

cbaker6 commented May 17, 2021

Here's my take:

  • We rename the existing PUT operation to PATCH.
  • We make the PUT use upsert and take as parameter full objects.
  • We switch ParseObject.save() from doing a PUT to a PATCH.
  • We need a new method for PUTs, maybe ParseObject.put()?

I am not sure as to whether any mechanism would be affected.
And of course, this could break any code base that uses the REST api directly. The fix would be to change from PUT to PATCH.

@mstniy suggested steps seems reasonable to me as they would hopefully yield the least of amount of errors added to the server (and salvage all of the current PUT testcases) as you both mentioned that the current parse-server's PUT acts as PATCH.

The changes on the client SDKs will most likely follow the same process and should be easier to do (thinking from the Swift SDK and probably some of the others, this would be straight forward). To the point of:

We need a new method for PUTs, maybe ParseObject.put()?

Will something like .replace() work for put? This way on the client side, save will do POST/PATCH (created/updated) and if the user really wants to replace an object, they use .replace().

Maybe when these changes occur, we can take advantage of the X-Parse-Client-Version header (I believe on the JS SDK is currently using this, but we can add the header to all PUT calls on all "patched" clients). We basically modify this on the server

function compatible(compatibleSDK) {
return function (clientSDK) {
if (typeof clientSDK === 'string') {
clientSDK = fromString(clientSDK);
}
// REST API, or custom SDK
if (!clientSDK) {
return true;
}
const clientVersion = clientSDK.version;
const compatiblityVersion = compatibleSDK[clientSDK.sdk];
return semver.satisfies(clientVersion, compatiblityVersion);
};
}
function supportsForwardDelete(clientSDK) {
return compatible({
js: '>=1.9.0',
})(clientSDK);
}

When a client's version is compatible, use the correct version of PUT. If the client version is less, or no header provided (allow backwards compatibility with older client versions) use PATCH on the server instead of PUT. This will allow the "patched" server to be able to deal with any client version.

Of course, if someone uses a "patched" client SDK with an non "patched" server they will have unexpected behavior.

@cbaker6
Copy link
Contributor

cbaker6 commented Aug 29, 2021

@mtrezza bringing your attention back here as it seems like an important discussion. I’m not sure if a PR was ever started by @mstniy

@mstniy
Copy link
Contributor

mstniy commented Aug 29, 2021

No, I haven't created a PR yet

@mtrezza
Copy link
Member

mtrezza commented Aug 29, 2021

Thanks, I think we agree so far that this requires a significant change affecting all SDKs, if we want to solve this sustainably.

Deprecating a feature across all SDKs is a novelty in the context of our newly introduced Deprecation Policy. The question is how to do it in a way that is:

  • a) technically sustainable
  • b) transparent for the developer dealing with multiple versions of SDKs
  • c) according to our deprecation policy to give the developer time to adjust while still being able to upgrade to new versions

Luckily we have some tools:

  • using the X-Parse-Client-Version
  • adding a compatibility table for Parse SDKs <-> Parse Server, this is a missing piece already now
  • the Parse Server deprecator

Do we already agree on which approach to take? I think we'd need to compare implications.

@cbaker6
Copy link
Contributor

cbaker6 commented Aug 29, 2021

How do you mean? Replacing a full object would be a PATCH. There is no HTTP method to replace a full object.

I think what I meant here is .replace() would call the “new“ PUT Command after the PATCH Command is added (which replaces the old PUT).

@cbaker6
Copy link
Contributor

cbaker6 commented Aug 29, 2021

c) according to our deprecation policy to give the developer time to adjust while still being able to upgrade to new versions

I would imagine the developers who will run into the most issues are the ones who don’t use the Client SDKs and instead use REST or Graph directly, but we can assume those who to send the X-Parse-Client-Version are using the deprecated way, as @mstniy mentioned in #7139 (comment)

@cbaker6
Copy link
Contributor

cbaker6 commented Aug 29, 2021

Do we already agree on which approach to take? I think we'd need to compare implications.

Following @mtrezza’s approach seems right to deprecate in a step-by-step way. My question here, is this process needed because of the tools already available? Meaning following the process @mstniy mentioned and I commented on here may fix this now without causing issues (I’m saying this without seeing the CI updates of course) because it will automatically support the current way and the new way.

Note that when the SDKs get the fix (avoiding “patch” because of the context of the convo) many of them will also need to add the X-Parse-Client-Version Header because I don’t think most of them are using it besides JS and the Swift SDKs. parse-community/Parse-Swift#146 provides some insight on part of the features that will need to be added to the other SDKs

@mtrezza
Copy link
Member

mtrezza commented Aug 29, 2021

Ah right, we need to give an option to update vs. upsert.

@mtrezza
Copy link
Member

mtrezza commented Aug 29, 2021

I've edited this comment several times now, I think I'll have to sleep over this 🙂

@cbaker6
Copy link
Contributor

cbaker6 commented Aug 29, 2021

Some things to think about when it comes to server responses for ParseObjects. Currently a PUT returns updatedAt to the client, while a POST returns createdAt.

The proposed changes seem to leave POST unchanged, PATCH returns updatedAt, and POST returning either createdAt or updatedAt? For the new POST, the logic will need to be handled in the client SDKs.

@mtrezza
Copy link
Member

mtrezza commented Aug 30, 2021

TL;DR

For now I suggest to only add an upsert option or method to the SDK to add the missing upsert funcionality without changing any routes. Later in 2023/24 we can plan a breaking change to adapt Parse Server to become spec compliant and properly handle PUT, PATCH requests.


Let's try to break this down...

There are two separate issues:

  • a) Fix the issue of not offering a way to upsert an object. Upsert means to create a new object if no objectId is set or the set objectId does not exist, or replace an entire object if the objectId exists.
  • b) Fix the issue of Parse Server not being http spec compliant because it doesn't have a PATCH route for partial object updates, and the PUT route currently incorrectly behaves as would be expected from a PATCH route.

These two issues can be looked at individually, because:

  • to fix (a) it is not required to fix (b), but fixing (b) will also fix (a)
  • while (a) is an actually issue that impedes development with Parse Server, (b) is a mere formality that does not (as much)
  • (b) requires a change that breaks the backward compatibility of new SDKs with old Parse Servers:
    • old SDK <-> old Parse Server: no break
    • old SDK <-> new Parse Server: no break; Parse Server can use X-Parse-Client-Version to decide whether to treat PUT request as spec compliant PUT or PATCH
    • new SDK <-> new Parse Server: no break; same as old SDK <-> new Parse Server
    • ❌ new SDK <-> old Parse Server: break; Parse Server treats PUT request as spec compliant PATCH and instead of entirely replacing the object it is partially updated; and it apparently does not have upsert behavior to a new object, as originally described in this issue.

This means that (b) will pose the challenge to developers, because if they want to upgrade to a new SDK version, they will have to upgrade to new Parse Server as well. Upgrading Parse Server is currently a major obstacle for many deployments, because Parse Server 5.0 will bring many changes, some of them breaking. Many deployments seem to run on old versions of Parse Server and there is a certain "upgrade fatigue" because in the past we had no deprecation policy, sudden breaking changes, infrequent and non-matured releases, no LTS, etc. This situation is expected to improve dramatically from Parse Server 5.0 onwards and over the course of 2022/23.

This means we are not in a good place at the moment to fix (b) by introducing a breaking change that will be a burden for many developers. It would also be an unnecessarily high burden, because (b) is merely a formality. So I suggest to fix (b) when we can expect a majority of developers having jumped onto the release train because they know that keeping their deployments up-to-date is almost effortless. I expect that to be sometime in 2023/24.

If we solely focus on (a), an approach could be:

  1. Create PR to Parse Server to add upsert option to PUT request to tell Parse Server whether to treat the PUT like a spec compliant PUT or PATCH. Default should be PATCH, which is the status quo.
  2. Create PR to add a compatibility table Parse Server <-> Parse SDKs to Parse Server docs. This is currently a missing instrument, regardless of this issue, but in fixing this issue it becomes even more important to create transparency for the developer because not every SDK will be compatible with every version of Parse Server.
  3. Create PR to SDK can offer an obj.save({ upsert: true }); option or a dedicated obj.upsert(); method. This fixes (a) because it gives the developer a way to request a spec compliant PUT, which is currently not possible. A dedicated obj.upsert(); method may be more convenient, because we can easily show a deprecation warning at compile time, whereas a save option would only probably only be evaluable during runtime or on the server side.
  4. Developers can upgrade to new SDK, knowing that they will also have to upgrade to new Parse Server if they want to use the upsert option. All other developers who do not use theupsert option (that is the majority I suspect) can continue to upgrade only the SDK without having to upgrade Parse Server.

To prepare for the breaking change in 2023/24:

  1. Create PR to add a PATCH route to Parse Server. This alone makes Parse Server a bit more http spec compliant, because currently is has no PATCH route at all, even if PUT behaves like PATCH. This PR can be done anytime, as it only adds a route.

Sometime in 2023/24 (Parse Server 6/7):

  1. Create PR for SDK to use PATCH for partial object update, PUT for entire object replacement if exists or object creation if not exists. This creates an incompatibility with old Parse Server versions. This should be done once we assume that we have overcome the current "upgrade fatigue".

@cbaker6 This is rather long, but I would appreciate if you could examine the details and let me know your opinion.

@cbaker6
Copy link
Contributor

cbaker6 commented Dec 12, 2021

In attempt to have the Swift SDK as close to spec compliant as possible while still making it work with the current Parse Server, I made changes in parse-community/Parse-Swift#299.

Just further confirming that when I remove the guard on objectId when attempting to replace()(PUT/upsert), I get the following when testing on parse-server:5.0.0-beta.4:

  • remove() with no objectId: I receive a 404 with Cannot PUT /1/classes/GameScore.
  • remove() with a new objectId: error: Object not found. {"code":101,"stack":"Error: Object not found.\n at /parse-server/lib/Controllers/DatabaseController.js:581:17\n at processTicksAndRejections (internal/process/task_queues.js:95:5)"}

The same results occur no matter the server setting, allowCustomObjectId = true/false.

I've created a branch of ParseSwift for testing upsert in Swift Playgrounds whenever development on the server-side for upsert and update()(PATCH) begins.

@ArkeshGKalathiya
Copy link
Author

Any update? How can I help on this?

@mtrezza
Copy link
Member

mtrezza commented Mar 21, 2022

@ArkeshGKalathiya You could read through this thread and suggest a PR.

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

No branches or pull requests

5 participants