Skip to content

Change to not initialize collections by default on generated POJOs #206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

hbelmiro
Copy link
Contributor

Fixes #204.

Changes:

  • Set initializeCollections jsonschema2pojo property to false
  • Added test
  • Fixed test that started to fail after setting initializeCollections to false

@tsurdilo
Copy link
Contributor

Can you show what the resulting json (Workflow.toJson(workflow)) looks like with this change?

@hbelmiro
Copy link
Contributor Author

When initializeCollections = true (current code):

{
  "id" : "functionrefs",
  "name" : "Customer Credit Check Workflow",
  "description" : "Perform Customer Credit Check",
  "version" : "1.0",
  "start" : "TestFunctionRef",
  "specVersion" : "0.8",
  "expressionLang" : "jq",
  "events" : [ ],
  "functions" : [ {
    "name" : "creditCheckFunction",
    "operation" : "http://myapis.org/creditcheckapi.json#doCreditCheck",
    "type" : "rest"
  }, {
    "name" : "sendRejectionEmailFunction",
    "operation" : "http://myapis.org/creditcheckapi.json#rejectionEmail",
    "type" : "rest"
  } ],
  "retries" : [ ],
  "errors" : [ ],
  "secrets" : [ ],
  "states" : [ {
    "actionMode" : "sequential",
    "actions" : [ {
      "functionRef" : "creditCheckFunction"
    }, {
      "functionRef" : {
        "refName" : "sendRejectionEmailFunction",
        "arguments" : {
          "applicant" : "${ .customer }"
        },
        "invoke" : "sync"
      }
    } ],
    "usedForCompensation" : false,
    "name" : "TestFunctionRefs",
    "type" : "operation",
    "end" : true
  } ]
}

When initializeCollections = false (this change):

{
  "id" : "functionrefs",
  "name" : "Customer Credit Check Workflow",
  "description" : "Perform Customer Credit Check",
  "version" : "1.0",
  "start" : "TestFunctionRef",
  "specVersion" : "0.8",
  "events" : [ ],
  "functions" : [ {
    "name" : "creditCheckFunction",
    "operation" : "http://myapis.org/creditcheckapi.json#doCreditCheck",
    "type" : "rest"
  }, {
    "name" : "sendRejectionEmailFunction",
    "operation" : "http://myapis.org/creditcheckapi.json#rejectionEmail",
    "type" : "rest"
  } ],
  "retries" : [ ],
  "errors" : [ ],
  "secrets" : [ ],
  "states" : [ {
    "actionMode" : "sequential",
    "actions" : [ {
      "functionRef" : "creditCheckFunction"
    }, {
      "functionRef" : {
        "refName" : "sendRejectionEmailFunction",
        "arguments" : {
          "applicant" : "${ .customer }"
        },
        "invoke" : "sync"
      }
    } ],
    "usedForCompensation" : false,
    "name" : "TestFunctionRefs",
    "type" : "operation",
    "end" : true
  } ]
}

There's only one difference: "expressionLang" : "jq" didn't appear in the json when initializeCollections = false
This shouldn't happen. If we'll go on with this change, I'll have to investigate it. But this is a different issue.
The most important thing, for what we're discussing, is that annotations doesn't appear in json no matter the value of initializeCollection. This behavior is correct when initializeCollections = false (this change), since Workflow#getAnnotations returns null. But when initializeCollections = true, Workflow#getAnnotations returns an empty list. So an empty array should appear in the resulting json. It looks like the json parser ignores the attribute if it's null or empty.

@tsurdilo
Copy link
Contributor

if (workflow.getAnnotations() != null && !workflow.getAnnotations().isEmpty()) {

@tsurdilo
Copy link
Contributor

if its null or empty it should not be spit out as json/yaml

@tsurdilo
Copy link
Contributor

we need tests that round-trip markup it seems ...will add

@hbelmiro hbelmiro marked this pull request as draft June 27, 2022 11:48
@hbelmiro
Copy link
Contributor Author

Reverted to draft to investigate the jq issue.

@tsurdilo
Copy link
Contributor

tsurdilo commented Jun 27, 2022

There's only one difference: "expressionLang" : "jq" didn't appear in the json

jq is the default value if its not in json/yaml so this should be ok imo

the markup that sdk spits out from object model imo does not 100% have to match the original user markup (for example default prop values and empty arrays can be omitted) , but it has to be semantically the same (and it has to be validatable if original was too)

@hbelmiro
Copy link
Contributor Author

There's only one difference: "expressionLang" : "jq" didn't appear in the json

jq is the default value if its not in json/yaml so this should be ok imo

the markup that sdk spits out from object model imo does not 100% have to match the original user markup (for example default prop values and empty arrays can be omitted) , but it has to be semantically the same (and it has to be validatable if original was too)

Yes. Makes sense.

@hbelmiro hbelmiro marked this pull request as ready for review June 27, 2022 17:09
@hbelmiro
Copy link
Contributor Author

we need tests that round-trip markup it seems ...will add

@tsurdilo Just to clarify, you need to add these tests before we move on with this PR?

@tsurdilo
Copy link
Contributor

we need tests that round-trip markup it seems ...will add

@tsurdilo Just to clarify, you need to add these tests before we move on with this PR?

Yes, I'm updating all tests to make sure the json+yaml outputs are validated. This could help us figure out all possible issues the code might have right now and then we can look how to handle them. Hope thats ok with you.

@hbelmiro
Copy link
Contributor Author

@tsurdilo did you manage to update the tests you mention above so we can move this PR forward?
If you need any help with that, just let me know.

@hbelmiro
Copy link
Contributor Author

hbelmiro commented Apr 4, 2023

@manick02 do you mind taking a look at this PR? I'm not sure if @tsurdilo is seeing my messages.
If you have any questions or need any clarification on this, please don't hesitate to ask me.

@tsurdilo tsurdilo merged commit 88590ec into serverlessworkflow:main Apr 4, 2023
@hbelmiro hbelmiro deleted the initialized-collections branch April 5, 2023 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

POJOs being generated with initialized collections by default
2 participants