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: Setting atomic operation on Parse Object returns operation instead of prospective value #860

Merged
merged 74 commits into from May 13, 2023

Conversation

Nidal-Bakir
Copy link
Member

@Nidal-Bakir Nidal-Bakir commented Mar 19, 2023

New Pull Request Checklist

Issue Description

Closes: #843, #842, #834, #696

Approach

Create a new Class/DataType to handle any parse operation

  • setAdd
  • setAddUnique
  • setAddAll
  • setAddAllUnique
  • setRemove
  • setRemoveAll
  • setIncrement
  • setDecrement
  • addRelation
  • removeRelation

Why does this PR fixes all these issues?
Well, they all suffer from the same bug (using Maps to store operations). Because every operation needs to be in a new object and the old way of using maps should be removed, keeping both and fixing them one by one could create unexpected behaviors because we would need to use both of them while fixing the bugs.

TODOs before merging

  • Add tests
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • A changelog entry

@parse-github-assistant
Copy link

parse-github-assistant bot commented Mar 19, 2023

Thanks for opening this pull request!

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

@codecov
Copy link

codecov bot commented Mar 19, 2023

Codecov Report

Patch coverage: 93.75% and project coverage change: +11.98 🎉

Comparison is base (2e133ff) 26.71% compared to head (bf8b111) 38.69%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #860       +/-   ##
===========================================
+ Coverage   26.71%   38.69%   +11.98%     
===========================================
  Files          47       56        +9     
  Lines        2905     3277      +372     
===========================================
+ Hits          776     1268      +492     
+ Misses       2129     2009      -120     
Impacted Files Coverage Δ
packages/dart/lib/parse_server_sdk.dart 28.57% <ø> (ø)
packages/dart/lib/src/objects/parse_exception.dart 21.42% <21.42%> (ø)
packages/dart/lib/src/utils/parse_decoder.dart 84.48% <80.00%> (+10.89%) ⬆️
packages/dart/lib/src/utils/parse_encoder.dart 71.79% <90.90%> (+5.12%) ⬆️
...b/src/objects/parse_operation/parse_operation.dart 91.00% <91.00%> (ø)
packages/dart/lib/src/objects/parse_relation.dart 91.66% <92.63%> (+37.49%) ⬆️
packages/dart/lib/src/objects/parse_base.dart 89.55% <93.75%> (+36.04%) ⬆️
...rse_operation/parse_remove_relation_operation.dart 94.11% <94.11%> (ø)
packages/dart/lib/src/objects/parse_object.dart 95.57% <97.50%> (+4.89%) ⬆️
packages/dart/lib/src/network/parse_query.dart 26.56% <100.00%> (+2.25%) ⬆️
... and 10 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Maybe this will pass the dry run check. Added "Unreleased" instead of the release date to indicate that it's not an actual release.

packages/dart/CHANGELOG.md Outdated Show resolved Hide resolved
@mtrezza
Copy link
Member

mtrezza commented May 9, 2023

@Nidal-Bakir In my understanding #867 is the only other breaking PR we need to merge to make the 5.0.0 release. @mbfakourii Would you agree?

@mbfakourii
Copy link
Member

mbfakourii commented May 9, 2023

@Nidal-Bakir In my understanding #867 is the only other breaking PR we need to merge to make the 5.0.0 release. @mbfakourii Would you agree?

Yes

@mtrezza
Copy link
Member

mtrezza commented May 9, 2023

Alright, CI passes, so we can merge this now?

@Nidal-Bakir
Copy link
Member Author

Alright, CI passes, so we can merge this now?

Sure

mtrezza
mtrezza previously approved these changes May 13, 2023
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!

@mtrezza
Copy link
Member

mtrezza commented May 13, 2023

@parse-community/flutter-sdk-review Is this ready for merge?

@Nidal-Bakir
Copy link
Member Author

We are ready to release Dart 5.0.0
Let us wait for the CI

@Nidal-Bakir
Copy link
Member Author

Alright, everything is good. lets move to the Flutter package

@juriSacchetta
Copy link
Contributor

juriSacchetta commented May 15, 2023

Hello everyone!

Firstly, I would like to express my gratitude to @Nidal-Bakir for the tremendous work done!

On another note, I'm currently in the process of migrating to the latest release, and I've encountered two issues:

While fetching objects from the database, their relations are automatically resolved. Although this can be a valuable feature, I have multiple bidirectional ties among objects, leading to a stack overflow exception. Is there a way to disable this feature? Could you provide me with a workaround or suggest a more suitable approach?

The second problem pertains to ParseRelation. I'm experiencing an issue that has been present in previous versions as well. As far as I understand, when an object is fetched from the database, it brings along the reference of the relationship. However, when the relation is parsed, it is created as a normal ParseRelation<ParseObject>. For instance, let's say I fetch a post, and the Dart object contains a ParseRelation<Like> called "likes." When I attempt to use this relation, I encounter the following error: The key Likes is associated with a value (Instance of '_ParseRelation<ParseObject>') and cannot be a relation.

Could you please assist me in resolving these concerns? Thank you!

@Nidal-Bakir
Copy link
Member Author

Nidal-Bakir commented May 15, 2023

Hay @juriSacchetta, Thank you for your kind words I appreciate your feedback.

Please open an issue with your bug.

Thanks

@catalunha
Copy link

Really @Nidal-Bakir your work has been invaluable in refining such an important backend tool for Dart. Congratulations and may God continue to give you knowledge and strategy.

@catalunha
Copy link

If I understood your problem correctly @juriSacchetta automatically we only have complete data when it is a Pointer and we ask for it via includeObject. For objects in Relation fields we have to request the list separately via another manual query.

@juriSacchetta
Copy link
Contributor

Hello @Nidal-Bakir and @catalunha,

Thank you for your responses.

Upon further examination of my code, I have realized that the "stack overflow" error is caused by the usage of the jsonencode method instead of toString in ParseBase. I mistakenly attributed this error solely to the library version change, and I apologize for the confusion. However, I find it rather perplexing that such an error arises solely from the library version update. Can you offer any insights or suggestions as to why this might be happening?

Regarding the other problem, I will proceed to open an issue for it.

Thank you once again for your assistance.

@maravilhosinga
Copy link

From this version there is a bug causing error in server side when doing some queries:

This is the error:

SyntaxError: Unexpected token } in JSON at position 335 at JSON.parse (<anonymous>) at ClassesRouter.handleFind (/home/msinga/teego_server_devs/node_modules/parse-server/lib/Routers/ClassesRouter.js:29:25) at /home/msinga/teego_server_devs/node_modules/parse-server/lib/Routers/ClassesRouter.js:162:19 at /home/msinga/teego_server_devs/node_modules/parse-server/lib/PromiseRouter.js:153:7 at Layer.handle [as handle_request] (/home/msinga/teego_server_devs/node_modules/express/lib/router/layer.js:95:5) at next (/home/msinga/teego_server_devs/node_modules/express/lib/router/route.js:144:13) at Route.dispatch (/home/msinga/teego_server_devs/node_modules/express/lib/router/route.js:114:3) at Layer.handle [as handle_request] (/home/msinga/teego_server_devs/node_modules/express/lib/router/layer.js:95:5) at /home/msinga/teego_server_devs/node_modules/express/lib/router/index.js:284:15 at param (/home/msinga/teego_server_devs/node_modules/express/lib/router/index.js:365:14) SyntaxError: Unexpected token } in JSON at position 335 at JSON.parse (<anonymous>) at ClassesRouter.handleFind (/home/msinga/teego_server_devs/node_modules/parse-server/lib/Routers/ClassesRouter.js:29:25) at /home/msinga/teego_server_devs/node_modules/parse-server/lib/Routers/ClassesRouter.js:162:19 at /home/msinga/teego_server_devs/node_modules/parse-server/lib/PromiseRouter.js:153:7 at Layer.handle [as handle_request] (/home/msinga/teego_server_devs/node_modules/express/lib/router/layer.js:95:5) at next (/home/msinga/teego_server_devs/node_modules/express/lib/router/route.js:144:13) at Route.dispatch (/home/msinga/teego_server_devs/node_modules/express/lib/router/route.js:114:3) at Layer.handle [as handle_request] (/home/msinga/teego_server_devs/node_modules/express/lib/router/layer.js:95:5) at /home/msinga/teego_server_devs/node_modules/express/lib/router/index.js:284:15 at param (/home/msinga/teego_server_devs/node_modules/express/lib/router/index.js:365:14)

@Nidal-Bakir
Copy link
Member Author

@maravilhosinga

Please open an issue with your bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:breaking Breaking change requires major version increment and `BREAKING CHANGE` commit message
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incrementing or Decrementing existing value for the first time will remove it
6 participants