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

Add data validation to all resources #1519

Merged
merged 7 commits into from
Nov 27, 2019
Merged

Add data validation to all resources #1519

merged 7 commits into from
Nov 27, 2019

Conversation

krhubert
Copy link
Contributor

close #1505

@krhubert krhubert added the enhancement New feature or request label Nov 20, 2019
@krhubert krhubert added this to the next milestone Nov 20, 2019
@krhubert krhubert self-assigned this Nov 20, 2019
protobuf/api/event.proto Outdated Show resolved Hide resolved
@@ -17,7 +21,55 @@ const (
envSeparator = "="
)

var envNameRegexp = regexp.MustCompile("^[a-zA-Z][a-zA-Z0-9_]*$")
// New returns a new instance of 'validate' with more validation fields.
Copy link
Member

@NicolasMahe NicolasMahe Nov 21, 2019

Choose a reason for hiding this comment

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

Could you also move the function validateServiceStruct from service validator package and make it generic:

func validateServiceStruct(s *service.Service) error {
var errs xerrors.Errors
// validate service struct based on tag
if err := validate.Struct(s); err != nil {
for _, e := range err.(validator.ValidationErrors) {
// Remove the name of the struct and the field from namespace
trimmedNamespace := strings.TrimPrefix(e.Namespace(), namespacePrefix)
trimmedNamespace = strings.TrimSuffix(trimmedNamespace, e.Field())
// Only use it when in-cascade field
namespace := ""
if e.Field() != trimmedNamespace {
namespace = trimmedNamespace
}
errs = append(errs, fmt.Errorf("%s%s", namespace, e.Translate(translator)))
}
}
return errs.ErrorOrNil()
}

to something like:

func Validate(s interface{}) error {
	 validate, translator := New()
	var errs xerrors.Errors
	// validate service struct based on tag
	if err := validate.Struct(s); err != nil {
		for _, e := range err.(validator.ValidationErrors) {
			// Remove the name of the struct and the field from namespace
			trimmedNamespace := strings.TrimPrefix(e.Namespace(), namespacePrefix)
			trimmedNamespace = strings.TrimSuffix(trimmedNamespace, e.Field())
			// Only use it when in-cascade field
			namespace := ""
			if e.Field() != trimmedNamespace {
				namespace = trimmedNamespace
			}
			errs = append(errs, fmt.Errorf("%s%s", namespace, e.Translate(translator)))
		}
	}
	return errs.ErrorOrNil()
}

I guess having a simple helper that improve the error and manage the translation will be better ;)

Copy link
Member

Choose a reason for hiding this comment

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

Also, the remaining functions in service/validator package should be move to service package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I left it in the service, because I'm not sure if we want such generic validation everywhere. It was designed for service only.

But I've moved the creation of Validate

@NicolasMahe NicolasMahe changed the title wip Add data validation on all resource Nov 21, 2019
@krhubert krhubert marked this pull request as ready for review November 21, 2019 18:03
@krhubert
Copy link
Contributor Author

I've left validation for cosmos app (backend), because it could be easily done when PR about execution #1463 will be merged.

Then validation will be done in the handler for every message. I haven't found middleware (or some similar concept) for cosmos handlers.

protobuf/api/event.proto Outdated Show resolved Hide resolved
@@ -31,12 +31,22 @@ service Execution {
// CreateExecutionRequest defines request to create a single execution.
message CreateExecutionRequest {
bytes instanceHash = 1 [
(gogoproto.moretags) = 'validate:"hash"',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(gogoproto.moretags) = 'validate:"hash"',
(gogoproto.moretags) = 'validate:"required,hash"',


// taskKey to filter executions.
string taskKey = 2 [
(gogoproto.moretags) = 'validate:"printascii"'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(gogoproto.moretags) = 'validate:"printascii"'
(gogoproto.moretags) = 'validate:"required,printascii"'

protobuf/api/execution.proto Outdated Show resolved Hide resolved
protobuf/api/execution.proto Outdated Show resolved Hide resolved
sdk/service/msgs.go Outdated Show resolved Hide resolved
sdk/service/msgs.go Outdated Show resolved Hide resolved
@@ -69,6 +71,13 @@ func (s *Server) Close() {
s.instance.GracefulStop()
}

func validateInterceptor(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's the best place here.
I would like the SDK to also check its inputs and a lot of grpc request message are directly forward to the sdk.
maybe check at both place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why SDK should check that. For now we use only grpc as entry for sdk. SDK transforms over time and for now, you just can't use it without running engine. So it should stay here for now. And when sdk transforms to the point where you can use it without engine, then we might add the validation here.

Copy link
Member

Choose a reason for hiding this comment

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

I just think that the verification should be done in last entry point of the app.
We also plan to move to SDK the definition of the message, so it will make even more sense to already put the verification in SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is the last entry point? For now, it is the cosmos app and there messages are validated. SDK is a kind of middleware between our grpc api and cosmos api.

We also plan to move to SDK the definition of the message,

Then we will move the validation as well, but for now let's keep it as it is.

x/xvalidator/validator.go Show resolved Hide resolved
x/xvalidator/validator.go Show resolved Hide resolved
@NicolasMahe
Copy link
Member

Then validation will be done in the handler for every message. I haven't found middleware (or some similar concept) for cosmos handlers.

each cosmos message has to implement func (msg msgCreateRunner) ValidateBasic() cosmostypes.Error. the validation should be done there.

@NicolasMahe NicolasMahe changed the title Add data validation on all resource Add data validation on to resource Nov 25, 2019
@NicolasMahe NicolasMahe changed the title Add data validation on to resource Add data validation to all resource Nov 25, 2019
@NicolasMahe NicolasMahe modified the milestones: v0.17.0, next Nov 25, 2019
@@ -54,7 +58,9 @@ message CreateEventRequest {
];
Copy link
Member

Choose a reason for hiding this comment

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

Also add required to instanceHash and data

Copy link
Member

Choose a reason for hiding this comment

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

in CreateEventRequest

e2e/execution_test.go Outdated Show resolved Hide resolved
Copy link
Member

@NicolasMahe NicolasMahe left a comment

Choose a reason for hiding this comment

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

please make the last modif and merge

@krhubert krhubert merged commit ae75410 into dev Nov 27, 2019
@krhubert krhubert deleted the feature/validation branch November 27, 2019 12:34
@NicolasMahe NicolasMahe changed the title Add data validation to all resource Add data validation to all resources Dec 13, 2019
@NicolasMahe NicolasMahe added the release:add Pull requests that add something label Dec 13, 2019
@NicolasMahe NicolasMahe mentioned this pull request Dec 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request release:add Pull requests that add something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add data validation to all resource
2 participants