moved from types to classes #114

antmendoza · 2021-06-14T20:55:16Z

Many thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:
closes #111
closes #104

Special notes for reviewers:

There are many files changes but we can resume them in:

updated README.md
remove types and create classes instead.
added to builder constant and 'oneOf' related logic
create constructors on every class that populate default values, special implementation for workflow
removed WorkflowConverter and moved methods to workflow (fromSource, toYaml, toJson)
definitions are generated now into src/lib/schema/types/workflow.ts
format validation error output

I am still working in the generate-classes file to have most of the code autogenerated, will create a PR soon.
Additional information (if needed):

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

antmendoza · 2021-06-14T21:06:04Z

format validation error output

this is not related with the rest of the changes, I can create another PR for this if you want

JBBianchi

Impressive work @antmendoza, congratz! There are still few things to address, the "bigger" one being the "hydrating" of the non primitive types in the constructors.

JBBianchi · 2021-06-15T09:42:28Z

src/lib/builders/action-builder.ts

+    const result = {} as Specification.Action;
+
+    Object.assign(result, data);
+    validate('Action', result);
+    return result;


I think the building functions should rely on the constructor of their underlying type.

For instance:

const model = new Specification.Action(data); // I don't like `result`, it's not "self describing", but that's just a little detail, not very important validate('Action', model ); return model;

This keeps the creation of the object and the defaulting of the properties DRY. If you take the example of callbackstate-builder, it does the same think than it's constructor counterpart. Furthermore, using a constructor instead of an object literal allows the use of instanceof (aka runtime typechecking in addition to compile time typechecking):

const foo = new Specification.Action(); // This means "create a new instance of Action" const bar = {} as Specification.Action; // This means to TypeScript only "consider this object literal as Action" console.log(foo instanceof Specification.Action); // true, the object is "typed" at runtime console.log(bar instanceof Specification.Action); // false, this is just an object literal

thanks @JBBianchi

I can change the property name to model, no problem

The problem using constructor is that the code in the actual constructor is setting values we don't want in the final serialized json for example https://github.com/antmendoza/sdk-typescript/blob/from-types-to-classes/src/lib/definitions/function.ts#L18, we don't want type:rest if the client has not set it. Instead we want the value on deserialization.

Indeed, and I don't really see how to prevent that... Maybe with a prepareSerialization() method or something that would need to be called on the whole object tree to mutate the serialized copy but that doesn't sound right... Or maybe using class-transformer for that? But that feels wrong as well :/

JBBianchi · 2021-06-15T09:45:07Z

src/lib/builders/eventstate-builder.ts

+    const result = {
+      type: 'event',
+    } as Specification.Eventstate;
+
+    if (!data.end && !data.transition) {
+      result.end = true;
+    }
+
+    Object.assign(result, data);


In relation to my previous comment, this should be done in the constructor and be kept in one place only.

JBBianchi · 2021-06-15T10:03:09Z

src/lib/builders/delaystate-builder.ts

+    const result = {
+      type: 'delay',
+    } as Specification.Delaystate;
+
+    Object.assign(result, data);
+    validate('Delaystate', result);
+    return result;


Handling of usedForCompensation is missing?

Since usedForCompensation is required now I don't think we have to set the value, I am thinking now that setting the value useForCompensation in the constructor is not needed, I will have a look

It might not be useful indeed, it should validate/be considerated false by default without having to specify it. @tsurdilo do you agree?

JBBianchi · 2021-06-15T10:04:18Z

README.md

@@ -19,7 +19,7 @@ To build the project and run tests locally:
 ```sh
 git clone https://github.com/serverlessworkflow/sdk-typescript.git
 cd sdk-typescript
-npm install && npm run update-code-base && npm run test


With or without running the code generation should produce the same output...

JBBianchi · 2021-06-15T10:05:25Z

src/lib/definitions/action.ts

+
+export class Action {
+  constructor(model: any) {
+    const result = {};


Use the name defaultModel instead of result ? It's more descriptive?

JBBianchi · 2021-06-15T10:23:14Z

src/lib/definitions/subflowstate.ts

+
+export class Subflowstate {
+  constructor(model: any) {
+    const result = {


Missing type: 'subflow' (after Object.assign to enforce the const event if something else is specified in model ?)

JBBianchi · 2021-06-15T10:23:21Z

src/lib/definitions/subflowstate.ts

+  /**
+   * State type
+   */
+  type?: 'subflow';


type is required

JBBianchi · 2021-06-15T11:05:19Z

src/lib/definitions/workflow.ts

+    const functions = this.functions;
+    if (typeof functions === typeof []) {
+      this.functions = (functions as Function[]).map((f) => new Function(JSON.stringify(f))) as Functions;
    }


I don't understand the copy of the reference of function neither the stringify in the Function constructor.

I thing I would go for:

if (typeof model.functions !== typeof '') { this.functions = (model.functions as Function[]).map((f) => new Function(f)) as Functions; }

but maybe I'm missing something?

Furthermore, I think this should ideally be done for all properties that are not primitive types, in all the definitions... That's the only way to properly hydrate the models I think.

JBBianchi · 2021-06-15T11:05:58Z

src/lib/definitions/workflow.ts

-        [key: string]: any;
-      };
+
+    const states = this.states;


Same, I don't think copying the reference really makes sense.

JBBianchi · 2021-06-15T11:07:16Z

src/lib/definitions/workflow.ts

-      metadata?: /* Metadata information */ Metadata;
+  static fromSource(value: string): Specification.Workflow {
+    try {
+      return yaml.load(value) as Specification.Workflow;


Should be wrapped into the constructor:

return new Specification.Workflow(yaml.load(value));

antmendoza · 2021-06-15T21:13:58Z

After reading all your comments @JBBianchi most of then are related to setting default values and constants and I think that everything can be merged in the constructor, as you've mentioned

👍

antmendoza · 2021-06-16T05:33:27Z

By using the constructor in the builders makes the test fail, since serialized workflow (json file) do not has set default values like type:rest for Function class.

see antmendoza@86d4583#diff-c590f689b760b021149371854a8e8dcf45b515f719c7a8abfd5dafea608cf7f2R136

JBBianchi · 2021-06-16T07:37:26Z

By using the constructor in the builders makes the test fail, since serialized workflow (json file) do not has set default values like type:rest for Function class.

see antmendoza@86d4583#diff-c590f689b760b021149371854a8e8dcf45b515f719c7a8abfd5dafea608cf7f2R136

If you use constructors all the way down, even when deserializing, it should be ok, shouldn't it?

antmendoza · 2021-06-19T08:10:46Z

deserializing is working ok, serializing is when we are populating default values we don't want to be in the final serialized string

as you mention before, we have to modify the object before serialize it, the place I can see now to do it is in the xxxBuildingFn event if I don't like it.


function functionBuildingFn(data: Specification.Function): () => Specification.Function {
  return () => {
    
    const model = new Specification.Function(data);
    
    //we want the value only if the client has set it.
    model.type = data.type;
    
    validate('Function', model);
    return model;
  };
}

having this logic here is weird to me, anyway.

(I am thinking that the proper way might be having two serilize/deserialize methods or classes per type, as they have specific logic )

Let's make this works and we will see later, as the API should not change

I am going to modify the code in this way and see if everything works as expected

JBBianchi · 2021-06-19T09:07:46Z

Ah yes, sorry, I mixed things up. Serializing is indeed a bit tricky (maybe even more than "a bit")...
The objects could maybe have a makeSerializable() method that would recursively delete default values:

export class Workflow {

  makeSerializable() {
    const clone = deepCopy(this);
    // managing self defaults
    if (clone.expressionLang === 'jq') {
      delete clone.expressionLang;
    }
    // deeply removing defaults
    if (typeof clone.start !== typeof '') {
      clone.start = clone.start.makeSerializable();
    }
    if (clone.execTimeout) {
      clone.execTimeout = clone.execTimeout.makeSerializable();
    }
    //...
    clone.states = clone.states.map(s => s.makeSerializable());
    return clone;
  }

  static toJson(workflow: Workflow): string {
    validate('Workflow', workflow);
    return JSON.stringify(workflow.makeSerializable());
  }

  static toYaml(workflow: Workflow): string {
    validate('Workflow', workflow);
    return yaml.dump(workflow.makeSerializable());
  }

}

antmendoza · 2021-06-19T13:42:07Z

I don't think we should delete always default values, only if the user/client has not set them

JBBianchi · 2021-06-20T16:30:25Z

How to differentiate the two use cases?

antmendoza · 2021-06-21T06:17:34Z

We can't, to make it work I am re-setting the value after creating the object, in the builder. Pretty ugly, I know.

function functionBuildingFn(data: Specification.Function): () => Specification.Function {
  return () => {
    const model = new Specification.Function(data);

    //HERE
    model.type = data.type;

    validate('Function', model);
    return model;
  };
}

I am testing also the implementation by "hidratating" non-primitive object and I think that it is not enough, since non-primitive properties are created as Object type

    const data = {
      "events": [
        {
          "name": "CarBidEvent",
          "type": "carBidMadeType",
          "source": "carBidEventSource"
        }
      ]
    }
    
    const model = new Workflow(data);
   // @ts-ignore
   expect(model.events[0]!.constructor.name).toBe("Eventdef"); -> // Expected 'Object' to be 'Eventdef'.

so for each non primitive property I am reseting it by instantiating the corresponding class

JBBianchi · 2021-06-21T07:18:53Z

We can't, to make it work I am re-setting the value after creating the object, in the builder. Pretty ugly, I know.
function functionBuildingFn(data: Specification.Function): () => Specification.Function {
  return () => {
    const model = new Specification.Function(data);

    //HERE
    model.type = data.type;

    validate('Function', model);
    return model;
  };
}

I don't understand why you'd set type again here, model.type is set inside the constructor ... Am I missing something?

I am testing also the implementation by "hidratating" non-primitive object and I think that it is not enough, since non-primitive properties are created as Object type
    const data = {
      "events": [
        {
          "name": "CarBidEvent",
          "type": "carBidMadeType",
          "source": "carBidEventSource"
        }
      ]
    }
    
    const model = new Workflow(data);
   // @ts-ignore
   expect(model.events[0]!.constructor.name).toBe("Eventdef"); -> // Expected 'Object' to be 'Eventdef'.
so for each non primitive property I am reseting it by instantiating the corresponding class

In the current state of workflow.ts, I don't see the hydration of events. If the non primitive properties are not instanciated, they indeed are just plain objects. The constructor is missing

if (model.events && typeof model.events !== typeof '') {
  this.events = model.events.map(e => new Eventdef(e));
}

Another concern I just thought about: what if the user sets a property outside the constructor... ?

const wf = new Workflow();
wf.events = [
  {
    name: "CarBidEvent",
    type: "carBidMadeType",
    source: "carBidEventSource"
  }
];

This would assign a plain object to the events property, it would not be hydrated ... :/

The workaround would be to define the properties with getters/setters to hydrate objects instead of doing it in the constructor ...

export class Workflow {
  private _events?: Events;
  get events(): Events {
    return this._events;
  }
  set events(value: Events) {
    if (value && typeof value !== typeof '') {
      this.events = value.map(e => new Eventdef(e));
    }
    else {
      this.events = value;
    }
  }

  constructor(model: any) {
    const defaultModel = { expressionLang: 'jq' } as Specification.Workflow;
    Object.assign(this, defaultModel, model);
  }
}

but that's a lot of hassle...

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

antmendoza · 2021-06-21T08:27:50Z

I don't understand why you'd set type again here, model.type is set inside the constructor ...

I have added a test @JBBianchi

hope this helps

JBBianchi · 2021-06-21T09:06:47Z

I don't understand why you'd set type again here, model.type is set inside the constructor ...

I have added a test @JBBianchi

hope this helps

Sorry, it's Monday morning, maybe I'm not awake but I still don't understand. "In memory" objects should have the default value...

This line of function-builder is redundant:

model.type = data.type;

It's already assigned when calling:

const model = new Specification.Function(data);

As the function constructor already assign the values:

Object.assign(this, defaultModel, model);

In other words:

const fn1 = new Function({ name: 'function', operation: 'operation' }); // fn1.type === 'rest'
const fn2 = functionBuilder().name('function').operation('operation').build(); // fn2.type === 'rest'
const fn3 = new Function({ name: 'function', operation: 'operation', type: 'grpc' }); // fn3.type === 'grpc'
const fn4 = functionBuilder().name('function').operation('operation').type('grpc').build(); // fn4.type === 'grpc'
const fn5 = new Function({ name: 'function', operation: 'operation', type: 'rest' }); // fn5.type === 'rest'
const fn6 = functionBuilder().name('function').operation('operation').type('rest').build(); // fn6.type === 'rest'
fn1.makeSerializable(); // { name: "function", operation: "operation" }
fn2.makeSerializable(); // { name: "function", operation: "operation" }
fn3.makeSerializable(); // { name: "function", operation: "operation", type: 'gprc' }
fn4.makeSerializable(); // { name: "function", operation: "operation", type: 'gprc' }
fn5.makeSerializable(); // { name: "function", operation: "operation" } <-- default is gone even if the user specified it
fn6.makeSerializable(); // { name: "function", operation: "operation" } <-- default is gone even if the user specified it

Whenever the user set the default value or not, the value exists in memory but shouldn't get outputted in the serialized object I think. It's "not possible" to flag whenever the user actually set the property and still output the value in the serialized object if he did. At least, I don't think it's necessary/makes sense.

antmendoza · 2021-06-21T10:55:46Z

It is Monday for everyone @JBBianchi

I am not sure but I think that if the user set the value, the value should be outputted ,at least that would be the behaviour I was expecting as a client using the library.

As far I have seen, this only is happening with workflow.expressionLang and function.type

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

JBBianchi · 2021-06-22T08:10:28Z

I think usedForCompensation might also be concerned, eventstate.exclusive, operationstate.actionmode, parallelstate.completionType, subflowstate.waitForCompletion, workflow.end / workflow.end.compensate, repeat.continueOnError, workflow.keepActive, exectimeout.interrupt, transition.compensate, onevents.actionMode ...

antmendoza · 2021-06-22T09:13:57Z

👍 Thank you @JBBianchi , I am going to have a look

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

antmendoza · 2021-06-22T09:42:52Z

@JBBianchi I have added a couple of test for subflowstateBuilder and eventstateBuilder

615e174

how do you think this should work ?

JBBianchi

@JBBianchi I have added a couple of test for subflowstateBuilder and eventstateBuilder

615e174

how do you think this should work ?

I think the property assertion "toBeUndefined" are not valid, they should reflect the default value from what I understood from @tsurdilo guidelines.

Then, following my previous train of thoughts, the lines expect(JSON.stringify(object)).toBe( should be expect(JSON.stringify(object.makeSerializable())).toBe(

JBBianchi · 2021-06-22T12:59:52Z

tests/lib/builders/eventstate-builder.spec.ts

+import { eventstateBuilder } from '../../../src/lib/builders/eventstate-builder';
+import { oneventsBuilder } from '../../../src/lib/builders/onevents-builder';
+
+describe('subflowstateBuilder ', () => {


eventstate builder

JBBianchi · 2021-06-22T13:00:35Z

tests/lib/builders/eventstate-builder.spec.ts

+      .transition('Get Book Status')
+      .build();
+
+    expect(object.exclusive).toBeUndefined();


I guess this should be true (https://github.com/serverlessworkflow/specification/blob/0.6.x/schema/workflow.json#L535)

JBBianchi · 2021-06-22T13:01:21Z

tests/lib/builders/subflow-builder.spec.ts

+  it('should build an object', () => {
+    const object = subflowstateBuilder().name('StartApplication').workflowId('startApplicationWorkflowId').build();
+
+    expect(object.waitForCompletion).toBeUndefined();


this should be false (https://github.com/serverlessworkflow/specification/blob/0.6.x/schema/workflow.json#L1151)

antmendoza · 2021-06-22T15:49:31Z

Ok, sorry I miss all the defaults when get rid of 'class-transformer'

so putting this on code, the behaviour should be something like



describe('eventstateBuilder ', () => {
  it('should build an object', () => {
    
    const object = eventstateBuilder()
      .name('Book Lending Request')
      .onEvents([oneventsBuilder().eventRefs(['Book Lending Request Event']).build()])
      .transition('Get Book Status')
      .build();

    expect(object.exclusive).toBeTrue();
  
    const serializedObject = object.serialize();
    expect(serializedObject).toBe(
      JSON.stringify({
        type: 'event',
        name: 'Book Lending Request',
        onEvents: [
          {
            eventRefs: ['Book Lending Request Event'],
          },
        ],
        transition: 'Get Book Status',
      })
    );
    
    
    const deserializedObject = object.deserialize(serializedObject)
    expect(deserializedObject.exclusive).toBeTrue();
    
    
    
    //Having serialize/ deserialize de following signature
  
    //  deserialize(value: string): Eventstate {

    //   }
    //   serialize(): string {

    //   }
    //  
  
  });
});

JBBianchi · 2021-06-22T16:18:44Z

I think that's the idea. I don't know if you need a serialize() method on each object but at least a method to get rid of the defaults (in my example, makeSerializable() (which is an ugly name) returns an object. It is required to traverse nested objects to rebuild nested objects without defaults)

antmendoza · 2021-06-24T07:15:29Z

I think we should move serialize/deserialize functions to another class, workflowSerializer, functionSerializer... , otherwise the compiler is complaining all the time because objects does not have serialize/deserialize properties, which makes sense.

Making them optionals does not seem the solution.

BTW I am not sure that deserialize is needed

JBBianchi · 2021-06-24T09:20:06Z

otherwise the compiler is complaining all the time because objects does not have serialize/deserialize properties, which makes sense.

I'm not sure to understand what you mean by "the compiler is complaining all the time" but I'm not against having dedicated classes for those.

Note that serialize/deserialize should rather be toJson/toYaml/fromString (or maybe serialize(obj, SerializationFormat.Json) for instance) I think, like we have in the current workflow-converter.

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

antmendoza · 2021-06-25T16:43:27Z

Added default values to each constructor and normalize method when required, to delete recursively properties whose value = default value

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

antmendoza · 2021-06-28T19:06:52Z

@tsurdilo @JBBianchi this PR is ready to be reviewed.

@JBBianchi since we are creating object for each non-primitive property in the constructor , do you thing that hydration is still needed?

JBBianchi · 2021-06-29T11:19:04Z

You did an amazing job @antmendoza, congratz! 👏

@tsurdilo @JBBianchi this PR is ready to be reviewed.

I'll try to digg into the details and review asap. I did a quick test and a very fast look around, it seems great. Maybe a detail or two that I'll point but it will probably be more about code style/preference rather than concepts.

@JBBianchi since we are creating object for each non-primitive property in the constructor , do you thing that hydration is still needed?

I don't understand the question, creating a new instance for each non promivite prop is hydration. Instead of having "dumb" objects, you recursively hydrated each object to its instance. So if I'm not missing something, it's all good here !

Thanks again mate, you did great!

antmendoza · 2021-07-02T18:55:09Z

Hi @JBBianchi @tsurdilo

did you have the chance to look into the PR...?

this is not a final code, there will be more changes in the sdk for sure... If there is no any major issue I would like to have this merge at some point soon in order to cover the 0.6 spec version

If you need any clarification please let me know 🙂

JBBianchi · 2021-07-03T08:48:08Z

I'm very sorry I didn't have the opportunity to check in depth. At a first glance, it looks good so let's merge, we can still change things if needed.

moved from types to classes

b7e61dd

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

antmendoza requested a review from tsurdilo as a code owner June 14, 2021 20:55

antmendoza requested a review from JBBianchi June 14, 2021 21:02

JBBianchi requested changes Jun 15, 2021

View reviewed changes

antmendoza added 2 commits June 21, 2021 09:48

moved from types to classes

ebb04b6

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

added function-builder test

1f756a1

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

added function-builder test

4bb43e2

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

added eventstate-builder and subflow-builder test

615e174

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

JBBianchi reviewed Jun 22, 2021

View reviewed changes

added default values and normalize method

ed03caf

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

modified generate-builders.ts

01424a8

Signed-off-by: Antonio Mendoza Pérez <[email protected]>

tsurdilo approved these changes Jul 2, 2021

View reviewed changes

JBBianchi approved these changes Jul 3, 2021

View reviewed changes

antmendoza merged commit 6e6dc9d into serverlessworkflow:main Jul 4, 2021

JBBianchi mentioned this pull request Jul 5, 2021

Prewarmed/automatic "builder extensions" #105

Closed

antmendoza deleted the from-types-to-classes branch July 16, 2021 14:25

moved from types to classes #114

moved from types to classes #114

Uh oh!

Conversation

antmendoza commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antmendoza commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JBBianchi 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!

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!

antmendoza commented Jun 15, 2021

Uh oh!

antmendoza commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JBBianchi commented Jun 16, 2021

Uh oh!

antmendoza commented Jun 19, 2021

Uh oh!

JBBianchi commented Jun 19, 2021

Uh oh!

antmendoza commented Jun 19, 2021

Uh oh!

JBBianchi commented Jun 20, 2021

Uh oh!

antmendoza commented Jun 21, 2021

Uh oh!

JBBianchi commented Jun 21, 2021

Uh oh!

antmendoza commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JBBianchi commented Jun 21, 2021

Uh oh!

antmendoza commented Jun 21, 2021

Uh oh!

JBBianchi commented Jun 22, 2021

Uh oh!

antmendoza commented Jun 22, 2021

Uh oh!

antmendoza commented Jun 22, 2021

Uh oh!

JBBianchi 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!

antmendoza commented Jun 14, 2021 •

edited

Loading

antmendoza commented Jun 14, 2021 •

edited

Loading

antmendoza commented Jun 16, 2021 •

edited

Loading

antmendoza commented Jun 21, 2021 •

edited

Loading

antmendoza commented Jun 22, 2021 •

edited

Loading

antmendoza commented Jun 28, 2021 •

edited

Loading

antmendoza commented Jul 2, 2021 •

edited

Loading