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

[Question] What's the correct way to write cel expressions for validating a single item in a repeated field? #91

Closed
falau opened this issue Jan 18, 2024 · 1 comment · Fixed by #92
Labels
Bug Something isn't working

Comments

@falau
Copy link

falau commented Jan 18, 2024

Description

Hello, I'm updating protovalidate used in my Go/Java/Python applications to the latest versions, and encountered an inconsistent behavior, but I couldn't determine if it's a bug or it's an expected behavior as I can't find any example that resembles my use case.

My use case of cel validation is to apply string functions to validate a list of strings. For example, I used an expression similar to the message below to achieve what I need:

message TestRepeatedOne {
  repeated string paths = 1 [ (buf.validate.field).repeated = {
    items : {
      string : {min_len : 1},
      cel : {
        message : "leading spaces are not allowed",
        expression : "!this.startsWith(' ')",
      },
    },
  } ];
}

My understanding was that the variable this refers to each single item in a repeated field, so in this case I supposed it should be a string. This works well in protovalidate for Java/Python even with the latest releases, but not with protovalidate-go. The change happened in #80 is the cause of my issue here. With protovalidate-go >= v0.4.2, the following code produces an error on the type of the string function I want to apply.

bufValidator, _ := protovalidate.New()
testRepeatedOneMsg := &pb.TestRepeatedOne{Paths: []string{" mypath"}}
err := bufValidator.Validate(testRepeatedOneMsg)

The error is

compilation error: failed to compile items constraints for repeated mytest.TestRepeatedOne.paths: compilation error: failed to compile expression : ERROR: <input>:1:17: found no matching overload for 'startsWith' applied to 'list(string).(string)'

I briefly checked underlying code and thought the this variable here now resolves to the list itself rather than individual items, so tried this:

message TestRepeatedTwo {
  repeated string paths = 1 [ (buf.validate.field).repeated = {
    items : {
      string : {min_len : 1},
      cel : {
        message : "leading spaces are not allowed",
        expression : "this.all(x, !x.startsWith(' '))",
      },
    },
  } ];
}

However, it also gives error:

runtime error: error evaluating : got 'types.String', expected iterable type

so my assumption about the passed in type is wrong.

I know I can always use a message level validator like this:

message TestRepeatedPass {
  repeated string paths = 1 [ (buf.validate.field).repeated = {
    items : {
      string : {min_len : 1},
    },
  } ];
  option (buf.validate.message).cel = {
    id : "paths.spaces",
    message : "leading spaces are not allowed",
    expression : "this.paths.all(x, !x.startsWith(' '))"
  };
}

for the same validation. The validation errors produces from this approach however don't contain the information about which item in the list fails, while items.cel could hint it. As such, I'd still like to know what's the correct way to compose such expressions. I ask this question here instead of bufbuild/protovalidate-go is that I'm not sure this is a spec problem or a bug in the Go implementation. If my understanding in TestRepeatedOne is correct, then probably this counts a bug in the Go implementation, and I can move this issue there.

Environment

  • Operating System: macOS/Linux
  • Version: macOS 14.2.1 Sonoma/Ubuntu 22.04
  • Compiler/Toolchain: Go 1.21.6
  • Protobuf Compiler & Version: protoc-gen-go v1.32.0, protoc v4.25.2
  • Protovalidate Version: protovalidate-go >= v0.4.2 (tried v0.5.0)

Additional Context

The test results for the same messages on protovalidate-python and protovalidate-java are attached by the way:

  • [email protected]: All three messages passed, although it looks like that the expression in TestRepeatedTwo resolved this as a single string and the x in the all macro is actually characters in the string.
  • [email protected]: TestRepeatedTwo failed to compile(Failed to compile expression), but I guess that's expected. TestRepeatedOne works as expected.
@falau falau added the Bug Something isn't working label Jan 18, 2024
@rodaine
Copy link
Member

rodaine commented Jan 19, 2024

Hey, @falau! Thanks for the very detailed report. I'm going to move this to protovalidate-go as you actually caught a bug in how we construct the validations internally. You are correct that the issue is related to how the variable types are computed.

@rodaine rodaine transferred this issue from bufbuild/protovalidate Jan 19, 2024
rodaine added a commit that referenced this issue Jan 19, 2024
For repeated items, CEL types were being miscalculated for variables in
the expression resulting in type-check/compilation errors

Fixes #91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants