-
Notifications
You must be signed in to change notification settings - Fork 168
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
entproto/cmd/protoc-gen-entgrpc - infra for gRPC field validation #55
Conversation
Please use "squash and merge" when you land this and replace the Great work, and thanks for adding this 🙏 |
9d8be96
to
d654d65
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Minor comments.
checkIDFlag := "true" | ||
if op == "create" { | ||
checkIDFlag = "false" | ||
} | ||
|
||
if typeNeedsValidator(g.fieldMap) { | ||
g.Tmpl(`if err := validate%(typeName)(%(reqVar), %(checkIDFlag)); err != nil { | ||
return nil, %(statusErrf)(%(invalidArgument), "invalid argument: %s", err) | ||
}`, g.withGlobals(tmplValues{ | ||
"reqVar": reqVar, | ||
"checkIDFlag": checkIDFlag, | ||
})) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkIDFlag := "true" | |
if op == "create" { | |
checkIDFlag = "false" | |
} | |
if typeNeedsValidator(g.fieldMap) { | |
g.Tmpl(`if err := validate%(typeName)(%(reqVar), %(checkIDFlag)); err != nil { | |
return nil, %(statusErrf)(%(invalidArgument), "invalid argument: %s", err) | |
}`, g.withGlobals(tmplValues{ | |
"reqVar": reqVar, | |
"checkIDFlag": checkIDFlag, | |
})) | |
} | |
if typeNeedsValidator(g.fieldMap) { | |
g.Tmpl(`if err := validate%(typeName)(%(reqVar), %(checkIDFlag)); err != nil { | |
return nil, %(statusErrf)(%(invalidArgument), "invalid argument: %s", err) | |
}`, g.withGlobals(tmplValues{ | |
"reqVar": reqVar, | |
"checkIDFlag": strconv.FormatBool(op == "create"), | |
})) | |
} |
if d.IsEdgeField { | ||
return d.EdgeIDType() == field.TypeUUID | ||
} | ||
return d.EntField.IsUUID() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you use the EdgeIDType
method only here? Maybe you can do instead:
if d.IsEdgeField { | |
return d.EdgeIDType() == field.TypeUUID | |
} | |
return d.EntField.IsUUID() | |
f := d.EntField | |
if d.IsEdgeField { | |
f = d.EntEdge.Type.ID | |
} | |
return f.IsUUID() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
res, err := svc.client.User.Create(). | ||
SetBanned(bool(user.GetBanned())). | ||
SetCrmID(runtime.MustBytesToUUID(user.GetCrmId())). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you prefer to panic here instead of returning an error? Maybe it is worth mentioning this in the doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is guaranteed not to panic b/c a program that reaches here passed the validation code. In one of the next versions I will remove this "setter chain" and treat each field separately for supporting per field nullability etc. In that PR it might be easy to make it nicer as you suggest
Adding infra for validation on the gRPC layer + UUID field support (normal field, edge id, id)
Fixes ent/ent#1402