search

parse-community / Parse-SDK-Flutter

The Dart/Flutter SDK for Parse Platform
https://parseplatform.org
Apache License 2.0
575 stars 190 forks source link

feat: Add `saveEventually` and `deleteEventually` in `ParseObject` #911

Closed mbfakourii closed 1 year ago

mbfakourii commented 1 year ago

Pull Request

Issue

Closes: #149 and #194

Approach

In this feature, Functions saveEventually and deleteEventually in ParseObject were added

---> in saveEventually At first, normal save command is entered in parseObject, if a NetworkError is encountered, this object is saved in a list with a key with the help of ParseCoreData. In our collection, we have a list of objects in ParseCoreData that are in NetworkError.

---> in deleteEventually At first, normal delete command is entered in parseObject, if a NetworkError is encountered, this object is saved in a list with a key with the help of ParseCoreData. In our collection, we have a list of objects in ParseCoreData that are in NetworkError.

---> in await ParseObject.submitEventually() With the help of the function await ParseObject.submitEventually() (which is debatable how to use it) we checks the error objcets

In this function, we first get the error parseObjects from ParseCoreData, then we send this list to _saveChildren. In this function, these objects are sent to the server in the form of chunks, and finally, we empty the ParseCoreData list.

In the following, we also take the list of parseObjects that must be deleted from ParseCoreData and try to delete them, and then we delete the successfully deleted parseObjects from the ParseCoreData list.

@parse-community/flutter-sdk-review Please check these two functions thoroughly and give suggestions, this PR needs discussion. and please check the algorithm, is this method good?

Tasks

parse-github-assistant[bot] commented 1 year ago

I will reformat the title to use the proper commit message syntax.

parse-github-assistant[bot] commented 1 year ago

Thanks for opening this pull request!

codecov[bot] commented 1 year ago

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (21ce56f) 39.61% compared to head (638440b) 40.60%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #911 +/- ## ========================================== + Coverage 39.61% 40.60% +0.98% ========================================== Files 60 60 Lines 3337 3401 +64 ========================================== + Hits 1322 1381 +59 - Misses 2015 2020 +5 ``` | [Files](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/911?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community) | Coverage Δ | | |---|---|---| | [packages/dart/lib/parse\_server\_sdk.dart](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/911?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-cGFja2FnZXMvZGFydC9saWIvcGFyc2Vfc2VydmVyX3Nkay5kYXJ0) | `31.25% <50.00%> (+2.67%)` | :arrow_up: | | [...ackages/dart/lib/src/network/parse\_dio\_client.dart](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/911?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-cGFja2FnZXMvZGFydC9saWIvc3JjL25ldHdvcmsvcGFyc2VfZGlvX2NsaWVudC5kYXJ0) | `0.00% <0.00%> (ø)` | | | [...ckages/dart/lib/src/network/parse\_http\_client.dart](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/911?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-cGFja2FnZXMvZGFydC9saWIvc3JjL25ldHdvcmsvcGFyc2VfaHR0cF9jbGllbnQuZGFydA==) | `5.06% <0.00%> (-0.07%)` | :arrow_down: | | [packages/dart/lib/src/utils/parse\_utils.dart](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/911?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-cGFja2FnZXMvZGFydC9saWIvc3JjL3V0aWxzL3BhcnNlX3V0aWxzLmRhcnQ=) | `76.27% <75.00%> (-0.20%)` | :arrow_down: | | [packages/dart/lib/src/objects/parse\_object.dart](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/911?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-cGFja2FnZXMvZGFydC9saWIvc3JjL29iamVjdHMvcGFyc2Vfb2JqZWN0LmRhcnQ=) | `94.49% <92.85%> (-0.42%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/911/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mtrezza commented 1 year ago

Please check these two functions thoroughly and give suggestions, this PR needs discussion.

Changed to draft

mbfakourii commented 1 year ago

@mtrezza @Nidal-Bakir Do you think the implemented method is appropriate?

Nidal-Bakir commented 1 year ago

I suggest looking at what other SDKs do and doing the same. There is the concept of a Queue so the objects get saved in order.

Do other SDKs save the unsaved Objects even when the app is closed(destroyed) using native services (android and iOS) when the connection is restored for example?

mbfakourii commented 1 year ago

I suggest looking at what other SDKs do and doing the same. There is the concept of a Queue so the objects get saved in order.

Do other SDKs save the unsaved Objects even when the app is closed(destroyed) using native services (android and iOS) when the connection is restored for example?

I think the queue issue happens in dio or Http and we don't need to create this queue.

According to the ios documentation

saveEventually

Saves this object to the server at some unspecified time in the future, even if Parse is currently inaccessible.

Use this when you may not have a solid network connection, and don’t need to know when the save completes. If there is some problem with the object such that it can’t be saved, it will be silently discarded.

Objects saved with this method will be stored locally in an on-disk cache until they can be delivered to Parse. They will be sent immediately if possible. Otherwise, they will be sent the next time a network connection is available. Objects saved this way will persist even after the app is closed, in which case they will be sent the next time the app is opened. If more than 10MB of data is waiting to be sent, subsequent calls to -saveEventually will cause old saves to be silently discarded until the connection can be re-established, and the queued objects can be saved.

Based on what I understand, the internal structure of Android is implemented with this concept. If possible, take a look at the Android structure.

The concept that I didn't understand was where to double check and send the saved it objects?

I wrote the codes of this section in the await ParseObject.submitEventually() function! And I allow the user to use it wherever they need. But not sure about that !

mbfakourii commented 1 year ago

Overall everything is good.

I have some thoughts:

  • What will happen if the current user logs out?

    • should we clear the cache? or could you give the end user the option to decide what should happen?
  • should we give the end user control over the cached objects that need to be saved/deleted?

At the moment, the caches are not deleted, but I think a function should be written to delete the cache

Regarding the second question, I did not see anything in other SDKs. Do you have any suggestions for this? What control do you mean?

Nidal-Bakir commented 1 year ago

At the moment, the caches are not deleted, but I think a function should be written to delete the cache

Got it.

Regarding the second question, I did not see anything in other SDKs. Do you have any suggestions for this? What control do you mean?

I mean what if the user wants to clear the currently cached objects for saving/deleting and start over? Should we expose such a feature?

Or in general, should we expose the current cache state so the end user can alter it?

If yes we should consider a saving mechanism like functions to be invoked. for example:

something like that

mbfakourii commented 1 year ago

I mean what if the user wants to clear the currently cached objects for saving/deleting and start over? Should we expose such a feature?

Or in general, should we expose the current cache state so the end user can alter it?

If yes we should consider a saving mechanism like functions to be invoked. for example:

  • onBeforStartSaving()
  • shouldSave()
  • shouldDelete()
  • onBeforSave()
  • onBeforDelete()
  • onAfterSave()
  • onAfterDelete()
  • onAllSaved()
  • onAllDeleted()

something like that

I don't think this is necessary. I haven't seen anything about this in other SDKs. Do you have a reason why it needs to be added?

Nidal-Bakir commented 1 year ago

Do you have a reason why it needs to be added?

No, I'm just wounding if we need to give the end user finer control over the eventual save in general. If the other SDKs do not do that we are good.

So currently, we need to discuss how the submitEventually function should be called and what will happen when the user logs out when there are some unsaved objects.

mbfakourii commented 1 year ago

Do you have a reason why it needs to be added?

No, I'm just wounding if we need to give the end user finer control over the eventual save in general. If the other SDKs do not do that we are good.

So currently, we need to discuss how the submitEventually function should be called and what will happen when the user logs out when there are some unsaved objects.

Do you think we should leave this function to the user or place it somewhere like the save() function?

I don't think this function has been given to the user in other SDKs!

mtrezza commented 1 year ago

What's missing in this PR to get it merged?

mbfakourii commented 1 year ago

What's missing in this PR to get it merged?

The code is finished, the tests must be written

mbfakourii commented 1 year ago

The error related to Test Flutter beta has been resolved in this PR

mbfakourii commented 1 year ago

@mtrezza

Done 😃

mtrezza commented 1 year ago

Note for review: The Flutter test is failing, hence we need to merge https://github.com/parse-community/Parse-SDK-Flutter/pull/969 first before merging this PR.

mbfakourii commented 1 year ago

Could you check whether I've resolved the conflicts correctly? Then I believe this can be merged.

I checked, and there was no problem.