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

fix: Don't send keys that are unchanged to database #7590

Closed
wants to merge 2 commits into from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Sep 23, 2021

New Pull Request Checklist

Issue Description

Currently, at the moment, keys are set to dirty, even if they are unchanged.

Related issue: #7591

Approach

Removes database ops for keys that are unchanged.

TODOs before merging

  • Add test cases
  • Add entry to changelog
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK
  • ...

@parse-github-assistant
Copy link

parse-github-assistant bot commented Sep 23, 2021

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@dblythy dblythy changed the title Fix: Don't send keys that are unchanged to database fix: Don't send keys that are unchanged to database Sep 23, 2021
@codecov
Copy link

codecov bot commented Sep 23, 2021

Codecov Report

Merging #7590 (08102f6) into master (0fa8f5e) will decrease coverage by 0.35%.
The diff coverage is 100.00%.

❗ Current head 08102f6 differs from pull request most recent head 0ed7c0a. Consider uploading reports for the commit 0ed7c0a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7590      +/-   ##
==========================================
- Coverage   93.94%   93.59%   -0.36%     
==========================================
  Files         181      181              
  Lines       13281    13284       +3     
==========================================
- Hits        12477    12433      -44     
- Misses        804      851      +47     
Impacted Files Coverage Δ
src/RestWrite.js 93.81% <100.00%> (-0.13%) ⬇️
src/Adapters/Cache/RedisCacheAdapter.js 12.28% <0.00%> (-75.44%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.47% <0.00%> (-0.65%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 95.28% <0.00%> (-0.08%) ⬇️
src/ParseServerRESTController.js 98.50% <0.00%> (+1.49%) ⬆️

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 0fa8f5e...0ed7c0a. Read the comment docs.

@mtrezza
Copy link
Member

mtrezza commented Sep 23, 2021

Could you please open an issue in Parse Server to discuss this?

Currently this is linked to a Parse Swift SDK issue. An SDK issue usually requires a PR in the respective SDK. This here is also confusing because the Swift issue is classified as a "question", not a "bug", but this PR is a "fix". A "question" shouldn't require a "fix". If this is a Parse Server issue, then it helps for clarity if we discuss this separately in the context of Parse Server.

@dblythy
Copy link
Member Author

dblythy commented Sep 23, 2021

No worries, related issue is #7591

@dblythy dblythy closed this Sep 23, 2021
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