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

Removed "required" tag from failOnValidationErrors #192

Merged

Conversation

venera-program
Copy link
Contributor

What this PR does / why we need it:
Using the "required" tag on the DataInputSchema.FailOnValidationErrors field operates counter-intuitively to the go-playground docs. This also causes workflows to fail validation when the value of the field is manually set to false because the validator interprets this as the default/unset value.

Special notes for reviewers:
Not sure where to add a test for this case, need some insight on that front.

Additional information (if needed):
Go-playground doc reference: https://pkg.go.dev/github.com/go-playground/validator/v10#hdr-Required

This validates that the value is not the data types default zero value. For numbers ensures value is not zero. For strings ensures value is not "". For slices, maps, pointers, interfaces, channels and functions ensures the value is not nil. For structs ensures value is not the zero value when using WithRequiredStructEnabled.

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

Either we remove the required tag or keep this field as a pointer, so nil would be considered empty. Now the default will be false. That's fine. :)

@spolti
Copy link
Member

spolti commented Oct 3, 2023

+1 to keep as required and use pointer.

@ribeiromiranda
Copy link
Collaborator

ribeiromiranda commented Oct 3, 2023

@ricardozanini I agree to only removing the required tag, it is initialized by ApplyDefault, initially it is true and the other possibility is false. I think, it doesn't need to be a pointer.

func (d *DataInputSchema) ApplyDefault() {
	d.FailOnValidationErrors = true
}

@venera-program You can add a test in https://github.com/serverlessworkflow/sdk-go/blob/main/model/workflow_validator_test.go

@ricardozanini
Copy link
Member

@venera-program let's keep it as it is to simplify things rather than a pointer and please add a test case where @ribeiromiranda pointed out. 🙏

@venera-program
Copy link
Contributor Author

venera-program commented Oct 3, 2023

@ricardozanini I added a test that fails with the scenario caused by the old tag and a test parsing from a yaml source.

Edit: PR is done from my end.

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

@ribeiromiranda can u review?

@ribeiromiranda
Copy link
Collaborator

ribeiromiranda commented Oct 4, 2023

@ribeiromiranda can u review?

Review completed.

@ricardozanini ricardozanini merged commit 78b1c06 into serverlessworkflow:main Oct 4, 2023
6 checks passed
@ricardozanini
Copy link
Member

@venera-program let me know if you need a patch release or if there's more to add before we do a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants