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

Support Idempotency #1210

Merged
merged 10 commits into from
Aug 31, 2020
Merged

Support Idempotency #1210

merged 10 commits into from
Aug 31, 2020

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Aug 19, 2020

Ref: parse-community/parse-server#6744, parse-community/parse-server#6748

Idempotency enforcement for client requests. This deduplicates requests where the client intends to send one request to Parse Server but due to network issues the server receives the request multiple times.
Caution, this is an experimental feature that may not be appropriate for production.

To enable use either of the following:

  • Parse.CoreManager.set('IDEMPOTENCY': true)
  • Parse.idempotency = true;

This PR addresses

  • Add X-Parse-Request-Id uuid to the request.
  • Improve xmlhttprequest error message handling
  • Improve clearing of database after test
  • Improve tests the require indexing

@codecov
Copy link

codecov bot commented Aug 19, 2020

Codecov Report

Merging #1210 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1210      +/-   ##
==========================================
+ Coverage   92.34%   92.36%   +0.01%     
==========================================
  Files          54       54              
  Lines        5294     5307      +13     
  Branches     1191     1195       +4     
==========================================
+ Hits         4889     4902      +13     
  Misses        405      405              
Impacted Files Coverage Δ
src/CoreManager.js 100.00% <ø> (ø)
src/Parse.js 86.17% <100.00%> (+0.58%) ⬆️
src/RESTController.js 85.54% <100.00%> (+0.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8afd4c...108a5aa. Read the comment docs.

@dplewis
Copy link
Member Author

dplewis commented Aug 19, 2020

@mtrezza How does this look? Is this an accurate way to test idempotency?

@mtrezza
Copy link
Member

mtrezza commented Aug 19, 2020

Wow, I had this on my list for next month but you overtook me there 👏
Let me take a look at it later today.

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor comments.

Does the JS SDK have an automated request retry mechanism like the iOS and Android SDKs? If so, I think a test case makes sense that ensures that the UUID does not change on request retry. That may then also require additional logic in the REST controller to ensure that the UUID does not change.

It could make sense to also add test cases for jobs and object save and update, then we would cover the whole range of functionality, but I think initially a test case just for the Parse Cloud function does suffice as well.

src/__tests__/RESTController-test.js Outdated Show resolved Hide resolved
src/__tests__/RESTController-test.js Outdated Show resolved Hide resolved
src/RESTController.js Outdated Show resolved Hide resolved
@mtrezza
Copy link
Member

mtrezza commented Aug 20, 2020

Does the JS SDK have an automated request retry mechanism like the iOS and Android SDKs? If so, I think a test case makes sense that ensures that the UUID does not change on request retry. That may then also require additional logic in the REST controller to ensure that the UUID does not change.

I've looked into the Parse JS SDK and it seems that it does not have an auto-retry mechanism on failed requests, nor does XHR. For future reference: If such a mechanism ever gets implemented, similar to how it exists in the iOS and Android SDKs, this test case will likely need to be adapted to properly test that the UUID does not change.

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Can we add a test where we ensure that the request ID header is not sent for GET request?

@dplewis
Copy link
Member Author

dplewis commented Aug 20, 2020

@mtrezza I've added additional test. There is an auto-retry strategy in the SDK (in RESTController.js) I added a fix for that also. This is ready for review

@dplewis dplewis requested a review from mtrezza August 20, 2020 15:06
@mtrezza
Copy link
Member

mtrezza commented Aug 20, 2020

Great, I will take a look later today.

@dplewis
Copy link
Member Author

dplewis commented Aug 28, 2020

@mtrezza @davimacedo How does this look?

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

Good job @dplewis. It looks good to me.

@dplewis dplewis merged commit b2848a5 into master Aug 31, 2020
@dplewis dplewis deleted the idempotency branch August 31, 2020 21:58
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

It's already merged but I reviewed it anyway just to be sure and it looks good. Thanks for this PR!

@dplewis
Copy link
Member Author

dplewis commented Mar 14, 2021

@mtrezza I didn’t want to open a new issue, I’m writing documentation for this. How would you catch a duplicate request error?

try {
  await Parse.Cloud.run('duplicate')
  // code here would run for the first request
} catch(e) {
 // duplicate request here
}

@mtrezza
Copy link
Member

mtrezza commented Mar 14, 2021

@dplewis Yes, in addition the catch block would check for ParseError.DUPLICATE_REQUEST 159, see https://github.com/parse-community/Parse-SDK-JS/pull/1189/files.

The comment is not correct in all cases, it could "run for the first time" but end up in the catch block for another reason than duplicate request. So you could add "runs for first time and succeeds" in try block and in the catch block "runs for first times and failed" if error code is not 159, and "runs multiple times as duplicate requests" if error code is 159.

@dplewis
Copy link
Member Author

dplewis commented Mar 14, 2021

Can you think of a way to replicate a duplicate request in a test environment?

try {
  const value = await Parse.Cloud.run('duplicate');
  // do something with value
} catch(e) {
  if (e.code === Parse.Error.DUPLICATE_REQUEST) {
   // duplicate request here
  }
  // handle other errors
}

Edit: I can just create a custom RESTController

@mtrezza
Copy link
Member

mtrezza commented Mar 14, 2021

Sure, just send an identical request ID in the header and the request should fail as duplicate request. To send an identical request ID you could spyOn the request ID and fake it to make it the same in every request. If you look at the existing test cases for idempotency, they show exactly that. Let me know if you need more details, then I'll look into the JS SDK.

@dplewis
Copy link
Member Author

dplewis commented Mar 14, 2021

The tests cases are good, I was referring to a real environment. Website, press a button and send a duplicate request.
I just copied the RESTController, set a static requestId and CoreManager.setRESTController.

@mtrezza
Copy link
Member

mtrezza commented Mar 14, 2021

I was referring to a real environment. Website, press a button and send a duplicate request.

You mean as an example description in the docs or as a real coded test? I think I understand now, you mean testing this in a real environment, outside of this repo. Well, yes, you could set up a firewall with a rule that allows only requests client --> server and blocks responses client <-- server. The JS SDK (if it has a retry mechanism?) would retry a request after a timeout because it didn't receive an answer and the server would then respond with duplicate request. Note that is has to be a POST or PUT request.

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