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

feat: Enable Parse.idempotency by default #2164

Closed
wants to merge 3 commits into from

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Jun 7, 2024

Pull Request

Issue

There isn't a well documented way to turn idempotency on, enabling it by default only requires configuration on the server side. This is how it is handled in other SDKs

parse-community/Parse-Swift#62
parse-community/Parse-SDK-iOS-OSX#1790
parse-community/Parse-SDK-Android#1190

Approach

  • Remove check for POST / PUT request method as they are handled by servers
  • Add requestId to every request by default

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)

Copy link

parse-github-assistant bot commented Jun 7, 2024

Thanks for opening this pull request!

  • ❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Closes: #123 in the PR description, so I can recognize it.

Copy link

codecov bot commented Jun 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (df6df7c) to head (6ac20ca).
Report is 1 commits behind head on alpha.

Additional details and impacted files
@@            Coverage Diff            @@
##             alpha     #2164   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           64        64           
  Lines         6364      6363    -1     
  Branches      1507      1509    +2     
=========================================
- Hits          6364      6363    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dplewis dplewis requested a review from a team June 7, 2024 16:24
@mtrezza
Copy link
Member

mtrezza commented Jun 8, 2024

  • This would likely be a breaking change for the JS SDK, since it changes the request headers.
  • What's the purpose of turning this on by default? Is that on be default in the other client SDKs?

@dplewis
Copy link
Member Author

dplewis commented Jun 8, 2024

This should have been enabled from the beginning. This is the default behavior of the other SDKs as I mentioned in the OP.

I wouldn't consider this a breaking change as wouldn't cause any disruptions to the developers. Adding this request header by default is pretty much useless unless the feature is enabled on the server. If it’s enabled on the server already it’s likely enabled in the SDK

A breaking change would be removing the ability to turn it off.

@mtrezza
Copy link
Member

mtrezza commented Jun 8, 2024

I'm not sure it should be on by default. That's at least not what I had in mind when I designed the feature. It's costly on server side resources when it's enabled on the server and a minority of users is likely using it. Sending a UID by default from the client to the server without the server using it would not be good practice because that generates unnecessary costs.

It could be breaking considering that we change the request headers. An additional header may cause a request to be rejected on tightly configured firewalls. It's like changing a request HTTP method, that would also be a breaking change for the same reason.

@dplewis dplewis closed this Jun 8, 2024
@dplewis dplewis deleted the idempotency-enabled branch June 8, 2024 22:16
@dplewis
Copy link
Member Author

dplewis commented Jun 9, 2024

@mtrezza Im closing this as it works for me. From a client side cost side we should purge the other SDKs headers as well from the parse.com days. Can you add documentation so developers can enable this feature since designed it?

@mtrezza
Copy link
Member

mtrezza commented Jun 10, 2024

I agree that other client SDKs should also not send the idempotency header by default. For docs see here.

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.

2 participants