Closed hbelmiro closed 3 months ago
the schemas in the spec are the source of truth what should be changed is that schema validation should use the schemas deployed on our website rather then internal: https://github.com/serverlessworkflow/sdk-java/blob/main/api/src/main/java/io/serverlessworkflow/api/validation/WorkflowSchemaLoader.java#L28
would suggest only making this change the schemas in sdk are only there for code generation and the required elements play no difference at all (you could remove all required elements completely and the code gen should work same. reason we have schemas in this sdk is only there cause of limitations by java code gen tools that exist and that we use
so i would not worry about keeping the schemas required elements in sync (maybe just remove them?), but update the schema validation code to use spec schemas instead
@tsurdilo Im glad you mention this, because there is an issue I wanted to discuss with you some time ago.
As you are aware, there are some POJOs that are generated from the schema and others that are hardcoded. That is the reason why the workflow.json is copied into the SDK.
In particular, the issue seems to be related with the handling of oneOf in the schema
For example, for Events
, in the spec we have
"events": {
"$ref": "events.json#/events"
},
and
"events": {
"oneOf": [
{
"type": "string",
"format": "uri",
"description": "URI to a resource containing event definitions (json or yaml)"
},
{
"type": "array",
"description": "Workflow CloudEvent definitions. Defines CloudEvents that can be consumed or produced",
"items": {
"type": "object",
"$ref": "#/definitions/eventdef"
},
"additionalItems": false,
"minItems": 1
}
]
}
And in the SDK we have
events": {
"type": "object",
"existingJavaType": "io.serverlessworkflow.api.workflow.Events",
"description": "Workflow event definitions"
}
and
public class Events {
private String refValue;
private List<EventDefinition> eventDefs;
public Events() {}
public Events(List<EventDefinition> eventDefs) {
this.eventDefs = eventDefs;
}
public Events(String refValue) {
this.refValue = refValue;
}
public String getRefValue() {
return refValue;
}
public void setRefValue(String refValue) {
this.refValue = refValue;
}
public List<EventDefinition> getEventDefs() {
return eventDefs;
}
public void setEventDefs(List<EventDefinition> eventDefs) {
this.eventDefs = eventDefs;
}
}
The problem is that the library we are using https://github.com/joelittlejohn/jsonschema2pojo does not handle oneOf
as a union like (like our generated classes) but as an Object
I would like to remove that limitation by improving the generator and use the spec files directly to generate the code we want to have.
So, we will not have any hardcoded POJO like the ones here https://github.com/serverlessworkflow/sdk-java/tree/main/api/src/main/java/io/serverlessworkflow/api/workflow. The idea is to change the generator to generate them from the spec (this implies either change the generator or use a custom one)
Since this change in the generator is not trivial , I want to achieve consensus before starting.
So, wdyt?
@fjtirado if you think we can improve the generation to use the spec schemas that sounds nice, maybe you can start this work in a separate branch and let's see where it takes us as I assume this might be a larger effort. Every SDK has its little things that are different which is imo ok. Limitations that are present due to gen tools I think can be masked very often by improving validation, so yes definitely what you propose now is imo a great idea but I don't think its something super urgent to do, so take your time no pressure at all. We can/should look at improving validation (not so much its current structure, but more so adding more validation points) to help users as well as again mask the current sdk limitations.
@tsurdilo Great, Ill try and see where it goes. The target of the effort is precisely to improve validation. If all POJOs are generated from the schema, then we can add javax validation annotations based on the schema in an uniform way, and, activating those annotations, we will have 95% of all the validation we need (implementation parsers can add their own, more complex, ones)
This is is the issue with oneOf https://github.com/joelittlejohn/jsonschema2pojo/issues/653 in the library we are using.
we are going to move schema validation to use spec schemas rather than the sdk ones, so these things i think won't be as big of an issue
@tsurdilo ok, whats exactly the plan? Currently the generated POJOS in the SDK already has the javax.annotations. The not generated ones does not.
the "non-generated" ones should be base classes only. maybe lets open a proposal for changes you want to do (open as issue if you want) and lets see what you have in mind and go from there?
Unfortunately, non generated ones are mostly concrete classes, check here for the complete list https://github.com/serverlessworkflow/sdk-java/tree/main/api/src/main/java/io/serverlessworkflow/api/workflow
As commented before this matches the schema types that are using oneOf
, which is not supported by https://github.com/joelittlejohn/jsonschema2pojo/issues/392
@tsurdilo ok, whats exactly the plan? Currently the generated POJOS in the SDK already has the javax.annotations. The not generated ones does not.
I am not 100%. sure right now. Thinking about long term solution atm. Evaluate different ways to generate object model from schema it one thing for sure.
I found some inconsistencies between the JSON schemas and the specification. Can you please clarify which one is correct so I can submit a fix?
Spec version: 0.8
workflow.json
id
key
not definedname
version
states/switchstate.json
default
defaultCondition
is required. Seems to be a typoswitchconditions/eventcondition.json
transition
transition
(x)orend
is requiredbranches/branch.json
actions
states/foreach.json
actions
events/onevents.json
actions
error/error.json
error
error
is not a propertytransition
transition
orend
is requiredretry/retrydef.json
maxAttempts
end/continueas.json
kind