Skip to content

add DataStore configuration API #369

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

Closed
wants to merge 3 commits into from
Closed

Conversation

drochetti
Copy link
Contributor

@drochetti drochetti commented Apr 1, 2020

DataStore Configuration

DataStore requires some configuration. This API is heavily based and aligned with the current JavaScript implementation: https://aws-amplify.github.io/docs/js/datastore#config.

Developer Experience

Default:

// default configuration can be omitted
Amplify.add(plugin: AWSDataStorePlugin(schema: schema, configuration: .default))

Custom - only required option:

// custom configuration (only conflictHandler is required)
Amplify.add(plugin: AWSDataStorePlugin(schema: schema, configuration: .custom(
    conflictHandler: { (data, resolve) in
        let (local, remote) = data
        // implement conflict resolution logic
        // then call the resolve callback with the result
        resolve(.new(mergedModel))
    }
)))

Custom - all options (and their respective defaults):

// custom configuration with all options
Amplify.add(plugin: AWSDataStorePlugin(schema: schema, configuration: .custom(
    conflictHandler: { (data, resolve) in
        .resolve(.discard) // same as .resolve(.new(data.remote))
    },
    errorHandler: { error in
        Amplify.Logging.error(error: error)
    },
    syncInterval: 1_440,
    syncMaxRecords: 10_000,
    syncPageSize: 1_000
)))

Differences from the JavaScript implementation

Configuration options

The JavaScript have the same options, but the sync-related configuration have different names. We decided to slightly change the property names so they have a common prefix: sync*. The equivalent options would be:

JavaScript iOS
maxRecordsToSync syncMaxRecords
fullSyncInterval syncInterval
syncPageSize syncPageSize

Conflict Handler

The conflictHandler in JavaScript relies on Promises (async/await) to allow asynchronous code to be executed inside the callback. The solution in Swift was to pass a resolver function to the callback so once the resolution code is done, the resolver is invoked. The approach suits both synchronous and asynchronous needs within the conflict resolution logic.

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

@drochetti drochetti added the datastore Issues related to the DataStore category label Apr 1, 2020
@drochetti drochetti requested a review from wooj2 April 1, 2020 00:50
@@ -9,7 +9,7 @@ import Foundation

/// Protocol that defines a contract between the consumer and the DataStore plugin.
/// All models have to be registered and have an associated `version`.
public protocol AmplifyModelRegistration {
public protocol DataStoreSchema {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about SchemaRegistry or ModelSchema? Having "DataStore" in APIPlugin seems odd

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments about aligning naming --

For your consideration, on Android, we have two similar components which are named.

  1. The ModelProvider, and its code-generated AmplifyModelProvider (an example of one). This component is read-only and declares what the different types of models are, and a version for their contract, as a whole.
  2. The ModelSchemaRegistry, which accepts a collection of model classes as inputs, and then constructs and maintains ModelSchema for each, by parsing Java annotations and using Java reflection, on the @Model-annotated classes.

To recall, as above, Android generates the schema at runtime, not compile-time. So, there are some architectural differences. But we can probably keep the names a little tighter between the platforms even so, if we try.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You guys probably already realized this, but if we make this change, we'll need to update the codegen tools so that it generates:

final public class AmplifyModels: DataStoreSchema {

instead of what it currently geneates:

final public class AmplifyModels: AmplifyModelRegistration {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about SchemaRegistry or ModelSchema? Having "DataStore" in APIPlugin seems odd

I understand the concern, but we need to keep in mind that the model-based API integration should be revisited. Right now API imports classes and protocols from the DataStore category, so the integration is there already. We need to think of ways of making that more isolated.

That said, ModelSchema cannot be used because it's already used by Model, as the schema of each Model. That said, I'll think of a better name for it, SchemaRegistry could be one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments about aligning naming --

For your consideration, on Android, we have two similar components which are named.

  1. The ModelProvider, and its code-generated AmplifyModelProvider (an example of one). This component is read-only and declares what the different types of models are, and a version for their contract, as a whole.
  2. The ModelSchemaRegistry, which accepts a collection of model classes as inputs, and then constructs and maintains ModelSchema for each, by parsing Java annotations and using Java reflection, on the @Model-annotated classes.

To recall, as above, Android generates the schema at runtime, not compile-time. So, there are some architectural differences. But we can probably keep the names a little tighter between the platforms even so, if we try.

thanks @jamesonwilliams for the call-out. JavaScript simply calls it schema (using initSchema(schema: Schema). Before iOS was using ModelRegistration which is odd, so this is an attempt to align iOS more with JS. I'll revisit the Android and JS implementations to see if we can agree on something. I'll get in touch with you directly before I submit an update to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamesonwilliams @wooj2 @lawmicha I updated to ModelSchemaProvider. Jameson, do you think it's OK to rename ModelProvider to ModelSchemaProvider on your end?

Copy link

@jamesonwilliams jamesonwilliams Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I may have accidentally just created more confusion. :-(

On Android, the CLI code-gens an AmplifyModelProvider implements ModelProvider; the ModelProvider interface returns a Set<Class<? extends Model>>.

At runtime, we access that Set<Class<? extends Model>>. For each modelClass in that set, we establish a 1:1 relationship to a ModelSchema. We build the ModelSchemas in the library at run-time, by inspecting each Class<? extends Model> instance via reflection. We end up with a List<ModelSchema>, that is held in-memory in a
class called ModelSchemaRegistry.

ModelSchemaProvider might actually be a better name for that first interface. But, it would create confusion with some of our internal components, presently.

@jamesonwilliams jamesonwilliams self-requested a review April 9, 2020 22:27
Copy link

@jamesonwilliams jamesonwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A clean PR as always @drochetti. The PR description is particularly beautiful; thanks for the clear and concise summary.

Some thoughts inline.

Overall I think the configuration structure here is good. We can use most of these same names and concepts in the Android code.

@wooj2 tells me that JavaScript has provisions to rename their config fields, to the new syncFoo style?

Many time-based identifiers include units, due the frequent ambiguity around them. I would push us to change syncInterval should be syncIntervalMs.

The impact of the syncIntervalMs is fairly difficult to describe accurately and concisely in natural language. So, I am okay with calling it just a "sync interval," and offloading its definition into a paragraph of carefully worded documentation. I think there is some technical precision needed in that approach to defining it, however. Here's my stab.

When the DataStore initializes, it considers this value to determine what type of sync to perform. If the last base sync occurred longer ago than this amount of time, a new base sync will be performed. If the last base sync occurred before the current time, but after the current time minus the sync interval, then a delta sync will be performed.

@@ -9,7 +9,7 @@ import Foundation

/// Protocol that defines a contract between the consumer and the DataStore plugin.
/// All models have to be registered and have an associated `version`.
public protocol AmplifyModelRegistration {
public protocol DataStoreSchema {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments about aligning naming --

For your consideration, on Android, we have two similar components which are named.

  1. The ModelProvider, and its code-generated AmplifyModelProvider (an example of one). This component is read-only and declares what the different types of models are, and a version for their contract, as a whole.
  2. The ModelSchemaRegistry, which accepts a collection of model classes as inputs, and then constructs and maintains ModelSchema for each, by parsing Java annotations and using Java reflection, on the @Model-annotated classes.

To recall, as above, Android generates the schema at runtime, not compile-time. So, there are some architectural differences. But we can probably keep the names a little tighter between the platforms even so, if we try.

Comment on lines 45 to 49
/// A callback function called on unhandled errors
public let errorHandler: DataStoreErrorHandler

/// A callback called when a conflict could not be resolved by the service
public let conflictHandler: DataStoreConflictHandler

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR doesn't "wire" these into the code, right? We're just defining the attributes and usage of the DataStoreConfiguration, in this PR?

I think it looks good, if so.

We should layer these two handlers in some heft docs. One particular thing that might be worth noting the Apple doc is:

  1. The conflict handler runs synchronously, and will block (what actions) until complete?

I like the error handler. I like plain callbacks in general. But, why are we using an error handler callback, instead of broadcasting errors to Hub? Or, will we do both? Does the error handler work synchronously, and block other operations, until complete? If so, which ones? Can we document all of this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bringing this up, and all valid points. I've opened an internal ticket against us to complete this before GA.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wooj2 I think the doco could go right into this PR, no? The error events over Hub might be a good follow up task.

let schema: DataStoreSchema

/// The DataStore configuration
let configuration: DataStoreConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On line72, in the function configure(using configuration: Any), we pass in the amplify configuration (I think), so we might want to rename this to something like dataStoreConfiguration

@aws-amplify aws-amplify deleted a comment from drochetti Apr 16, 2020
public enum DataStoreConflictHandlerResult {

/// Discard the local changes in favor of the remote ones.
case discard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be very explicit: If this discards local changes in favor of remote ones, does that mean updating the system, at the time of the conflict resolution handler, with the state of the remote item? If so then let's document that. Better yet, maybe more appropriate names would be: applyRemote, retryLocal, and retry(Model)?

wooj2 added a commit that referenced this pull request Apr 21, 2020
wooj2 added a commit that referenced this pull request Apr 21, 2020
* add DataStore configuration API
* add documentation, fix formatting and add TimeInterval utilities
@wooj2
Copy link
Contributor

wooj2 commented Apr 21, 2020

We will keep this PR around FOR REFERENCE ONLY until we resolve:

  • AmplifyModelRegistration vs ModelSchemaProvider vs something else
  • Renaming discard vs applyRemote, etc.

@drochetti
Copy link
Contributor Author

This PR was used as a base for discussion and work was divided into other PRs. Closing this one.

@drochetti drochetti closed this May 20, 2020
@palpatim palpatim deleted the feature/datastore-config branch May 27, 2020 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datastore Issues related to the DataStore category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants