Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[BUG] CelValue runtime path mishandles fixed32/fixed64 and FieldMask field access #1075

Copy link
Copy link

Description

@andrewparmet
Issue body actions

Describe the bug

My motivation is to build support for protokt (alternate protobuf codegen + runtime) in protovalidate-java, which uses CEL.

I think I've encountered two independent bugs in the planner runtime; both surface when CEL navigates a com.google.protobuf.Message whose conversion goes through ProtoCelValueConverter:

  1. proto fixed32/fixed64 fields resolve as signed int, breaking uint overloads: the CEL spec maps proto fixed32/fixed64 to CEL uint, and the CelValue path via ProtoCelValueConverter.fromProtoMessageFieldToCelValue does not: it has cases for UINT32/UINT64 but not FIXED32/FIXED64, so those fall through to normalizePrimitive and surface as Integer/Long.

  2. FieldMask-typed fields can't be navigated as messages: BaseProtoCelValueConverter.fromWellKnownProto converts FieldMask to a comma-separated string of its paths, which is different from cel-go and cel-java's ProtoLiteAdapter.adaptValueToWellKnownProto.

To Reproduce
Check which components this affects:

  • parser
  • checker
  • runtime

Reproducers (in Kotlin to get dependencies inline):

  1. @file:Repository("https://repo1.maven.org/maven2")
    @file:DependsOn("dev.cel:cel:0.13.0")
    @file:DependsOn("com.google.protobuf:protobuf-java:4.30.1")
    
    import com.google.protobuf.DescriptorProtos.DescriptorProto
    import com.google.protobuf.DescriptorProtos.FieldDescriptorProto
    import com.google.protobuf.DescriptorProtos.FileDescriptorProto
    import com.google.protobuf.Descriptors.FileDescriptor
    import com.google.protobuf.DynamicMessage
    import dev.cel.bundle.CelFactory
    import dev.cel.common.types.StructTypeReference
    
    // A minimal one-field message: `Msg { fixed32 f = 1; }`, built from a runtime
    // descriptor so the reproducer needs no .proto compilation.
    val fileProto =
        FileDescriptorProto.newBuilder()
            .setName("repro.proto")
            .setSyntax("proto3")
            .addMessageType(
                DescriptorProto.newBuilder()
                    .setName("Msg")
                    .addField(
                        FieldDescriptorProto.newBuilder()
                            .setName("f")
                            .setNumber(1)
                            .setType(FieldDescriptorProto.Type.TYPE_FIXED32)
                            .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL)
                    )
            )
            .build()
    
    val fileDescriptor = FileDescriptor.buildFrom(fileProto, arrayOf())
    val msgDescriptor = fileDescriptor.findMessageTypeByName("Msg")
    
    val cel =
        CelFactory.plannerCelBuilder()
            .addMessageTypes(msgDescriptor)
            .addVar("msg", StructTypeReference.create("Msg"))
            .build()
    
    // The checker types a proto fixed32 field as CEL `uint`; `msg.f > 5u` type-checks.
    val ast = cel.compile("msg.f > 5u").ast
    val program = cel.createProgram(ast)
    
    val msg =
        DynamicMessage.newBuilder(msgDescriptor)
            .setField(msgDescriptor.findFieldByNumber(1), 6)
            .build()
    
    val result = program.eval(mapOf("msg" to msg))
    println("msg.f > 5u  ->  $result")
    check(result == true) { "expected true" }
    println("OK")

    Throws dev.cel.runtime.CelEvaluationException: evaluation error at <input>:6: No matching overload for function '_>_'. Overload candidates: greater_uint64.. Potential fix: Handle FIXED32/FIXED64 as unsigned in ProtoCelValueConverter #1073

  2. @file:Repository("https://repo1.maven.org/maven2")
    @file:DependsOn("dev.cel:cel:0.13.0")
    @file:DependsOn("com.google.protobuf:protobuf-java:4.30.1")
    
    import com.google.protobuf.DescriptorProtos.DescriptorProto
    import com.google.protobuf.DescriptorProtos.FieldDescriptorProto
    import com.google.protobuf.DescriptorProtos.FileDescriptorProto
    import com.google.protobuf.Descriptors.FileDescriptor
    import com.google.protobuf.DynamicMessage
    import com.google.protobuf.FieldMask
    import dev.cel.bundle.CelFactory
    import dev.cel.common.types.StructTypeReference
    
    // A minimal one-field message: `Msg { google.protobuf.FieldMask m = 1; }`.
    val fileProto =
        FileDescriptorProto.newBuilder()
            .setName("repro.proto")
            .setSyntax("proto3")
            .addDependency("google/protobuf/field_mask.proto")
            .addMessageType(
                DescriptorProto.newBuilder()
                    .setName("Msg")
                    .addField(
                        FieldDescriptorProto.newBuilder()
                            .setName("m")
                            .setNumber(1)
                            .setType(FieldDescriptorProto.Type.TYPE_MESSAGE)
                            .setTypeName(".google.protobuf.FieldMask")
                            .setLabel(FieldDescriptorProto.Label.LABEL_OPTIONAL)
                    )
            )
            .build()
    
    val fileDescriptor =
        FileDescriptor.buildFrom(fileProto, arrayOf(FieldMask.getDescriptor().file))
    val msgDescriptor = fileDescriptor.findMessageTypeByName("Msg")
    
    val cel =
        CelFactory.plannerCelBuilder()
            .addMessageTypes(msgDescriptor)
            .addVar("msg", StructTypeReference.create("Msg"))
            .build()
    
    // `msg.m.paths` selects the repeated `paths` field of the FieldMask.
    val ast = cel.compile("msg.m.paths").ast
    val program = cel.createProgram(ast)
    
    val msg =
        DynamicMessage.newBuilder(msgDescriptor)
            .setField(
                msgDescriptor.findFieldByNumber(1),
                FieldMask.newBuilder().addPaths("a").addPaths("b").build()
            )
            .build()
    
    val result = program.eval(mapOf("msg" to msg))
    println("msg.m.paths  ->  $result")
    check(result == listOf("a", "b")) { "expected [a, b]" }
    println("OK")

    Throws dev.cel.runtime.CelEvaluationException: evaluation error at <input>:5: Error resolving field 'paths'. Field selections must be performed on messages or maps.. Potential fix: Preserve FieldMask as a message in the CelValue runtime path #1074

Additional context
For the full context see bufbuild/protovalidate-java#202 and open-toast/protokt#402

Reactions are currently unavailable

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Morty Proxy This is a proxified and sanitized view of the page, visit original site.