fix(DataStore): support for QueryPredicateConstant.all for graphql queries

[wooj2]

Customers will experience a crash in the event they define a sync query with the .all constant. For example:

  let syncExpression1 = DataStoreSyncExpression.syncExpression(Todo.schema, where: {
      return QueryPredicateConstant.all
  }

This code change remedies this by using [:] as the filter value -- effectively disabling the selective sync expression.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[lawmicha]

are there other ways for the developer to get into this state?

1. Calling API directly and passing in the .all predicate- what is the expected behavior? [:] filter?
2. Calling DataStore save with a condition, does this get synced using API? is this even a valid use case?
3. Calling DataStore delete with a condition that is .all ? This is the other usage of .all right?
4. and i believe the syncExpression is the scenario that you have mentioned, if they configure DataStore with a selective sync, it would have crashed the app without this PR. Is it common to Selective sync with .all ? wouldn't they just not pass anything in to sync everything?

[codecov]

Codecov Report

Merging #1013 (03eb218) into main (f83d26e) will increase coverage by 3.78%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1013      +/-   ##
==========================================
+ Coverage   56.71%   60.50%   +3.78%     
==========================================
  Files         634      535      -99     
  Lines       18395    14650    -3745     
==========================================
- Hits        10433     8864    -1569     
+ Misses       7962     5786    -2176     

Flag	Coverage Δ
API_plugin_unit_test	47.97% <ø> (ø)
AWSPluginsCore	?
Amplify	45.85% <ø> (ø)
Analytics_plugin_unit_test	73.41% <ø> (ø)
Auth_plugin_unit_test	83.31% <ø> (ø)
DataStore_plugin_unit_test	70.97% <0.00%> (-0.04%)	⬇️
Predictions_plugin_unit_test	48.63% <ø> (+15.14%)	⬆️
Storage_plugin_unit_test	57.72% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ataStoreCategoryPlugin/Storage/StorageEngine.swift	50.44% <0.00%> (-0.31%)	⬇️
.../AWSPluginsCore/Auth/Provider/APIKeyProvider.swift
...sCore/Auth/Configuration/APIKeyConfiguration.swift
...ginsCore/Model/Decorator/PaginationDecorator.swift
...re/AWSPluginsCore/Model/Support/SelectionSet.swift
...ync/MutationSync/MutationSyncMetadata+Schema.swift
.../Configuration/CognitoUserPoolsConfiguration.swift
...Requests/PredictionsIdentifyRequest+Validate.swift
...luginsCore/Model/Decorator/AuthRuleDecorator.swift
...upport/Utils/ConvertSpeechToTextTransformers.swift
... and 87 more

---

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 223b51e...03eb218. Read the comment docs.

[Kilo-Loco]

are there other ways for the developer to get into this state?

1. Calling API directly and passing in the .all predicate- what is the expected behavior? [:] filter?
2. Calling DataStore save with a condition, does this get synced using API? is this even a valid use case?
3. Calling DataStore delete with a condition that is .all ? This is the other usage of .all right?
4. and i believe the syncExpression is the scenario that you have mentioned, if they configure DataStore with a selective sync, it would have crashed the app without this PR. Is it common to Selective sync with .all ? wouldn't they just not pass anything in to sync everything?

The requirement to return QueryPredicateConstant.all is to be able to write logic inside of the resolver block that checks against a nil value. My use case is as follows:

let syncExpression = DataStoreSyncExpression.syncExpression(TVSeries.schema) {
    let rating = UserDefaults.standard.integer(forKey: "preferredRating")
    guard rating > 0 else {
        return QueryPredicateConstant.all
    }
    return TVSeries.keys.rating >= 4
}
let config = DataStoreConfiguration.custom(syncExpressions: [syncExpression])

There's also an example of this kind of use case found in Android DataStore: Sync data to cloud docs

[wooj2]

Calling API directly and passing in the .all predicate- what is the expected behavior? [:] filter?

I believe passing in [:] is the correct behavior. We are effectively saying include "all", which i believe translates to not having a filter.

• Calling DataStore save with a condition, does this get synced using API? is this even a valid use case?

In the case of save, using the .all constant actually crashes because we are calling into the deprecated version of toJSON(_:options) (without the schema). We should update our code so that we are no longer calling the deprecated methods. I can do this, but prefer not to do that as part of this PR.

Calling DataStore delete with a condition that is .all ? This is the other usage of .all right?

Delete with predicate on a model type is currently not exposed to customer. we expose delete(_ model: M, modelSchema:where:completion), but completely ignore the predicate we pass in.

and i believe the syncExpression is the scenario that you have mentioned, if they configure DataStore with a selective sync, it would have crashed the app without this PR. Is it common to Selective sync with .all ? wouldn't they just not pass anything in to sync everything?

See @Kilo-Loco 's response

[lawmicha]

In the case of save, using the .all constant actually crashes because we are calling into the deprecated version of toJSON(_:options) (without the schema). We should update our code so that we are no longer calling the deprecated methods. I can do this, but prefer not to do that as part of this PR.

nice catch, opened #1014

[lawmicha]

Delete with predicate on a model type is currently not exposed to customer. we expose delete(_ model: M, modelSchema:where:completion), but completely ignore the predicate we pass in.

Unless this is implemented, it seems like it should be crashing the app during development time if predicate is passed in for delete call so developers are not confused that it's not yet implemented. Thanks for checking, opening issue #1015

[lawmicha]

Thanks for clarifying @Kilo-Loco @wooj2

[lawmicha]

just thinking whether the return value should just be nil and the method return GraphQLFilter? type. I think keeping it GraphQLFilter is fine since FilterDecorator should be checking if the filter is empty anyways, just to be safe.

[wooj2]

Hmm I think it would be useful to change if we had a use case of detecting nil vs an empty GraphQLFilter. In this case, of QueryPredicateConstant.all, i think that returning [:] is an accurate representation of .all -- that is, there is no filter -- so at this point, i'm not seeing a strong case for changing this to an optional.