serverlessworkflow / sdk-typescript

Typescript SDK for Serverless Workflow
https://serverlessworkflow.io/
Apache License 2.0
64 stars 16 forks source link

ignore additionalProperties when are declared as function #119

Closed antmendoza closed 3 years ago

antmendoza commented 3 years ago

Many thanks for submitting your Pull Request :heart:!

What this PR does / why we need it:

When I was testing the sdk into other project I realice that the behaviour of the binary (./dist/umd/index.umd.js) was different than the actual source code. The validate function was throwing an error related to have found the "normalize" method as additional property

  {
    instancePath: '',
    schemaPath: '#/additionalProperties',
    keyword: 'additionalProperties',
    params: { additionalProperty: 'normalize' },
    message: 'must NOT have additional properties'
  }

I have changed the way we are deffinig "normalize" method to be able to reproduce the error in test.

Special notes for reviewers: if you want to reproduce the error I've faced:


import {
    actionBuilder,
    databasedswitchBuilder,
    defaultdefBuilder,
    functionBuilder,
    functionrefBuilder,
    operationstateBuilder,
    Specification,
    subflowstateBuilder,
    transitiondataconditionBuilder,
    workflowBuilder,
} from '@severlessworkflow/sdk-typescript';

const workflow = workflowBuilder()
    .id('applicantrequest')
    .version('1.0')
    .name('Applicant Request Decision Workflow')
    .description('Determine if applicant request is valid')
    .start('CheckApplication')
    .functions([
        functionBuilder()
            .name('sendRejectionEmailFunction')
            .operation('http://myapis.org/applicationapi.json#emailRejection')
            .build(),
    ])
    .states([
        databasedswitchBuilder()
            .name('CheckApplication')
            .dataConditions([
                transitiondataconditionBuilder()
                    .condition('${ .applicants | .age >= 18 }')
                    .transition('StartApplication')
                    .build(),
                transitiondataconditionBuilder()
                    .condition('${ .applicants | .age < 18 }')
                    .transition('RejectApplication')
                    .build(),
            ])
            .default(defaultdefBuilder().transition('RejectApplication').build())
            .build(),
        subflowstateBuilder().name('StartApplication').workflowId('startApplicationWorkflowId').build(),
        operationstateBuilder()
            .name('RejectApplication')
            .actionMode('sequential')
            .actions([
                actionBuilder()
                    .functionRef(
                        functionrefBuilder()
                            .refName('sendRejectionEmailFunction')
                            .arguments({applicant: '${ .applicant }'})
                            .build(),
                    )
                    .build(),
            ])
            .build(),
    ])
    .build();

console.log(Specification.Workflow.toYaml(workflow));

you will see an error like the following:

  {
    instancePath: '/dataConditions/0',
    schemaPath: '#/additionalProperties',
    keyword: 'additionalProperties',
    params: { additionalProperty: 'normalize' },
    message: 'must NOT have additional properties'
  },

........                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                    

Error: Databasedswitch is invalid: /dataConditions/0 | #/additionalProperties | must NOT have additional properties
      data: {
    "type": "switch",
    "usedForCompensation": false,
    "name": "CheckApplication",
    "dataConditions": [
        {
            "condition": "${ .applicants | .age >= 18 }",
            "transition": "StartApplication"
        },
        {
            "condition": "${ .applicants | .age < 18 }",
            "transition": "RejectApplication"
        }
    ],
    "default": {
        "transition": "RejectApplication"
    }
}

Additional information (if needed):

antmendoza commented 3 years ago

Hi @JBBianchi , did you have the chance to look into this?

I have been reading through rollup documentation and plugins and I didn't find any option to generate the methods as plain functions instead of functions expressions

JBBianchi commented 3 years ago

@antmendoza I had a look yesterday but couldn't investigate that much. I must admit I don't really see where the difference lies. Maybe in one case the function/method is in the object prototype (no problem) and in another case the function/method is in the object directly (error). One dirty workaround might be to delete normalize() from the clone:

  normalize?(): Databasedswitch {
    const clone = new Databasedswitch(this);
    normalizeUsedForCompensationProperty(clone);
    normalizeOnErrorsProperty(clone);
    normalizeDataConditionsProperty(clone);
    delete clone.normalize;
    return clone;
  }

It also means normalize() will need to be checked before being called as it becomes nullable.

It's just an idea, it's probably not the solution.

antmendoza commented 3 years ago

thank you @JBBianchi ,

I have tested your solution and works fine, just it involves more changes:

if you think that this is a better solution I can change the code, no problem

tsurdilo commented 3 years ago

@antmendoza lgtm :)

JBBianchi commented 3 years ago

thank you @JBBianchi ,

I have tested your solution and works fine, just it involves more changes:

* invoke "normalize" in constructors for each property we are "hydrating"

* delete normalize property in the validate method as well.

if you think that this is a better solution I can change the code, no problem

Does the PR actually fixes the problem? I read I have changed the way we are deffinig "normalize" method to be able to reproduce the error in test. and understood it was "only" making the code consistent between dev & published versions. If the PR fixes the issue, then just ignore what I wrote and merge :)

antmendoza commented 3 years ago

understood it was "only" making the code consistent between dev & published versions

yes, those changes are not really needed. it can be reverted.

Does the PR actually fixes the problem?

Yes as far as I now.

I am using the sdk with this changes into other project and I am not facing the mentioned issue.

Also I have run the index.html example (I have to create a test here) that was having the same problem, since is using the same bundle, and it is working ok now.

JBBianchi commented 3 years ago

understood it was "only" making the code consistent between dev & published versions

yes, those changes are not really needed. it can be reverted.

Does the PR actually fixes the problem?

Yes as far as I now.

I am using the sdk with this changes into other project and I am not facing the mentioned issue.

Also I have run the index.html example (I have to create a test here) that was having the same problem, since is using the same bundle, and it is working ok now.

Ok, sorry I misunderstood. Then all green for me :) Thanks a lot 👍