Add delta table by ssd71 · Pull Request #1647 · aerogear/graphback

ssd71 · 2020-07-03T06:06:00Z

This is a spike towards investigating Delta Tables for datasync, can be tested out as follows:

for delta Table

This is enabled in schema
Use the following schema with datasync template

""" 
@model
@datasync(deltaTable: true)
"""
type Comment {
  id: ID!
  text: String
  description: String
}

It should create a comment_delta collection in the database

conflict

Conflicts are enabled in code
Import TimestampConflictEngine from datasync package:

import { TimestampConflictEngine } from '@graphback/datasync'

Provide a map of model name to conflict strategy(pardon the horrendous inconsistency in the names, I would love some feedback about what to call them) in createDataSyncMongoDbProvider as follows:

createDataSyncMongoDbProvider(db, { Comment: TimestampConflictEngine })

This tells it to use Timestamp based conflicts for the Comment model, there is also another strategy intended for use with delta tables called DeltaConflictEngine, it checks if the fields to be updated have changed since the client passed updatedAt timestamp, and raises a conflict if they have, I would love some feedback about this as well.

Note, if using delta table, sync query only returns fields that have changed, not sure this is optimal so feedback needed

wtrocki · 2020-07-03T07:23:36Z

+    delete data.id;
+
+    const res = await this.db.collection(this.collectionName).insertOne({
+      doc: data


We might need more information here. Like what operation was executed etc.

Agreed 👍

wtrocki · 2020-07-03T07:36:13Z

+
+  public sync(lastSync: string, context: GraphbackContext, filter?: any): Promise<Type[]> {
+
+    return this.deltaProvider.sync(parseInt(lastSync, 10)).then((res: any[]) => {


So we went to the 1.5 - WE have separate delta table with duplicate of the data. (Not really delta table itself.)?

It is open for discussion, really, I just wanted get some feedback, I haven't hacked around with it that much to make a decisive move yet

yep. No worries. This works and it is very very well written. But we cannot get conflict information from it. Let's drop some short research to see how we can get that done

So we went to the 1.5 - WE have separate delta table with duplicate of the data. (Not really delta table itself.)?

This should be more of a changelog + auditing table. This article might be of help https://ludwigstuyck.wordpress.com/2013/04/04/history-tracking/

craicoverflow · 2020-07-03T08:36:52Z

+      this.deltaProvider.insertDiff(result.value).catch((e: any) => {
+        console.error(`Error: Couldn't insert into delta table: ${e}`);
+      })


This block in update, delete and create seem to be the only things that make this provider different to DatasyncMongoDBDataProvider.

Could this just extend that instead, to save repeating code in two providers and increasing maintenance costs.

Or would it be feasible to use the same provider, and only execute this block if this.deltaProvider has been created, which could be conditionally created based on the existence of the the deltaTable annotation key.

This got me thinking also - maybe we should be passing the ModelDefinition to the provider instead of the base GraphQL type, as a lot of the preparsed metadata information is not available from the base type and you would have to re-parse it.

This got me thinking also - maybe we should be passing the ModelDefinition to the provider instead of the base GraphQL type, as a lot of the preparsed metadata information is not available from the base type and you would have to re-parse it.

+1, this rejoins this comment #1614 (comment)

craicoverflow

I've left one comment: https://github.com/aerogear/graphback/pull/1647/files#r449454898

Is there a related issue for this PR (I know it is a spike, but maybe some background).

ssd71 · 2020-07-04T06:14:05Z

Hi @craicoverflow , sorry for the late response
Some background can be found on #1381 #1445
Sorry if there isn't much to go on, I definitely should have discussed about it before spiking 😛

ssd71 · 2020-07-08T05:37:35Z

I have implemented a more meaningful delta table where we only keep the changes
I am using this table for sync, as well as conflicts
Consolidated custom conflicts([WIP]Custom conflicts for DataSync #1591) into this

To test out with datasync template:

""" 
@model
@datasync(deltaTable: true)
"""
type Comment {
  id: ID!
  text: String
  description: String
}

and in index.ts:

import { createDataSyncCRUDService, DataSyncPlugin, createDataSyncMongoDbProvider, DeltaConflictEngine } from '@graphback/datasync';
...
const { typeDefs, resolvers, contextCreator } = buildGraphbackAPI(modelDefs, {
    dataProviderCreator: createDataSyncMongoDbProvider(db, { Comment: DeltaConflictEngine }),
    serviceCreator: createDataSyncCRUDService({ pubSub: new PubSub() }),
    plugins: [
      new DataSyncPlugin()
    ]
  });

craicoverflow

Tried to verify but I don't know how to use createDataSyncMongoDbProvider with the new conflictStrategyMap param. Could you update the PR description with step-by-step guide how to verify this?

craicoverflow · 2020-07-08T07:27:54Z

+  checkForConflicts(serverState: any, clientState: any): ConflictStateMap | undefined 
+
+  /**
+   * checkForConflicts


Suggested change

* checkForConflicts

* resolveConflicts

craicoverflow · 2020-07-08T07:28:54Z

+}
+
+/**
+ * d


Incomplete documentation

craicoverflow · 2020-07-08T07:29:22Z

+ * d
+ */
+export abstract class CustomConflictEngine implements ConflictEngine {
+  protected conflictFieldName: string = undefined;


Suggested change

protected conflictFieldName: string = undefined;

protected conflictFieldName: string;

This should be the same.

craicoverflow · 2020-07-08T07:32:06Z

+  protected conflictFieldName: string = undefined;
+
+  public checkForConflicts(serverState: any, clientState: any): ConflictStateMap {
+    if (serverState[this.conflictFieldName] !== undefined && clientState[this.conflictFieldName].toString() !== serverState[fieldNames.updatedAt].toString()) {


This statement is very long, wondering if we can make it smaller or else break it into a couple of lines

Suggested change

if (serverState[this.conflictFieldName] !== undefined && clientState[this.conflictFieldName].toString() !== serverState[fieldNames.updatedAt].toString()) {

if (serverState[this.conflictFieldName] && clientState[this.conflictFieldName].toString() !== serverState[fieldNames.updatedAt].toString()) {

craicoverflow · 2020-07-08T07:35:14Z

+export class DeltaConflictEngine extends TimestampConflictEngine {
+  public checkForConflicts(serverDelta: any, clientSets: any): ConflictStateMap {
+
+    if (super.checkForConflicts(serverDelta, clientSets) === undefined) {


If the returned result is ever null instead of undefined then this won't evaluate as expected as null !== undefined. Doing this will work for both possibilities:

Suggested change

if (super.checkForConflicts(serverDelta, clientSets) === undefined) {

if (!super.checkForConflicts(serverDelta, clientSets)) {

craicoverflow · 2020-07-08T08:14:24Z

+
+export function getDiffEntry(data: any): DiffEntry {
+  const sets = Object.assign({}, data);
+  const objectId = sets.id instanceof ObjectId ? sets.id : new ObjectId(sets.id);


I've seen this used somewhere else too. Would be nice in a helper function.

craicoverflow · 2020-07-08T08:20:30Z

-    if (isDataSyncModel(model)) {
-      return new DataSyncMongoDBDataProvider(model.graphqlType, db)
+    const annotationData = isDataSyncModel(model);
+    if (annotationData) {


You could improve readibility if you try to avoid nested ifs (where possible):

if (!annotationData) { return new MongoDBDataProvider(model.graphqlType, db) } let conflictStrategy: new() => ConflictEngine; if (conflictStrategyMap !== undefined) { conflictStrategy = conflictStrategyMap[model.graphqlType.name]; } if (annotationData.deltaTable === true) { return new DeltaDBDataProvider(model.graphqlType, db, conflictStrategy); } return new DataSyncMongoDBDataProvider(model.graphqlType, db, conflictStrategy);

craicoverflow · 2020-07-08T08:21:52Z

+      if (conflictStrategyMap !== undefined) {
+        conflictStrategy = conflictStrategyMap[model.graphqlType.name];
+      }
+      if (annotationData.deltaTable === true) {


annotationData can be boolean, so this would throw an error.

craicoverflow · 2020-07-08T08:22:45Z

 import { DataSyncCRUDService } from "./services";

-export function isDataSyncModel(model: ModelDefinition): boolean {
+export function isDataSyncModel(model: ModelDefinition): any {


If this can now return the annotation data the function name should change too.

craicoverflow · 2020-07-08T08:24:37Z

  public async update(data: Type, context: GraphbackContext): Promise<Type> {
    const { idField } = getDatabaseArguments(this.tableMap, data);
+    const updateSets = Object.assign({}, data as any);
+    delete updateSets.id;


getDatabaseArguments will return an ID field different to id if the @id annotation is different. So should this be deleting that key instead?

ssd71 · 2020-07-08T09:28:47Z

@craicoverflow Sorry for lack of proper instructions, I have updated PR description a bit, please test it out at your leisure

craicoverflow · 2020-07-08T09:49:13Z

No problem, thanks for updating :)

Should this still be in draft btw? Or are you seeking more implementation feedback first.

ssd71 · 2020-07-08T09:51:08Z

Looking for implementation feedback actually, not very confident about it, would definitely appreciate it 😄

wtrocki · 2020-07-08T11:52:45Z

-    }
+      const queryResult = await this.getServerState(clientData);
+      if (queryResult) {
+        const conflict = this.conflictEngine.checkForConflicts(queryResult, clientData)


I think while this made sense for the Postgress when we were building it for Mongo this approach makes less sense. There could be some operation happening in the middle of those two.

Approach here could be that we can research mongodb optimistic locking
We should check if we can have conditional errors in mongo - error that is returned if query do not accept some condition.

While this approach is ok.. it is definitely not production ready as consistency is not guaranteed here.

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

+  timestamp: Date
+}
+
+export function getDiffEntry(data: any): DiffEntry {


I would love to get more explaination what you creating here.
I see couple fields removed that I would consider essential.

wtrocki · 2020-07-08T11:56:50Z

@ssd71 Results of the investigations are usually presented in the form of the docs. It is nice to have working code for it, but if we getting just code and no info about the approach or strategy you are taking here it is not possible to really move forward.

We want to investigate proper patterns and document how we see this working. Step by step kinda approach. Maybe some quick diagram etc.
I see some changes - some of those sound good, some of those might need more explanation as they look controversial. Let's put some document or comment in issue first.

ssd71 · 2020-07-24T10:26:17Z

Results of investigation and spike documented in #1738

ssd71 requested review from craicoverflow, machi1990 and wtrocki July 3, 2020 06:06

wtrocki reviewed Jul 3, 2020

View reviewed changes

craicoverflow reviewed Jul 3, 2020

View reviewed changes

ssd71 added 3 commits July 8, 2020 10:52

feat(datasync): add delta table

ea68826

feat: implement audit/delta table and sync

85832e2

test: fix failing tests

c0a825c

ssd71 force-pushed the one-small-step branch from 582c523 to c0a825c Compare July 8, 2020 05:30

ssd71 requested review from craicoverflow and wtrocki July 8, 2020 05:37

ssd71 added the spike label Jul 8, 2020

machi1990 mentioned this pull request Jul 8, 2020

parse metadata and store them in ModelDefinition as much as possible #1675

Closed

craicoverflow reviewed Jul 8, 2020

View reviewed changes

wtrocki reviewed Jul 8, 2020

View reviewed changes

ssd71 closed this Jul 24, 2020

craicoverflow mentioned this pull request Jul 27, 2020

[WIP]Datasync prototype #1755

Closed

3 tasks

wtrocki deleted the one-small-step branch May 6, 2021 14:20


		public sync(lastSync: string, context: GraphbackContext, filter?: any): Promise<Type[]> {

		return this.deltaProvider.sync(parseInt(lastSync, 10)).then((res: any[]) => {

+              }
+              /**
+               * d

	protected conflictFieldName: string = undefined;
	protected conflictFieldName: string;

	if (serverState[this.conflictFieldName] !== undefined && clientState[this.conflictFieldName].toString() !== serverState[fieldNames.updatedAt].toString()) {
	if (serverState[this.conflictFieldName] && clientState[this.conflictFieldName].toString() !== serverState[fieldNames.updatedAt].toString()) {

	if (super.checkForConflicts(serverDelta, clientSets) === undefined) {
	if (!super.checkForConflicts(serverDelta, clientSets)) {

Uh oh!

Conversation

ssd71 commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craicoverflow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssd71 commented Jul 4, 2020

Uh oh!

ssd71 commented Jul 8, 2020

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssd71 commented Jul 8, 2020

Uh oh!

craicoverflow commented Jul 8, 2020

Uh oh!

ssd71 commented Jul 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtrocki commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ssd71 commented Jul 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ssd71 commented Jul 3, 2020 •

edited

Loading

craicoverflow left a comment •

edited

Loading

wtrocki commented Jul 8, 2020 •

edited

Loading