feat(datasync): add conflicts by version by ssd71 · Pull Request #1833 · aerogear/graphback

ssd71 · 2020-08-07T10:59:19Z

Rather hopeful about this one 🤞

const { typeDefs, resolvers, contextCreator } = createDataSync(modelDefs, db, { Comment: { enabled: true, conflictResolution: serverSideConflictResolution }});

TODOS

Agreement on if this is how it should be
Delete Conflicts?
Tests
Docs

wtrocki · 2020-08-07T11:03:28Z

    });

-    if (this.config.useVersion) {
+    if (this.config?.modelUsesVersion[model.graphqlType.name] || false) {


move to variable

wtrocki · 2020-08-07T11:04:39Z

+    const { _id,  ...delta } = updateDocument;
+    const version = delta[DataSyncFieldNames.version];
+    // eslint-disable-next-line @typescript-eslint/tslint/config
+    delete delta[DataSyncFieldNames.version];delete delta[DataSyncFieldNames.lastUpdatedAt];delete delta[DataSyncFieldNames.deleted]; 


Why deleting those fields... and why in single line :)

Deleting so as to ensure only the fields changed by the user are kept in the diff

wtrocki · 2020-08-07T11:15:20Z

+  }
+
+  // returns final resolved update document
+  public async resolveConflictFromDelta(updateDocument: any): Promise<any> {


Resolving conflict algorithm for dummies.

1. Accept client side version form client and validate it. 2. Look for the delta table entry with id and version. 3. This entry will be our `base` for conflict resolution. 4. Fetch server side entry 5. Calculate diff for server side entry 5. Perform conflict check (check if fields that were changed are mergeable based on the diffs) 6. if mergable perform merge 7. If not mergable (the same fields changed - perform predefined conflict resolution. 8. Call out resolution algorithm (user provided function or default ones we provide) We can use `MostRecentWins strategy` - that is why do not remove last updated :) 9. try to save result to database - conditial check if version did not changed since then 7. Return data to user. Another condition - if items reached their time (TTL ;) we have no way to kick offix compatible error to user.

All of this logic is already implemented in:
https://github.com/aerogear/offix/tree/master/packages/offix-conflicts-client

See also offix conflict server. Obviously you are free to invent your own algorithm but based on what I see I will probably prefer us to copy paste the code that was written and used in production rather than try to invent new way :)

wtrocki · 2020-08-07T11:15:54Z

+    dataProviderCreator: createDataSyncConflictProviderCreator(db, dataSyncConfig),
+    plugins:[
+      ...graphbackConfig.plugins || [],
+      new DataSyncPlugin({ modelUsesVersion: Object.keys(dataSyncConfig).reduce((versionMap: any, modelName: string) => {


Separate variable?

Actually why we do not do this inside plugin. It feels wrong to do it here.

+1 it should work the same if a user wants to use buildGraphbackAPI directly instead of createDataSync

wtrocki · 2020-08-07T11:18:09Z

+export class MongoDeltaHelper {
+  protected collection: Collection;
+  public constructor(model: ModelDefinition, db: Db) {
+    this.collection = db.collection(getDeltaTableName(getTableName(model.graphqlType)));


Extract variable

wtrocki · 2020-08-07T11:20:04Z

+  public async update(updateDocument: any, context: GraphbackContext): Promise<Type> {
+    const { _id } = updateDocument;
+
+    const MAX_RETRIES = 3;


Probably not the best place to setup this value.

wtrocki · 2020-08-07T11:21:14Z

+
+
+        if (updateDocument[DataSyncFieldNames.version] !== serverStateData[DataSyncFieldNames.version]) {
+          const conflict: ConflictStateMap = {


This doesn't mean conflict.
It means only that versions are different.
We need diffs to know if conflict occured.

Server data might have different fields than client as well.

craicoverflow · 2020-08-10T09:42:39Z

    });

-    if (this.config.useVersion) {
+    const modelUsesVersion = !!this.config.modelConfigMap[model.graphqlType.name]?.enabled;


I don't think a double-negation is required here.

Suggested change

const modelUsesVersion = !!this.config.modelConfigMap[model.graphqlType.name]?.enabled;

const modelUsesVersion = this.config.modelConfigMap[model.graphqlType.name]?.enabled;

Hmm wouldn't that make the expression true when the rest of the statement is undefined?

My bad, what I mean to say I don't think there is a need for any negation here.

machi1990

Done a quick review and I've added my comments inline. Let me know what you think.

craicoverflow · 2020-08-10T09:46:52Z

+/**
+ * M
+ */
+export class MongoDeltaHelper {


a helper class is used to assist in providing some functionality, which isn't the main goal of the application or class in which it is used.

I think the name MongoDeltaHelper is too generic/non-specific. It is in fact a very integral class providing a very specific purpose. I see a helper class as being a collection of methods which do not necessarily work together or have common purpose.

craicoverflow · 2020-08-10T09:52:43Z

+    this.collectionName = getDeltaTableName(getTableName(model.graphqlType));
+  }
+
+  public async insertDiff(updatedDocument: any) {


We could bring better type support to this class with generics:

export class MongoDeltaHelper<T = any> ... public async insertDiff(updatedDocument: T) {

craicoverflow · 2020-08-10T10:15:37Z

+
+type DataSyncGraphbackConfig = Partial<GraphbackConfig>
+
+export function createDataSyncAPI(model: string | GraphQLSchema, db: Db, dataSyncConfigMap: { [modelName: string]: DataSyncModelConflictConfig} = {}, graphbackConfig: DataSyncGraphbackConfig = {}): GraphbackAPI {


I think 5 arguments is too complicated for the user API, what do you think about reducing it to max 2?

DataSyncGraphbackConfig can copy the required config props (crud, plugins) from GraphbackAPIConfig. This means very minimal repetition, but greatly simplifies user experience, which is paramount IMO.

export function createDataSyncAPI(model: string | GraphQLSchema, config: DataSyncGraphbackConfig): GraphbackAPI {

Thoughts? Do you see any complications that might arise from this? cc @machi1990

craicoverflow · 2020-08-10T10:17:30Z

+import { createDataSyncCRUDService } from '../services';
+import { DataSyncPlugin } from '../DataSyncPlugin';
+
+type DataSyncGraphbackConfig = Partial<GraphbackConfig>


GraphbackConfig is not used in runtime API (it is used for the generate command. I think you are looking for GraphbackAPIConfig?

craicoverflow · 2020-08-10T10:18:33Z

+export function createDataSyncAPI(model: string | GraphQLSchema, db: Db, dataSyncConfigMap: { [modelName: string]: DataSyncModelConflictConfig} = {}, graphbackConfig: DataSyncGraphbackConfig = {}): GraphbackAPI {
+
+  return buildGraphbackAPI(model, {
+    serviceCreator: createDataSyncCRUDService({ pubSub: new PubSub() }),


What if user wants to use their own PubSub library?

serviceCreator should still be an (optional) option in the API config, as the user may want to customise/extend it.

craicoverflow · 2020-08-10T10:20:28Z

+export const MAX_RETRIES = 3;
+
+/**
+ * Da


Requires API docs

craicoverflow · 2020-08-10T10:21:49Z

+    return result;
+  }
+
+  public async update(updateDocument: any, context: GraphbackContext): Promise<Type> {


Suggested change

public async update(updateDocument: any, context: GraphbackContext): Promise<Type> {

public async update(updateDocument: Partial<Type>, context: GraphbackContext): Promise<Type> {

Should be done

craicoverflow · 2020-08-10T10:27:47Z

+
+      const serverData = await this.db.collection(this.collectionName).findOne({ _id, [DataSyncFieldNames.deleted]: false});
+      const base = await this.deltaHelper.findBaseForConflicts(updateDocument);
+      const clientData = updateDocument;


Why use this separate variable which references updateDocument? I don't see updateDocument being modified after this (unless I missed it)

wtrocki · 2020-08-12T13:58:42Z

+  },
+  resolveDelete(conflict: ConflictMetadata): any {
+
+    throw new ConflictError(conflict);


We might need to know what operation thrown conflict (in case someone will start batching requests on client)

Separate issue probably?

wtrocki · 2020-08-12T14:02:46Z

This looks very very good.
I would merge this in order to start playing with this in other project but we need more integration tests.

Co-authored-by: Enda <ephelan@redhat.com> Co-authored-by: Manyanda Chitimbo <manyanda.chitimbo@gmail.com>

…leted documents

ssd71 · 2020-08-12T15:53:14Z

Fixed clientWins strategy to force deletes as well as added some more integration tests for deletion

wtrocki · 2020-08-12T16:07:57Z

+      }
+    }
+  }});
+  console.log(errors);


Probably worth to assert this to be empty. :)

ssd71 requested review from craicoverflow, machi1990 and wtrocki August 7, 2020 10:59

ssd71 force-pushed the datasync-conflicts branch from 6b69d34 to 3cff93f Compare August 7, 2020 11:00

wtrocki reviewed Aug 7, 2020

View reviewed changes

wtrocki approved these changes Aug 7, 2020

View reviewed changes

craicoverflow reviewed Aug 7, 2020

View reviewed changes

Comment thread packages/graphback-datasync/src/DataSyncPlugin.ts Outdated

craicoverflow reviewed Aug 7, 2020

View reviewed changes

Comment thread packages/graphback-datasync/src/helpers/createDataSync.ts Outdated

ssd71 force-pushed the datasync-conflicts branch 2 times, most recently from 9c2724a to 601fe9f Compare August 10, 2020 07:02

ssd71 requested review from craicoverflow and wtrocki August 10, 2020 07:09