Switch to `dev.cel:cel` by srikrsna-buf · Pull Request #303 · bufbuild/protovalidate-java

srikrsna-buf · 2025-06-06T07:03:24Z

Switch to Google's CEL implementation.

Type checking is stricter, which meant:
- Converting to CEL types (Long, UnsignedLong, Double, etc...) before passing it to the runtime. Eg: Floats and Double are different for something as simple as - operator and using Floats doesn't work with any function.
- Changing zero values to CEL supported ones.
- Declaring separate function overloads based on type. Resulting in a slight increase in function definitions.
Functions bindings are based on java types.
Partial evaluation is not well documented so it is not implemented in the PR. Also unsure if it is working on main, more on this in a comment.
Global variables per program/runtime are not supported. All the variables need to be passed to eval.

Deliberately didn't refactor to keep relatively easier to review.

srikrsna-buf · 2025-06-06T07:58:45Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


For google.rpc.Status

Where is that used?

In some of the cel protos we generate code for

srikrsna-buf · 2025-06-06T09:27:55Z

+standard_rules/bytes:
+ - pattern/invalid/not_utf8
+# input: [ type.googleapis.com/buf.validate.conformance.cases.BytesPattern ]:{val:"\x99"}
+# want: runtime error: value must be valid UTF-8 to apply regexp
+# got: validation error (1 violation)
+# 1. rule_id: "bytes.pattern"
+# message: "value must match regex pattern `^[\\x00-\\x7F]+$`"
+# field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_BYTES}
+# rule: "bytes.pattern" elements:{field_number:15 field_name:"bytes" field_type:TYPE_MESSAGE} elements:{field_number:4 field_name:"pattern" field_type:TYPE_STRING}


Can be solved if we override the bytes_to_string: #304

srikrsna-buf · 2025-06-06T09:36:19Z

- Program program = ruleEnv.program(rule.astExpression.ast, globals, PARTIAL_EVAL_OPTIONS);
- Program.EvalResult evalResult = program.eval(Activation.emptyActivation());
- Val value = evalResult.getVal();
- if (value != null) {
- Object val = value.value();
- if (val instanceof Boolean && value.booleanValue()) {
- continue;
- }
- if (val instanceof String && val.equals("")) {
- continue;
- }
- }
- Ast residual = ruleEnv.residualAst(rule.astExpression.ast, evalResult.getEvalDetails());
- programs.add(
- new CompiledProgram(
- ruleEnv.program(residual, globals),
- rule.astExpression.source,
- rule.rulePath,
- ruleValue));


From an internal feature standpoint this is the only step we skipped.
I am also not sure if it ever worked. It seems we have to tell which variables/attrs are not available by passing a PartialActivation type to eval. See here.

timostamm

Looks great so far, but I wasn't able to review everything in this pass.

timostamm · 2025-06-06T14:05:12Z

+/**
+ * CEL supports protobuf natively but when we pass it field values (like scalars, repeated, and
+ * maps) it has no way to treat them like a proto message field. This class has methods to convert
+ * to a cel values.
+ */
+final class ProtoAdapter {


timostamm · 2025-06-06T14:07:52Z

+ * for accessing the variable `now` that's constant within an evaluation.
 */
-class NowVariable implements Activation {
+class NowVariable implements CelVariableResolver {


IIRC, we had a bug early on where now was cached for a validator instance, instead of for a validation pass. The conformance tests can't catch it. Would be good to be sure we don't regress here.

I did not know that. I checked the references for NowVariable class, it is only created here alongside this variable.

Found the issue: #185

Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 · 2025-06-06T19:07:37Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


Where is that used?

ghost

LGTM. I think you could also roll #304 into this too, if you wanted, but fine either way.

Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf · 2025-06-06T07:03:24Z

Switch to Google's CEL implementation.

Type checking is stricter, which meant:
- Converting to CEL types (Long, UnsignedLong, Double, etc...) before passing it to the runtime. Eg: Floats and Double are different for something as simple as - operator and using Floats doesn't work with any function.
- Changing zero values to CEL supported ones.
- Declaring separate function overloads based on type. Resulting in a slight increase in function definitions.
Functions bindings are based on java types.
Partial evaluation is not well documented so it is not implemented in the PR. Also unsure if it is working on main, more on this in a comment.
Global variables per program/runtime are not supported. All the variables need to be passed to eval.

Deliberately didn't refactor to keep relatively easier to review.

srikrsna-buf · 2025-06-06T07:58:45Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


For google.rpc.Status

Where is that used?

In some of the cel protos we generate code for

srikrsna-buf · 2025-06-06T09:27:55Z

+standard_rules/bytes:
+ - pattern/invalid/not_utf8
+# input: [ type.googleapis.com/buf.validate.conformance.cases.BytesPattern ]:{val:"\x99"}
+# want: runtime error: value must be valid UTF-8 to apply regexp
+# got: validation error (1 violation)
+# 1. rule_id: "bytes.pattern"
+# message: "value must match regex pattern `^[\\x00-\\x7F]+$`"
+# field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_BYTES}
+# rule: "bytes.pattern" elements:{field_number:15 field_name:"bytes" field_type:TYPE_MESSAGE} elements:{field_number:4 field_name:"pattern" field_type:TYPE_STRING}


Can be solved if we override the bytes_to_string: #304

srikrsna-buf · 2025-06-06T09:36:19Z

- Program program = ruleEnv.program(rule.astExpression.ast, globals, PARTIAL_EVAL_OPTIONS);
- Program.EvalResult evalResult = program.eval(Activation.emptyActivation());
- Val value = evalResult.getVal();
- if (value != null) {
- Object val = value.value();
- if (val instanceof Boolean && value.booleanValue()) {
- continue;
- }
- if (val instanceof String && val.equals("")) {
- continue;
- }
- }
- Ast residual = ruleEnv.residualAst(rule.astExpression.ast, evalResult.getEvalDetails());
- programs.add(
- new CompiledProgram(
- ruleEnv.program(residual, globals),
- rule.astExpression.source,
- rule.rulePath,
- ruleValue));


From an internal feature standpoint this is the only step we skipped.
I am also not sure if it ever worked. It seems we have to tell which variables/attrs are not available by passing a PartialActivation type to eval. See here.

timostamm

Looks great so far, but I wasn't able to review everything in this pass.

timostamm · 2025-06-06T14:05:12Z

+/**
+ * CEL supports protobuf natively but when we pass it field values (like scalars, repeated, and
+ * maps) it has no way to treat them like a proto message field. This class has methods to convert
+ * to a cel values.
+ */
+final class ProtoAdapter {


timostamm · 2025-06-06T14:07:52Z

+ * for accessing the variable `now` that's constant within an evaluation.
 */
-class NowVariable implements Activation {
+class NowVariable implements CelVariableResolver {


IIRC, we had a bug early on where now was cached for a validator instance, instead of for a validation pass. The conformance tests can't catch it. Would be good to be sure we don't regress here.

I did not know that. I checked the references for NowVariable class, it is only created here alongside this variable.

Found the issue: #185

Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 · 2025-06-06T19:07:37Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


Where is that used?

ghost

LGTM. I think you could also roll #304 into this too, if you wanted, but fine either way.

Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf · 2025-06-06T07:03:24Z

Switch to Google's CEL implementation.

Type checking is stricter, which meant:
- Converting to CEL types (Long, UnsignedLong, Double, etc...) before passing it to the runtime. Eg: Floats and Double are different for something as simple as - operator and using Floats doesn't work with any function.
- Changing zero values to CEL supported ones.
- Declaring separate function overloads based on type. Resulting in a slight increase in function definitions.
Functions bindings are based on java types.
Partial evaluation is not well documented so it is not implemented in the PR. Also unsure if it is working on main, more on this in a comment.
Global variables per program/runtime are not supported. All the variables need to be passed to eval.

Deliberately didn't refactor to keep relatively easier to review.

srikrsna-buf · 2025-06-06T07:58:45Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


For google.rpc.Status

Where is that used?

In some of the cel protos we generate code for

srikrsna-buf · 2025-06-06T09:27:55Z

+standard_rules/bytes:
+ - pattern/invalid/not_utf8
+# input: [ type.googleapis.com/buf.validate.conformance.cases.BytesPattern ]:{val:"\x99"}
+# want: runtime error: value must be valid UTF-8 to apply regexp
+# got: validation error (1 violation)
+# 1. rule_id: "bytes.pattern"
+# message: "value must match regex pattern `^[\\x00-\\x7F]+$`"
+# field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_BYTES}
+# rule: "bytes.pattern" elements:{field_number:15 field_name:"bytes" field_type:TYPE_MESSAGE} elements:{field_number:4 field_name:"pattern" field_type:TYPE_STRING}


Can be solved if we override the bytes_to_string: #304

srikrsna-buf · 2025-06-06T09:36:19Z

- Program program = ruleEnv.program(rule.astExpression.ast, globals, PARTIAL_EVAL_OPTIONS);
- Program.EvalResult evalResult = program.eval(Activation.emptyActivation());
- Val value = evalResult.getVal();
- if (value != null) {
- Object val = value.value();
- if (val instanceof Boolean && value.booleanValue()) {
- continue;
- }
- if (val instanceof String && val.equals("")) {
- continue;
- }
- }
- Ast residual = ruleEnv.residualAst(rule.astExpression.ast, evalResult.getEvalDetails());
- programs.add(
- new CompiledProgram(
- ruleEnv.program(residual, globals),
- rule.astExpression.source,
- rule.rulePath,
- ruleValue));


From an internal feature standpoint this is the only step we skipped.
I am also not sure if it ever worked. It seems we have to tell which variables/attrs are not available by passing a PartialActivation type to eval. See here.

timostamm

Looks great so far, but I wasn't able to review everything in this pass.

timostamm · 2025-06-06T14:05:12Z

+/**
+ * CEL supports protobuf natively but when we pass it field values (like scalars, repeated, and
+ * maps) it has no way to treat them like a proto message field. This class has methods to convert
+ * to a cel values.
+ */
+final class ProtoAdapter {


timostamm · 2025-06-06T14:07:52Z

+ * for accessing the variable `now` that's constant within an evaluation.
 */
-class NowVariable implements Activation {
+class NowVariable implements CelVariableResolver {


IIRC, we had a bug early on where now was cached for a validator instance, instead of for a validation pass. The conformance tests can't catch it. Would be good to be sure we don't regress here.

I did not know that. I checked the references for NowVariable class, it is only created here alongside this variable.

Found the issue: #185

Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 · 2025-06-06T19:07:37Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


Where is that used?

ghost

LGTM. I think you could also roll #304 into this too, if you wanted, but fine either way.

Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf · 2025-06-06T07:03:24Z

Switch to Google's CEL implementation.

Type checking is stricter, which meant:
- Converting to CEL types (Long, UnsignedLong, Double, etc...) before passing it to the runtime. Eg: Floats and Double are different for something as simple as - operator and using Floats doesn't work with any function.
- Changing zero values to CEL supported ones.
- Declaring separate function overloads based on type. Resulting in a slight increase in function definitions.
Functions bindings are based on java types.
Partial evaluation is not well documented so it is not implemented in the PR. Also unsure if it is working on main, more on this in a comment.
Global variables per program/runtime are not supported. All the variables need to be passed to eval.

Deliberately didn't refactor to keep relatively easier to review.

srikrsna-buf · 2025-06-06T07:58:45Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


For google.rpc.Status

Where is that used?

In some of the cel protos we generate code for

srikrsna-buf · 2025-06-06T09:27:55Z

+standard_rules/bytes:
+ - pattern/invalid/not_utf8
+# input: [ type.googleapis.com/buf.validate.conformance.cases.BytesPattern ]:{val:"\x99"}
+# want: runtime error: value must be valid UTF-8 to apply regexp
+# got: validation error (1 violation)
+# 1. rule_id: "bytes.pattern"
+# message: "value must match regex pattern `^[\\x00-\\x7F]+$`"
+# field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_BYTES}
+# rule: "bytes.pattern" elements:{field_number:15 field_name:"bytes" field_type:TYPE_MESSAGE} elements:{field_number:4 field_name:"pattern" field_type:TYPE_STRING}


Can be solved if we override the bytes_to_string: #304

srikrsna-buf · 2025-06-06T09:36:19Z

- Program program = ruleEnv.program(rule.astExpression.ast, globals, PARTIAL_EVAL_OPTIONS);
- Program.EvalResult evalResult = program.eval(Activation.emptyActivation());
- Val value = evalResult.getVal();
- if (value != null) {
- Object val = value.value();
- if (val instanceof Boolean && value.booleanValue()) {
- continue;
- }
- if (val instanceof String && val.equals("")) {
- continue;
- }
- }
- Ast residual = ruleEnv.residualAst(rule.astExpression.ast, evalResult.getEvalDetails());
- programs.add(
- new CompiledProgram(
- ruleEnv.program(residual, globals),
- rule.astExpression.source,
- rule.rulePath,
- ruleValue));


From an internal feature standpoint this is the only step we skipped.
I am also not sure if it ever worked. It seems we have to tell which variables/attrs are not available by passing a PartialActivation type to eval. See here.

timostamm

Looks great so far, but I wasn't able to review everything in this pass.

timostamm · 2025-06-06T14:05:12Z

+/**
+ * CEL supports protobuf natively but when we pass it field values (like scalars, repeated, and
+ * maps) it has no way to treat them like a proto message field. This class has methods to convert
+ * to a cel values.
+ */
+final class ProtoAdapter {


timostamm · 2025-06-06T14:07:52Z

+ * for accessing the variable `now` that's constant within an evaluation.
 */
-class NowVariable implements Activation {
+class NowVariable implements CelVariableResolver {


IIRC, we had a bug early on where now was cached for a validator instance, instead of for a validation pass. The conformance tests can't catch it. Would be good to be sure we don't regress here.

I did not know that. I checked the references for NowVariable class, it is only created here alongside this variable.

Found the issue: #185

Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 · 2025-06-06T19:07:37Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


Where is that used?

ghost

LGTM. I think you could also roll #304 into this too, if you wanted, but fine either way.

Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf · 2025-06-06T07:03:24Z

Switch to Google's CEL implementation.

Type checking is stricter, which meant:
- Converting to CEL types (Long, UnsignedLong, Double, etc...) before passing it to the runtime. Eg: Floats and Double are different for something as simple as - operator and using Floats doesn't work with any function.
- Changing zero values to CEL supported ones.
- Declaring separate function overloads based on type. Resulting in a slight increase in function definitions.
Functions bindings are based on java types.
Partial evaluation is not well documented so it is not implemented in the PR. Also unsure if it is working on main, more on this in a comment.
Global variables per program/runtime are not supported. All the variables need to be passed to eval.

Deliberately didn't refactor to keep relatively easier to review.

srikrsna-buf · 2025-06-06T07:58:45Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


For google.rpc.Status

Where is that used?

In some of the cel protos we generate code for

srikrsna-buf · 2025-06-06T09:27:55Z

+standard_rules/bytes:
+ - pattern/invalid/not_utf8
+# input: [ type.googleapis.com/buf.validate.conformance.cases.BytesPattern ]:{val:"\x99"}
+# want: runtime error: value must be valid UTF-8 to apply regexp
+# got: validation error (1 violation)
+# 1. rule_id: "bytes.pattern"
+# message: "value must match regex pattern `^[\\x00-\\x7F]+$`"
+# field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_BYTES}
+# rule: "bytes.pattern" elements:{field_number:15 field_name:"bytes" field_type:TYPE_MESSAGE} elements:{field_number:4 field_name:"pattern" field_type:TYPE_STRING}


Can be solved if we override the bytes_to_string: #304

srikrsna-buf · 2025-06-06T09:36:19Z

- Program program = ruleEnv.program(rule.astExpression.ast, globals, PARTIAL_EVAL_OPTIONS);
- Program.EvalResult evalResult = program.eval(Activation.emptyActivation());
- Val value = evalResult.getVal();
- if (value != null) {
- Object val = value.value();
- if (val instanceof Boolean && value.booleanValue()) {
- continue;
- }
- if (val instanceof String && val.equals("")) {
- continue;
- }
- }
- Ast residual = ruleEnv.residualAst(rule.astExpression.ast, evalResult.getEvalDetails());
- programs.add(
- new CompiledProgram(
- ruleEnv.program(residual, globals),
- rule.astExpression.source,
- rule.rulePath,
- ruleValue));


From an internal feature standpoint this is the only step we skipped.
I am also not sure if it ever worked. It seems we have to tell which variables/attrs are not available by passing a PartialActivation type to eval. See here.

timostamm

Looks great so far, but I wasn't able to review everything in this pass.

timostamm · 2025-06-06T14:05:12Z

+/**
+ * CEL supports protobuf natively but when we pass it field values (like scalars, repeated, and
+ * maps) it has no way to treat them like a proto message field. This class has methods to convert
+ * to a cel values.
+ */
+final class ProtoAdapter {


timostamm · 2025-06-06T14:07:52Z

+ * for accessing the variable `now` that's constant within an evaluation.
 */
-class NowVariable implements Activation {
+class NowVariable implements CelVariableResolver {


IIRC, we had a bug early on where now was cached for a validator instance, instead of for a validation pass. The conformance tests can't catch it. Would be good to be sure we don't regress here.

I did not know that. I checked the references for NowVariable class, it is only created here alongside this variable.

Found the issue: #185

Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 · 2025-06-06T19:07:37Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


Where is that used?

ghost

LGTM. I think you could also roll #304 into this too, if you wanted, but fine either way.

Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf · 2025-06-06T07:03:24Z

Switch to Google's CEL implementation.

Type checking is stricter, which meant:
- Converting to CEL types (Long, UnsignedLong, Double, etc...) before passing it to the runtime. Eg: Floats and Double are different for something as simple as - operator and using Floats doesn't work with any function.
- Changing zero values to CEL supported ones.
- Declaring separate function overloads based on type. Resulting in a slight increase in function definitions.
Functions bindings are based on java types.
Partial evaluation is not well documented so it is not implemented in the PR. Also unsure if it is working on main, more on this in a comment.
Global variables per program/runtime are not supported. All the variables need to be passed to eval.

Deliberately didn't refactor to keep relatively easier to review.

srikrsna-buf · 2025-06-06T07:58:45Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


For google.rpc.Status

Where is that used?

In some of the cel protos we generate code for

srikrsna-buf · 2025-06-06T09:27:55Z

+standard_rules/bytes:
+ - pattern/invalid/not_utf8
+# input: [ type.googleapis.com/buf.validate.conformance.cases.BytesPattern ]:{val:"\x99"}
+# want: runtime error: value must be valid UTF-8 to apply regexp
+# got: validation error (1 violation)
+# 1. rule_id: "bytes.pattern"
+# message: "value must match regex pattern `^[\\x00-\\x7F]+$`"
+# field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_BYTES}
+# rule: "bytes.pattern" elements:{field_number:15 field_name:"bytes" field_type:TYPE_MESSAGE} elements:{field_number:4 field_name:"pattern" field_type:TYPE_STRING}


Can be solved if we override the bytes_to_string: #304

srikrsna-buf · 2025-06-06T09:36:19Z

- Program program = ruleEnv.program(rule.astExpression.ast, globals, PARTIAL_EVAL_OPTIONS);
- Program.EvalResult evalResult = program.eval(Activation.emptyActivation());
- Val value = evalResult.getVal();
- if (value != null) {
- Object val = value.value();
- if (val instanceof Boolean && value.booleanValue()) {
- continue;
- }
- if (val instanceof String && val.equals("")) {
- continue;
- }
- }
- Ast residual = ruleEnv.residualAst(rule.astExpression.ast, evalResult.getEvalDetails());
- programs.add(
- new CompiledProgram(
- ruleEnv.program(residual, globals),
- rule.astExpression.source,
- rule.rulePath,
- ruleValue));


From an internal feature standpoint this is the only step we skipped.
I am also not sure if it ever worked. It seems we have to tell which variables/attrs are not available by passing a PartialActivation type to eval. See here.

timostamm

Looks great so far, but I wasn't able to review everything in this pass.

timostamm · 2025-06-06T14:05:12Z

+/**
+ * CEL supports protobuf natively but when we pass it field values (like scalars, repeated, and
+ * maps) it has no way to treat them like a proto message field. This class has methods to convert
+ * to a cel values.
+ */
+final class ProtoAdapter {


timostamm · 2025-06-06T14:07:52Z

+ * for accessing the variable `now` that's constant within an evaluation.
 */
-class NowVariable implements Activation {
+class NowVariable implements CelVariableResolver {


IIRC, we had a bug early on where now was cached for a validator instance, instead of for a validation pass. The conformance tests can't catch it. Would be good to be sure we don't regress here.

I did not know that. I checked the references for NowVariable class, it is only created here alongside this variable.

Found the issue: #185

Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 · 2025-06-06T19:07:37Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


Where is that used?

ghost

LGTM. I think you could also roll #304 into this too, if you wanted, but fine either way.

Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf · 2025-06-06T07:03:24Z

Switch to Google's CEL implementation.

Type checking is stricter, which meant:
- Converting to CEL types (Long, UnsignedLong, Double, etc...) before passing it to the runtime. Eg: Floats and Double are different for something as simple as - operator and using Floats doesn't work with any function.
- Changing zero values to CEL supported ones.
- Declaring separate function overloads based on type. Resulting in a slight increase in function definitions.
Functions bindings are based on java types.
Partial evaluation is not well documented so it is not implemented in the PR. Also unsure if it is working on main, more on this in a comment.
Global variables per program/runtime are not supported. All the variables need to be passed to eval.

Deliberately didn't refactor to keep relatively easier to review.

srikrsna-buf · 2025-06-06T07:58:45Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


For google.rpc.Status

Where is that used?

In some of the cel protos we generate code for

srikrsna-buf · 2025-06-06T09:27:55Z

+standard_rules/bytes:
+ - pattern/invalid/not_utf8
+# input: [ type.googleapis.com/buf.validate.conformance.cases.BytesPattern ]:{val:"\x99"}
+# want: runtime error: value must be valid UTF-8 to apply regexp
+# got: validation error (1 violation)
+# 1. rule_id: "bytes.pattern"
+# message: "value must match regex pattern `^[\\x00-\\x7F]+$`"
+# field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_BYTES}
+# rule: "bytes.pattern" elements:{field_number:15 field_name:"bytes" field_type:TYPE_MESSAGE} elements:{field_number:4 field_name:"pattern" field_type:TYPE_STRING}


Can be solved if we override the bytes_to_string: #304

srikrsna-buf · 2025-06-06T09:36:19Z

- Program program = ruleEnv.program(rule.astExpression.ast, globals, PARTIAL_EVAL_OPTIONS);
- Program.EvalResult evalResult = program.eval(Activation.emptyActivation());
- Val value = evalResult.getVal();
- if (value != null) {
- Object val = value.value();
- if (val instanceof Boolean && value.booleanValue()) {
- continue;
- }
- if (val instanceof String && val.equals("")) {
- continue;
- }
- }
- Ast residual = ruleEnv.residualAst(rule.astExpression.ast, evalResult.getEvalDetails());
- programs.add(
- new CompiledProgram(
- ruleEnv.program(residual, globals),
- rule.astExpression.source,
- rule.rulePath,
- ruleValue));


From an internal feature standpoint this is the only step we skipped.
I am also not sure if it ever worked. It seems we have to tell which variables/attrs are not available by passing a PartialActivation type to eval. See here.

timostamm

Looks great so far, but I wasn't able to review everything in this pass.

timostamm · 2025-06-06T14:05:12Z

+/**
+ * CEL supports protobuf natively but when we pass it field values (like scalars, repeated, and
+ * maps) it has no way to treat them like a proto message field. This class has methods to convert
+ * to a cel values.
+ */
+final class ProtoAdapter {


timostamm · 2025-06-06T14:07:52Z

+ * for accessing the variable `now` that's constant within an evaluation.
 */
-class NowVariable implements Activation {
+class NowVariable implements CelVariableResolver {


IIRC, we had a bug early on where now was cached for a validator instance, instead of for a validation pass. The conformance tests can't catch it. Would be good to be sure we don't regress here.

I did not know that. I checked the references for NowVariable class, it is only created here alongside this variable.

Found the issue: #185

Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 · 2025-06-06T19:07:37Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


Where is that used?

ghost

LGTM. I think you could also roll #304 into this too, if you wanted, but fine either way.

Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf · 2025-06-06T07:03:24Z

Switch to Google's CEL implementation.

Type checking is stricter, which meant:
- Converting to CEL types (Long, UnsignedLong, Double, etc...) before passing it to the runtime. Eg: Floats and Double are different for something as simple as - operator and using Floats doesn't work with any function.
- Changing zero values to CEL supported ones.
- Declaring separate function overloads based on type. Resulting in a slight increase in function definitions.
Functions bindings are based on java types.
Partial evaluation is not well documented so it is not implemented in the PR. Also unsure if it is working on main, more on this in a comment.
Global variables per program/runtime are not supported. All the variables need to be passed to eval.

Deliberately didn't refactor to keep relatively easier to review.

srikrsna-buf · 2025-06-06T07:58:45Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


For google.rpc.Status

Where is that used?

In some of the cel protos we generate code for

srikrsna-buf · 2025-06-06T09:27:55Z

+standard_rules/bytes:
+ - pattern/invalid/not_utf8
+# input: [ type.googleapis.com/buf.validate.conformance.cases.BytesPattern ]:{val:"\x99"}
+# want: runtime error: value must be valid UTF-8 to apply regexp
+# got: validation error (1 violation)
+# 1. rule_id: "bytes.pattern"
+# message: "value must match regex pattern `^[\\x00-\\x7F]+$`"
+# field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_BYTES}
+# rule: "bytes.pattern" elements:{field_number:15 field_name:"bytes" field_type:TYPE_MESSAGE} elements:{field_number:4 field_name:"pattern" field_type:TYPE_STRING}


Can be solved if we override the bytes_to_string: #304

srikrsna-buf · 2025-06-06T09:36:19Z

- Program program = ruleEnv.program(rule.astExpression.ast, globals, PARTIAL_EVAL_OPTIONS);
- Program.EvalResult evalResult = program.eval(Activation.emptyActivation());
- Val value = evalResult.getVal();
- if (value != null) {
- Object val = value.value();
- if (val instanceof Boolean && value.booleanValue()) {
- continue;
- }
- if (val instanceof String && val.equals("")) {
- continue;
- }
- }
- Ast residual = ruleEnv.residualAst(rule.astExpression.ast, evalResult.getEvalDetails());
- programs.add(
- new CompiledProgram(
- ruleEnv.program(residual, globals),
- rule.astExpression.source,
- rule.rulePath,
- ruleValue));


From an internal feature standpoint this is the only step we skipped.
I am also not sure if it ever worked. It seems we have to tell which variables/attrs are not available by passing a PartialActivation type to eval. See here.

timostamm

Looks great so far, but I wasn't able to review everything in this pass.

timostamm · 2025-06-06T14:05:12Z

+/**
+ * CEL supports protobuf natively but when we pass it field values (like scalars, repeated, and
+ * maps) it has no way to treat them like a proto message field. This class has methods to convert
+ * to a cel values.
+ */
+final class ProtoAdapter {


timostamm · 2025-06-06T14:07:52Z

+ * for accessing the variable `now` that's constant within an evaluation.
 */
-class NowVariable implements Activation {
+class NowVariable implements CelVariableResolver {


IIRC, we had a bug early on where now was cached for a validator instance, instead of for a validation pass. The conformance tests can't catch it. Would be good to be sure we don't regress here.

I did not know that. I checked the references for NowVariable class, it is only created here alongside this variable.

Found the issue: #185

Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 · 2025-06-06T19:07:37Z

 buf("build.buf:buf:${libs.versions.buf.get()}:${osdetector.classifier}@exe")

 testImplementation(libs.assertj)
+ testImplementation(libs.grpc.protobuf)


Where is that used?

ghost

LGTM. I think you could also roll #304 into this too, if you wanted, but fine either way.

Co-authored-by: Steve Ayers <smaye81@gmail.com>

Switch to dev.cel:cel
1fae175

srikrsna-buf requested review from a user and smaye81 June 6, 2025 07:03

srikrsna-buf commented Jun 6, 2025
View reviewed changes

srikrsna-buf requested a review from timostamm June 6, 2025 12:25

timostamm reviewed Jun 6, 2025
View reviewed changes

srikrsna-buf added 3 commits June 6, 2025 21:13

Revert RepetedMin test
e13ebd0
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment changes
c552415
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment formatting
4ecf54c
Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 approved these changes Jun 6, 2025
View reviewed changes

ghost approved these changes Jun 6, 2025
View reviewed changes

Update src/main/java/build/buf/protovalidate/CompiledProgram.java
311157e
Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf merged commit 3c060a2 into mainJun 6, 2025

srikrsna-buf deleted the sk/cel branch June 6, 2025 19:53

Switch to dev.cel:cel
1fae175

srikrsna-buf requested review from a user and smaye81 June 6, 2025 07:03

srikrsna-buf commented Jun 6, 2025
View reviewed changes

srikrsna-buf requested a review from timostamm June 6, 2025 12:25

timostamm reviewed Jun 6, 2025
View reviewed changes

srikrsna-buf added 3 commits June 6, 2025 21:13

Revert RepetedMin test
e13ebd0
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment changes
c552415
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment formatting
4ecf54c
Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 approved these changes Jun 6, 2025
View reviewed changes

ghost approved these changes Jun 6, 2025
View reviewed changes

Update src/main/java/build/buf/protovalidate/CompiledProgram.java
311157e
Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf merged commit 3c060a2 into mainJun 6, 2025

srikrsna-buf deleted the sk/cel branch June 6, 2025 19:53

Switch to dev.cel:cel
1fae175

srikrsna-buf requested review from a user and smaye81 June 6, 2025 07:03

srikrsna-buf commented Jun 6, 2025
View reviewed changes

srikrsna-buf requested a review from timostamm June 6, 2025 12:25

timostamm reviewed Jun 6, 2025
View reviewed changes

srikrsna-buf added 3 commits June 6, 2025 21:13

Revert RepetedMin test
e13ebd0
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment changes
c552415
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment formatting
4ecf54c
Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 approved these changes Jun 6, 2025
View reviewed changes

ghost approved these changes Jun 6, 2025
View reviewed changes

Update src/main/java/build/buf/protovalidate/CompiledProgram.java
311157e
Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf merged commit 3c060a2 into mainJun 6, 2025

srikrsna-buf deleted the sk/cel branch June 6, 2025 19:53

Switch to dev.cel:cel
1fae175

srikrsna-buf requested review from a user and smaye81 June 6, 2025 07:03

srikrsna-buf commented Jun 6, 2025
View reviewed changes

srikrsna-buf requested a review from timostamm June 6, 2025 12:25

timostamm reviewed Jun 6, 2025
View reviewed changes

srikrsna-buf added 3 commits June 6, 2025 21:13

Revert RepetedMin test
e13ebd0
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment changes
c552415
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment formatting
4ecf54c
Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 approved these changes Jun 6, 2025
View reviewed changes

ghost approved these changes Jun 6, 2025
View reviewed changes

Update src/main/java/build/buf/protovalidate/CompiledProgram.java
311157e
Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf merged commit 3c060a2 into mainJun 6, 2025

srikrsna-buf deleted the sk/cel branch June 6, 2025 19:53

Switch to dev.cel:cel
1fae175

srikrsna-buf requested review from a user and smaye81 June 6, 2025 07:03

srikrsna-buf commented Jun 6, 2025
View reviewed changes

srikrsna-buf requested a review from timostamm June 6, 2025 12:25

timostamm reviewed Jun 6, 2025
View reviewed changes

srikrsna-buf added 3 commits June 6, 2025 21:13

Revert RepetedMin test
e13ebd0
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment changes
c552415
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment formatting
4ecf54c
Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 approved these changes Jun 6, 2025
View reviewed changes

ghost approved these changes Jun 6, 2025
View reviewed changes

Update src/main/java/build/buf/protovalidate/CompiledProgram.java
311157e
Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf merged commit 3c060a2 into mainJun 6, 2025

srikrsna-buf deleted the sk/cel branch June 6, 2025 19:53

Switch to dev.cel:cel
1fae175

srikrsna-buf requested review from a user and smaye81 June 6, 2025 07:03

srikrsna-buf commented Jun 6, 2025
View reviewed changes

srikrsna-buf requested a review from timostamm June 6, 2025 12:25

timostamm reviewed Jun 6, 2025
View reviewed changes

srikrsna-buf added 3 commits June 6, 2025 21:13

Revert RepetedMin test
e13ebd0
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment changes
c552415
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment formatting
4ecf54c
Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 approved these changes Jun 6, 2025
View reviewed changes

ghost approved these changes Jun 6, 2025
View reviewed changes

Update src/main/java/build/buf/protovalidate/CompiledProgram.java
311157e
Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf merged commit 3c060a2 into mainJun 6, 2025

srikrsna-buf deleted the sk/cel branch June 6, 2025 19:53

Switch to dev.cel:cel
1fae175

srikrsna-buf requested review from a user and smaye81 June 6, 2025 07:03

srikrsna-buf commented Jun 6, 2025
View reviewed changes

srikrsna-buf requested a review from timostamm June 6, 2025 12:25

timostamm reviewed Jun 6, 2025
View reviewed changes

srikrsna-buf added 3 commits June 6, 2025 21:13

Revert RepetedMin test
e13ebd0
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment changes
c552415
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment formatting
4ecf54c
Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 approved these changes Jun 6, 2025
View reviewed changes

ghost approved these changes Jun 6, 2025
View reviewed changes

Update src/main/java/build/buf/protovalidate/CompiledProgram.java
311157e
Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf merged commit 3c060a2 into mainJun 6, 2025

srikrsna-buf deleted the sk/cel branch June 6, 2025 19:53

Switch to dev.cel:cel
1fae175

srikrsna-buf requested review from a user and smaye81 June 6, 2025 07:03

srikrsna-buf commented Jun 6, 2025
View reviewed changes

srikrsna-buf requested a review from timostamm June 6, 2025 12:25

timostamm reviewed Jun 6, 2025
View reviewed changes

srikrsna-buf added 3 commits June 6, 2025 21:13

Revert RepetedMin test
e13ebd0
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment changes
c552415
Signed-off-by: Sri Krishna <skrishna@buf.build>

Revert unrelated comment formatting
4ecf54c
Signed-off-by: Sri Krishna <skrishna@buf.build>

smaye81 approved these changes Jun 6, 2025
View reviewed changes

ghost approved these changes Jun 6, 2025
View reviewed changes

Update src/main/java/build/buf/protovalidate/CompiledProgram.java
311157e
Co-authored-by: Steve Ayers <smaye81@gmail.com>

srikrsna-buf merged commit 3c060a2 into mainJun 6, 2025

srikrsna-buf deleted the sk/cel branch June 6, 2025 19:53

Uh oh!

Conversation

srikrsna-buf commented Jun 6, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timostamm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

srikrsna-buf commented Jun 6, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timostamm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

srikrsna-buf commented Jun 6, 2025•
edited
Loading

srikrsna-buf commented Jun 6, 2025•
edited
Loading

srikrsna-buf commented Jun 6, 2025•
edited
Loading