Skip to content
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

Align the SDK with the new DSL #203

Open
spolti opened this issue Jun 3, 2024 · 19 comments
Open

Align the SDK with the new DSL #203

spolti opened this issue Jun 3, 2024 · 19 comments
Labels
enhancement 🙏 New feature or request

Comments

@spolti
Copy link
Member

spolti commented Jun 3, 2024

What would you like to be added:

The DSL has been updated to a new and concise version.

Why is this needed:
The SDK needs to support the new DSL, as found here: https://github.com/serverlessworkflow/specification/blob/main/dsl-reference.md

For more info: serverlessworkflow/specification#843

@spolti spolti added the enhancement 🙏 New feature or request label Jun 3, 2024
@ribeiromiranda
Copy link
Collaborator

What is your opinion on generating the SDK code based on the json schema?https://github.com/serverlessworkflow/specification/blob/main/schema/workflow.yaml

@ricardozanini
Copy link
Member

@ribeiromiranda +1 if we can, although in my experience, the workflow can be really complex and hard to model using generators. We have much OneOf and AnyOf that makes everything harder to model in Go.

@ribeiromiranda
Copy link
Collaborator

I understand, generating code can get very complex.

Another very interesting way to make a json schema interpreter with libraries: https://cuelang.org or https://github.com/xeipuuv/gojsonschema.

Is it simpler than generating the code and robust to changes in the specification.

@ricardozanini
Copy link
Member

+1, are you willing to give it a try?

@fjtirado
Copy link

fjtirado commented Jun 12, 2024

The DSL is still changing, so I would wait till is more stable. Probably next week will be a good moment to start working on that. Relying completely in code generation is going to be complex because the presence of oneOf/anyOf/allOf, so I foresee following approaches.

  1. Just provide validation that certain definition file is valid according to the schema (using https://github.com/xeipuuv/gojsonschema) . And maybe a shortcut call to load the generic Json object (load the map[string]interface{} as in here) representing the workflow on memory
  2. Generate once and add stuff to the generated pojos manually. The workflow is represented in memory as pojo. Problem of this approach is that when the schema change the pojos needs to be updated manually (either repeating the initial procedure or directly changing the hardcoded classes, depending on the magnitude of the change)
  3. Similar than 2, but generating every time the schema is changed. The anyOf/oneOf/allOf are handled by defining extra classes that inherits from the generated one. That way, depending on the schema change (basically if affects the xxxOf section or not) , it might be not required to perform any manual change. For the Java SDK, Im gonna try this one.

@ricardozanini
Copy link
Member

ricardozanini commented Jun 12, 2024

@fjtirado despite the stability of the DSL, it's worth starting to explore our possibilities first.

Regarding your points, unfortunately, Go doesn't play well with inheritance. Let's see what we can do given @ribeiromiranda suggestions. I agree that in the Java SDK option 3 can be a good alternative for the xOf approach.

@fjtirado
Copy link

oints, unfortunately, Go doesn't play well with inheritance. Let's see what we can do given @ribeiromiranda suggestions. I agree that in the Java SDK option 3 can be a good alternative for the xOf approach.

Yes, for option 3. in go, the option is to wrap the generated Pojo into another one https://www.geeksforgeeks.org/inheritance-in-golang/ (since GO allows calling the methods of the generated pojo within the container one, it will do the trick)

@ribeiromiranda
Copy link
Collaborator

I think these features below can be using jsonschema validation:

  • Parse workflow JSON and YAML definitions
  • Validate workflow definitions (Schema)
    • Custom error messages

Development the validation (e.g. map jsonschema into a graph):

  • Validate workflow definitions (Integrity)
  • Generate workflow diagram (SVG)

Generate code based in jsonschema

  • Programmatically build workflow definitions


I don't know if it has other features or I might be simplistic? I think there is no need to map jsonschema into struct. If everyone agrees, I can try to develop some simple cases.

A very simple validation example:

package main

import (
	_ "embed"
	"fmt"
	"log"
	"os"

	"github.com/xeipuuv/gojsonschema"
	"sigs.k8s.io/yaml"
)

//go:embed workflow.yaml
var yamlWorkflow []byte

func main() {

	jsonWorkflow, err := yaml.YAMLToJSON(yamlWorkflow)
	if err != nil {
		log.Fatal(err.Error())
	}

	yamlExample, err := os.ReadFile("./example.yaml")
	if err != nil {
		log.Fatal(err.Error())
	}
	jsonExample, err := yaml.YAMLToJSON(yamlExample)
	if err != nil {
		log.Fatal(err.Error())
	}

	schemaLoader := gojsonschema.NewBytesLoader(jsonWorkflow)
	documentLoader := gojsonschema.NewBytesLoader(jsonExample)

	result, err := gojsonschema.Validate(schemaLoader, documentLoader)
	if err != nil {
		log.Fatal(err.Error())
	}

	if result.Valid() {
		fmt.Printf("The document is valid\n")
	} else {
		fmt.Printf("The document is not valid. see errors :\n")
		for _, desc := range result.Errors() {
			fmt.Printf("- %s\n", desc)
		}
	}
}

@ricardozanini
Copy link
Member

oints, unfortunately, Go doesn't play well with inheritance. Let's see what we can do given @ribeiromiranda suggestions. I agree that in the Java SDK option 3 can be a good alternative for the xOf approach.

Yes, for option 3. in go, the option is to wrap the generated Pojo into another one https://www.geeksforgeeks.org/inheritance-in-golang/ (since GO allows calling the methods of the generated pojo within the container one, it will do the trick)

I know how to do inheritance in Go, but that's not the point. The point is this technique is an anti-pattern in general we avoid.

@ricardozanini
Copy link
Member

@ribeiromiranda yup, that's the overall features we will provide for the SDK. Generating diagrams can be a nice to have, but not a priority since we do not have this feature atm anyway.

@fjtirado
Copy link

@ribeiromiranda I think that the input to the validator should be just the workflow definition stream (an io.reader) being validated (the loading of the validating schema should be hidden to the developer)

@spolti
Copy link
Member Author

spolti commented Jun 25, 2024

just a reminder about it:

Just provide validation that certain definition file is valid according to the schema (using https://github.com/xeipuuv/gojsonschema) . And maybe a shortcut call to load the generic Json object (load the map[string]interface{} as in here) representing the workflow on memory

using generics with the SDK is a not good approach due the kubernetes integration. we had some problem related to it in the past.

@ribeiromiranda
Copy link
Collaborator

just a reminder about it:

Just provide validation that certain definition file is valid according to the schema (using https://github.com/xeipuuv/gojsonschema) . And maybe a shortcut call to load the generic Json object (load the map[string]interface{} as in here) representing the workflow on memory

using generics with the SDK is a not good approach due the kubernetes integration. we had some problem related to it in the past.

The internal representation as a graph for the resources:

  • Validate workflow definitions (Integrity)
  • Generate workflow diagram (SVG)
  • Programmatically build workflow definitions

I don't know how Kubernetes integration works, the need to represent it with a struct

Representation example:

package graph

import (
	"bytes"
	"encoding/json"
	"fmt"
	"strings"
)

type Node struct {
	value  interface{}
	edgeds map[string]*Node
}

func (n *Node) UnmarshalJSON(data []byte) error {
	data = bytes.TrimSpace(data)
	if data[0] == '{' {
		dataMap := map[string]json.RawMessage{}
		err := json.Unmarshal(data, &dataMap)
		if err != nil {
			return err
		}

		for key, val := range dataMap {
			node := NewNode()
			err := json.Unmarshal(val, &node)
			if err != nil {
				return err
			}
			n.edgeds[key] = node
		}

	} else if data[0] == '[' {
		dataMap := []json.RawMessage{}
		err := json.Unmarshal(data, &dataMap)
		if err != nil {
			return err
		}

		for i, val := range dataMap {
			key := fmt.Sprintf("%d", i)
			node := NewNode()
			err := json.Unmarshal(val, &node)
			if err != nil {
				return err
			}
			n.edgeds[key] = node
		}
	} else {
		return json.Unmarshal(data, &n.value)
	}

	return nil
}

func (n *Node) Lookup(path string) (*Node, error) {
	pathSplit := strings.Split(path, ".")
	edge := n

	walked := []string{}
	for _, key := range pathSplit {
		walked = append(walked, key)
		if val, ok := edge.edgeds[key]; ok {
			edge = val
		} else {
			return nil, fmt.Errorf("path not found: %s", strings.Join(walked, "."))
		}
	}
	return edge, nil
}

func NewNode() *Node {
	return &Node{
		edgeds: map[string]*Node{},
	}
}

@ricardozanini
Copy link
Member

I don't see an issue with this representation, @ribeiromiranda @spolti. And I agree that generics is a no-go. Too much compatibility issues with k8s libraries.

@ribeiromiranda
Copy link
Collaborator

I created the repository with a proposal to implement the new DSL, if I follow this implementation I will create a PR.

This repository implemented a basic "validator" using jsonschema and "builder", in the file "example/example.go" there are some use cases.

https://github.com/galgotech/sdk-go-1.0

@ricardozanini
Copy link
Member

ricardozanini commented Aug 6, 2024

@ribeiromiranda sorry for the late reply, I was on PTO.
I liked the approach, but I only scanned the repo. I think it would be useful to send the PR so we can discuss/have a throughout review.

To me, it seems aligned with what we talked about.

@ricardozanini
Copy link
Member

One little thing: this is supposed to be v4, not v3 since we are skipping 0.9 release.

Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Copy link

github-actions bot commented Nov 8, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🙏 New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants