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

allow empty auth definitions in parser #111

Merged
merged 5 commits into from
Nov 14, 2022

Conversation

spolti
Copy link
Member

@spolti spolti commented Oct 20, 2022

fixes #110

Signed-off-by: spolti filippespolti@gmail.com

Many thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:

Special notes for reviewers:

Additional information (if needed):

model/auth.go Outdated Show resolved Hide resolved
model/auth.go Outdated Show resolved Hide resolved
@lsytj0413
Copy link
Collaborator

lsytj0413 commented Oct 21, 2022

The problem is not cause by AuthDefinitions.Unmarshal implement, according to the specificaiton, auth field is an array of object, which like:
image

But in the source code, we place an Defs field under Auth Definition struct. So after we marshal object to json, it looks like:

{
	"id": "applicantrequest",
	"name": "Applicant Request Decision Workflow",
	"description": "Determine if applicant request is valid",
	"version": "1.0",
	"start": {
		"stateName": "CheckApplication"
	},
	"specVersion": "0.7",
	"expressionLang": "jq",
	"auth": {
		"Defs": [{
			"name": "testAuth",
			"scheme": "bearer",
			"properties": {
				"token": "test_token"
			}
		}, {
			"name": "testAuth2",
			"scheme": "basic",
			"properties": {
				"username": "test_user",
				"password": "test_pwd"
			}
		}]
	},
	"states": [{
		"name": "CheckApplication",
		"type": "switch",
		"defaultCondition": {},
		"dataConditions": [{
			"condition": "${ .applicants | .age \u003e= 18 }",
			"transition": {
				"nextState": "StartApplication"
			}
		}, {
			"condition": "${ .applicants | .age \u003c 18 }",
			"transition": {
				"nextState": "RejectApplication"
			}
		}],
		"timeouts": {}
	}, {
		"name": "StartApplication",
		"type": "operation",
		"end": {
			"terminate": true
		},
		"actions": [{
			"subFlowRef": {
				"workflowId": "startApplicationWorkflowId",
				"invoke": "sync",
				"onParentComplete": "terminate"
			},
			"sleep": {},
			"actionDataFilter": {
				"useResults": true
			}
		}]
	}, {
		"name": "RejectApplication",
		"type": "operation",
		"end": {
			"terminate": true
		},
		"actionMode": "sequential",
		"actions": [{
			"functionRef": {
				"refName": "sendRejectionEmailFunction",
				"invoke": "sync"
			},
			"sleep": {},
			"actionDataFilter": {
				"useResults": true
			}
		}]
	}],
	"functions": [{
		"name": "sendRejectionEmailFunction",
		"operation": "http://myapis.org/applicationapi.json#emailRejection"
	}],
	"retries": [{
		"name": "TimeoutRetryStrategy",
		"delay": "PT1M",
		"maxAttempts": "5",
		"jitter": 0
	}]
}

It's not equivalent. I think we should fix the struct field layout.

@spolti spolti mentioned this pull request Oct 24, 2022
@spolti
Copy link
Member Author

spolti commented Oct 24, 2022

needed for v08 #70

model/auth.go Outdated Show resolved Hide resolved
fixes serverlessworkflow#110

Signed-off-by: spolti <filippespolti@gmail.com>
@spolti
Copy link
Member Author

spolti commented Oct 26, 2022

About the defs problem, there is another problem as well, see:

https://gist.github.com/spolti/ac43ce2cc5a51532987259db4f9fa5b7
Will address it on another issue: #126

model/auth.go Outdated Show resolved Hide resolved
@@ -108,7 +108,7 @@ type BaseWorkflow struct {
// Auth definitions can be used to define authentication information that should be applied to resources defined in the operation
// property of function definitions. It is not used as authentication information for the function invocation,
// but just to access the resource containing the function invocation information.
Auth AuthDefinitions `json:"auth,omitempty"`
Auth *AuthDefinitions `json:"auth,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the specification, should we directly change this field's type to []Auth?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, tried already, the problem is, if we do that we can't have strings :)
maybe declaring it as interface might help, let's see..

Copy link
Collaborator

@lsytj0413 lsytj0413 Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do it in this way.

// First, we redefine the []Auth
type AuthArray []Auth

// Second, change struct field
Auth AuthArray

// Third, define UnmarshalJSON for AuthArray, and implement it
func (a *AuthArray) UnmarshalJSON(data []byte) error {
// Unmarshal it as string or array
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that could work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to get it working with this approach, however with interface it is fully working, including the fix for this issue and #126

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spolti spolti force-pushed the nilAuth branch 2 times, most recently from 0174926 to 659f342 Compare October 28, 2022 19:56
fixes serverlessworkflow#110
fixes serverlessworkflow#126

Signed-off-by: spolti <filippespolti@gmail.com>
Signed-off-by: lsytj0413 <511121939@qq.com>
model/workflow.go Outdated Show resolved Hide resolved
fix(*): use different types to auth validate
model/workflow.go Outdated Show resolved Hide resolved
Signed-off-by: spolti <filippespolti@gmail.com>
@lsytj0413
Copy link
Collaborator

/lgtm

@ricardozanini ricardozanini merged commit 02fbe11 into serverlessworkflow:main Nov 14, 2022
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.

Valid parsed workflow cannot be unmarshalled after being marshalled to JSON
3 participants