better manage default values

[antmendoza]

Many thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:

@tsurdilo @JBBianchi

This is a POC to get feedback about this implementation, that allows us to have more control on what is been serialized/deserialized

We are using here https://github.com/typestack/class-transformer/ to convert from json to object and we have to enable experimental mode to use decorators/annotations (I still have to read more about decorators and possible inconveniences)

The actual implementation is under the folder poc, https://github.com/antmendoza/sdk-typescript/tree/poc_/src/poc, I have named every new class as PocXXX to don't confuse myself, the Poc prefix have to be removed and the idea is to convert each of the current interfaces to a class.

As you will see every class has a builder() method that allows us to have the same fluent api as before as well as validation against json schema as before (it is pending to add logical validation)

PocWorkflow.builder()
			.id('applicantrequest')
			.version('1.0')
			.name('Applicant Request Decision Workflow')
			.description('Determine if applicant request is valid')
			.start('CheckApplication')
			.build()

We have to annotate attributes ( example here and here) and in some cases (like the funcions attribute) add logic, to have nested object converted.

The actual change involve:

• convert every interface to a class
• add the build() method to the new class + validation, that means the implementation of the current builders (and remove the current builders)
• add logic to the class to serialized/deserialized object like here

I have written some tests that demonstrate the actual implementation.

Please let me know your thoughts.

[tsurdilo]

@antmendoza very nice!!
so with generating concrete classes we can set the defaults there rather than having some custom stuff to set/maintain it?

[antmendoza]

@tsurdilo this is not automatically generated, moving from interfaces to classes and all the annotations... we have to do it by hand.

Classes we could generate automatically probably, but all the code around I don't think so.

Default values are set in the method from fromString https://github.com/antmendoza/sdk-typescript/blob/poc_/src/poc/poc-workflow.ts#L120, (I am not happy with this way, but I wanted some feedback before of creating a PR with 200 changes).
Something similar as it is done here https://github.com/antmendoza/sdk-java/blob/main/api/src/main/java/io/serverlessworkflow/api/deserializers/DataInputSchemaDeserializer.java#L53

Constant are set in the constructor https://github.com/antmendoza/sdk-typescript/blob/poc_/src/poc/poc-databasedswitch.ts#L47

For default values and constant I can investigate and see if we could have something like

@Default(true)
usedForCompensation: boolean;

@Constant('switch')
type: 'switch';

but still we need to have some code to handle "oneOf" and conditional required fields https://github.com/antmendoza/sdk-typescript/blob/poc_/src/poc/poc-injectstate.ts#L77

[JBBianchi]

@antmendoza I'm very grateful for the huge amount of work you put into this but I must admit I'm quite confused. I didn't look deep enough into it but at first glance I have two concerns:

• this doesn't look compatible with automation (or at least I don't see how yet)
• if my first point is valid, it might be a little "over engineered". Again I didn't digg into it enough to give a proper opinion but I feel like the class transformer/decorator approach is a bit overkill. I feel like the same result could be achieved without those lib/layers. E.g.: I don't really get yet what's the advantage of plainToClass(PocWorkflow, Object.assign(defaultValues,yaml.load(value))) over an out of the box Object.assign(new PocWorkflow(),defaultValues,yaml.load(value))

Other questions/remarks that are more tangible:

• I might be mistaken but in the PocWorkflow for instance, it looks like the default expressionLang is set when using fromString but not when using the constructor?
• I don't see any management of the default values in toYaml/toJson, does it mean that the defaults "bleed out" when serializing ?
• I don't understand why you'd use the builder proxy in conjonction with the "PocBuilder", the PoC classes expose their own properties, you don't need a proxy in this case. I'm a bit confused ...

I'm really sorry if I missed something or if the critic is a bit harsh, once again, your work is much appreciated.

[antmendoza]

Hi @JBBianchi no problem,

thanks for your comments,

this doesn't look compatible with automation (or at least I don't see how yet)

It is not, I am not sure if it is gonna be easy but from my point of view having the types generated and builders with validation is enough, I don't think that we are having the specification changing with a new version every month. I don't see a problem having things done by hand as long as it is working.

if my first point is valid, it might be a little "over engineered". Again I didn't digg into it enough to give a proper opinion but I feel like the class transformer/decorator approach is a bit overkill. I feel like the same result could be achieved without those lib/layers. E.g.: I don't really get yet what's the advantage of plainToClass(PocWorkflow, Object.assign(defaultValues,yaml.load(value))) over an out of the box Object.assign(new PocWorkflow(),defaultValues,yaml.load(value))

Neither I am a big fan of having this library here but I have not found any better way, I will spend some more time this weekend on it.

I am still exploring this library but plainToClass and classToPlain invoke the constructor and make work the annotations/decorators the library provide ( like https://github.com/antmendoza/sdk-typescript/blob/poc_/src/poc/poc-function.ts#L20 ), so we can can control the object lifecycle (including serialization and deserialization)

When we deserialize a string we can populate default values, I don't know how to achieve this with Object.assign(new PocWorkflow(),defaultValues,yaml.load(value)), including child nodes, see https://github.com/antmendoza/sdk-typescript/blob/poc_/tests/poc/applicantrequest.spec.ts#L72

Other questions/remarks that are more tangible:

I might be mistaken but in the PocWorkflow for instance, it looks like the default expressionLang is set when using fromString but not when using the constructor?

That implementation was passing the test, but I think that a better approach is this one . I have changed it now. As I have mention before, by calling plainToClass invoque de @transform decorators and populate default values if not set.

I don't see any management of the default values in toYaml/toJson, does it mean that the defaults "bleed out" when serializing ?

I think that default should not be populated in serialization, only deserialization (#108 (comment) ). Please @tsurdilo correct me if I am wrong.

I don't understand why you'd use the builder proxy in conjonction with the "PocBuilder", the PoC classes expose their own properties, you don't need a proxy in this case. I'm a bit confused ...

Builder provide a fluent api and validation as you created it. I think that both are compatibles, in the same way we have now types exposing properties and builders.

[JBBianchi]

@antmendoza

I think I kinda see the interest of plainToClass and classToPlain and the decorators. I still think the same kind of result could be achieved without the use of the lib but it might be less sexy. For instance, I usually hydrate my classes with parameterized consturctor, something along those lines:

/** Requirements

const isObject = (value: any): boolean => {
  if (!value) return false;
  const type = typeof value;
  return type === 'object' && !Array.isArray(value);
}

// this is a quick & dirty trick, doesn't handle dates, regexp, symbols...
const deepCopy = (object: any): any => {
  return JSON.parse(JSON.stringify(object));
}

class Hydrator {
  constructor(model?: any) {
    if (model && isObject(model)) {
      Object.assign(
        this,
        deepCopy(model)
      );
    }
  }
}

**/

class PocWorkflow extends Hydrator {
  // ...props declarations...

  constructor(model?: any) {
    super(model);
    // set defaults
    this.expressionLang = this.expressionLang || 'jq';
    if (model) {
      // hydrate non primitive properties
      if (model.states?.lenght) {
        this.states = this.model.states.map(s => {
          switch (s.type) {
            case 'inject':
              return new PocInjectstate(s);
            case 'subflow':
              return new PocSubflowstate(s);
            case 'switch':
              return new PocDatabasedswitch(s);
          }
        });
      }
    }
  }

  static fromString(value: string): PocWorkflow {
    return new PocWorkflow(yaml.load(value));
  }

  static toYaml(workflow: PocWorkflow): string {
    validate('Workflow', workflow);
    const clone = deepCopy(workflow);
    // we don't want to output the defaults
    if (clone.expressionLang === 'jq') {
      delete clone.expressionLang;
    }
    return yaml.dump(clone);
  }

}

Anyway, my biggest concern still is the incompatibility with the current approach. From my understanding, the PoC classes have to define the properties (so they can be decorated for instance) and they can declare their own validation function. This means that the generated types & builders are of no use anymore. It would instead require to manually create "PoCBuilders" which kinda fallback to the first approach and its disadvantages.

In conclusion, we obviously don't have the constraints introduced by automatic generation when building everything manually. The question is(are) do we want the freedom at the cost of more work or ease of maintenance (and also, can we manage defaults with the current approach).

[JBBianchi]

@tsurdilo your input is also welcome knowing it's more of a conceptual/philosophical aspect rather than a technical one.

[antmendoza]

@JBBianchi

I usually prefer the way you are proposing here, but this time I think that it is more practical to use an external library that is doing the work for us, this or any other one, due to the amount of switch / if... else

builders and validation, I really like the way you implemented it and I don't think they are not used, instead I have copied the same code into the POC implementation

Having a separate class for builders? Idk to be honest, but still your builder proxy can be reused in this case.

I am also copying the code from interfaces into classes, so having the interfaces and types generated automatically is also used here (and very useful and time saving btw)

Moving from interfaces to classes is 1 minute each I would say, and adding the decorators less than 5 minutes once you have done a couple of them. We still have to add code into the fn function because the oneOf conditions. (not sure if this is the right place)

I don't think that after version 1 of the specification we will be having a lot of changes in the specification.

The issue with the actual implementation is the amount of if.. else we have to code to make serialization/deserialization work in this way and thus the amount of tests we will have to put together to ensure that it is working

I might be wrong, for sure 🙄

anyway, if this behaviour is not required for the sdks this conversation does not have any sense.

[JBBianchi]

I see your point and I think your solution is smart but still, my spider sense is tingling, something feels off.

I might be wrong, for sure 🙄

Please don't say that, you're working hard and providing a solution and that's great, thanks a lot! It is not a trivial problem and none of us have a solution. We are all at the same point, nobody is right or wrong, just trying to figure how out to make things work the best.

I'm challenging the choice for plainToClass and the decorators and suggesting the alternative "vanilla" methodology because it requires to redeclare the properties of the type in the builder class and that feels off to me (deduplication of code, union types, or 'data vs this conflict'). Futhermore, it would maybe be possible to rewrite the current builders as class in a similar way, with a constructor, fromString, toYaml/Json and maybe even validate ... It would use the same kind of "mixed" generation (with some part of manual code injected in the generated builders).

[tsurdilo]

I am a fan of generating concrete classes rather than interfaces because you can extend them.
To me personally the thing that should matter the most is not how things are more or less implemented as there are always multiple solutions to everything, but rather how the user api looks like.

From the perspective of the majority of people writing their dsl in json/yaml it would be nice if it's possible to just do:

  const workflow = Workflow.fromSource(source);

and then allow for getters and setters for everything. If builders are a must thats fine too but if possible to reduce it to the least amount of code user has to write.

I don't really think that validation needs to be necessarily built into the core classes. It could be externalized into its own class if that makes things easier. it could be its own module that can be added separately like its with the java sdk. just ideas.

For defining workflow using the object model, I like the way you have it here. Only thing that if possible to keep the amount of "build()" calls to a minimum as thats just noice for users imo.

Overall I am super impressed by typescript sdk and don't worry too much I think it will evolve over time.
Most important at least is yeah that it works :) has the most simple api (in terms of amount of code user has to do) and cherry on top is if it provides a way for users to extend it if they chose so ..and I believe that with generating classes would be easier to do. wdyt?

[JBBianchi]

@tsurdilo From my current knowledge state, builders are required with the build() call overhead. I have no viable solution for generating classes. I hope we find a solution for it in the future (like quicktype supporting union types instead of merging into one class) but for now, I think we are stuck with types/interfaces.

@antmendoza I had a deeper look and I think this additionnal layer brings a some confusion. The PocWorkflow (or PocSomething) constructor() and fromString() have a different behavior than the builder() method which doesn't rely on the class and its decorators.

One thing that was pointed to me on the JSON Schema Slack is the option to add the default values when validating: https://ajv.js.org/guide/modifying-data.html#assigning-defaults

This means we could output the default values when needed by passing a flag to build() for instance. In other words, most of the time, the defaults values would stay silent unless the dev using the SDK explicitly needs them and call build(true) on the higher object of the chain. (at least, that's how I see it, I didn't test yet).

[tsurdilo]

@tsurdilo From my current knowledge state, builders are required with the build() call overhead. I have no viable solution for generating classes. I hope we find a solution for it in the future (like quicktype supporting union types instead of merging into one class) but for now, I think we are stuck with types/interfaces.

Is this due to limitations of schema generators out there? Otherwise I don't really see this statement as a valid point honestly, sorry :)

We were able to solve this with Java SDK by for example:

1. creating a custom BaseWorkflow class: https://github.com/serverlessworkflow/sdk-java/blob/main/api/src/main/java/io/serverlessworkflow/api/workflow/BaseWorkflow.java
2. telling the generation engine to use it as a base class for the generated classes: https://github.com/serverlessworkflow/sdk-java/blob/main/api/src/main/resources/schema/workflow.json#L6

We also created custom interfaces for generic types, rather than trying to generate them: https://github.com/serverlessworkflow/sdk-java/tree/main/api/src/main/java/io/serverlessworkflow/api/interfaces
Idk if that helps.

Same thing for Function defs events, etc. Having this "custom" solution is a little bit of a setup but in end allows us to work around limitations of schema validators which is also the case in the Java world as well.

3. For hard stuff in the schema like some properties having multiple types, yes thats an issue we encountered also in java sdk and we fixed it by custom serializers and deserializers, for example let's look at "events" definition which can be either a string or array. We just say ok its type "object" and then we check which it really is: https://github.com/serverlessworkflow/sdk-java/blob/main/api/src/main/java/io/serverlessworkflow/api/deserializers/EventsDeserializer.java#L67

So there are ways to get around limitations IMO, but I understand the TypeScript world is quite different so maybe there are more obstacles there. Idk.

[tsurdilo]

@JBBianchi i mean worst case scenario there are even things like this - https://www.habarta.cz/typescript-generator/maven/typescript-generator-maven-plugin/generate-mojo.html

we could spit out typescript from the currently generated Java classes in the java-sdk :) idk if that would even be an option but just saying that if we are running into the same issues in typescript sdk but are too hard to solve there are always options :)

[antmendoza]

I don't really think that validation needs to be necessarily built into the core classes. It could be externalized into its own class if that makes things easier. it could be its own module that can be added separately like its with the java sdk. just ideas.

@tsurdilo there is also a workflowValidator class, where we have to add the workflow integrity validation.

@JBBianchi I don't have any problem having the builders in a separate class, on the contrary.

I had a deeper look and I think this additionnal layer brings a some confusion. The PocWorkflow (or PocSomething) constructor() and fromString() have a different behavior than the builder() method which doesn't rely on the class and its decorators.

I was just focusing in the output. We can change it (here and here) and remove the constructor. Now both (builder and fromString) rely on decorators.

I might agree that it is not the best solution we could reach but it's a solution that give us the behaviour we want. As @tsurdilo said once we have the API we can change the implementation.

My plan is to continue with this implementation (by hand) and see: 1)if it works; 2) if we agree an API. is it ok?

[JBBianchi]

@tsurdilo indeed, I never said it was impossible, I just said I reached the limitation of what I know.

@antmendoza sure, I don't see why not. I would advice to double check your methodology with v0.7 of the spec.

Nevertheless, bare in mind that going for classes vs types fixes some problems but introduce others, or at least drawbacks. Take Action for instance, with the type definition it clearly defines you can have either functionRef or eventRef set (as the spec says). It's type safe at compile time, the IDE can tell you that your need either one of those to be set. The class definition would look like this:

export class Action {
      name?: string;
      functionRef?: Functionref;
      eventRef?: Eventref;
      timeout?: string;
      actionDataFilter?: Actiondatafilter;
}

Which means you'd only know at runtime, when validating the object, that either functionRef or eventRef is required, it cannot be induced by the compiler. There might be another notation form in the class to say that one of the two is required but I don't know how.

[antmendoza]

Hi

we have most of the code for this PR auto generated, > 95% I would say. There is a lot of snippets with if.. else, I have to clean it a little bit and make more maintainable.

A couple of things, builder is a method inside each class, I would prefer to have in on a different class? wdyt?

For union types, as @JBBianchi has already mention we don't have validation at compile time, we will have to relies in the validate method.

And for union types like End, Startdef, Funciontref, Crondef..., I have moved the union to the property declaration

[antmendoza]

builder is a method inside each class, I would prefer to have in on a different class? wdyt?

or we can use the funcions we already have.

[JBBianchi]

I think the functions are enough, I don't really see the use for additionnal class.

[antmendoza]

👍

[antmendoza]

Quick update.

the class-transformer lib is not working as I was expecting and when a value is not set to a property we can not define constant while deserialization (instead is working only in serialization).

So I have implemented the solution more or less in the way that @JBBianchi proposed here, thanks btw.

I will create a PR today.