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

[BUG] CEL-Java is not fully compatible with Protobuf V4 #462

Closed
bugs84 opened this issue Oct 3, 2024 · 6 comments · Fixed by #466
Closed

[BUG] CEL-Java is not fully compatible with Protobuf V4 #462

bugs84 opened this issue Oct 3, 2024 · 6 comments · Fixed by #466

Comments

@bugs84
Copy link

bugs84 commented Oct 3, 2024

Describe the bug
ConstantFoldingOptimizer doesn't work for some expressions. e.g. "my_var in ['H', 'O']"

To Reproduce
Clone this repository, that reproduce the bug.
https://github.com/bugs84/CelOptimizerIssue

Current issue
Program ends with following exception:

Exception in thread "main" java.lang.NoSuchMethodError: 'com.google.protobuf.Internal$LongList dev.cel.expr.UnknownSet.mutableCopy(com.google.protobuf.Internal$LongList)'
	at dev.cel.expr.UnknownSet.access$600(UnknownSet.java:14)
	at dev.cel.expr.UnknownSet$Builder.ensureExprsIsMutable(UnknownSet.java:456)
	at dev.cel.expr.UnknownSet$Builder.addAllExprs(UnknownSet.java:539)
	at dev.cel.runtime.InterpreterUtil.createUnknownExprValue(InterpreterUtil.java:95)
	at dev.cel.runtime.InterpreterUtil.createUnknownExprValue(InterpreterUtil.java:84)
	at dev.cel.runtime.InterpreterUtil.valueOrUnknown(InterpreterUtil.java:146)
	at dev.cel.runtime.RuntimeUnknownResolver.resolveSimpleName(RuntimeUnknownResolver.java:114)
	at dev.cel.runtime.DefaultInterpreter$ExecutionFrame.resolveSimpleName(DefaultInterpreter.java:975)
	at dev.cel.runtime.DefaultInterpreter$ExecutionFrame.access$200(DefaultInterpreter.java:936)
	at dev.cel.runtime.DefaultInterpreter$DefaultInterpretable.resolveIdent(DefaultInterpreter.java:274)
	at dev.cel.runtime.DefaultInterpreter$DefaultInterpretable.evalIdent(DefaultInterpreter.java:262)
	at dev.cel.runtime.DefaultInterpreter$DefaultInterpretable.evalInternal(DefaultInterpreter.java:193)
	at dev.cel.runtime.DefaultInterpreter$DefaultInterpretable.evalCall(DefaultInterpreter.java:387)
	at dev.cel.runtime.DefaultInterpreter$DefaultInterpretable.evalInternal(DefaultInterpreter.java:199)
	at dev.cel.runtime.DefaultInterpreter$DefaultInterpretable.evalTrackingUnknowns(DefaultInterpreter.java:179)
	at dev.cel.runtime.DefaultInterpreter$DefaultInterpretable.eval(DefaultInterpreter.java:170)
	at dev.cel.runtime.CelRuntime$Program.evalInternal(CelRuntime.java:146)
	at dev.cel.runtime.CelRuntime$Program.evalInternal(CelRuntime.java:121)
	at dev.cel.runtime.CelRuntime$Program.evalInternal(CelRuntime.java:116)
	at dev.cel.runtime.CelRuntime$Program.eval(CelRuntime.java:49)
	at dev.cel.common.ast.CelExprUtil.evaluateExpr(CelExprUtil.java:42)
	at dev.cel.optimizer.optimizers.ConstantFoldingOptimizer.maybeFold(ConstantFoldingOptimizer.java:251)
	at dev.cel.optimizer.optimizers.ConstantFoldingOptimizer.optimize(ConstantFoldingOptimizer.java:113)
	at dev.cel.optimizer.CelOptimizerImpl.optimize(CelOptimizerImpl.java:45)
	at HelloWorld.run(HelloWorld.java:62)
	at HelloWorld.main(HelloWorld.java:73)
@l46kok
Copy link
Collaborator

l46kok commented Oct 3, 2024

I was able to reproduce this. My suspicion is that something about the protobuf upgrade to 4.28.0 is causing this issue, and that the binary compatibility may not have been fully restored as stated in protocolbuffers/protobuf#17247. Either that, or the dependent protos (ExprValue from proto-google-common-protos, used to represent unknowns in CEL) is not fully protobuf v4 compatible.

The workaround is to downgrade CEL to 0.6.0 as of now. I'll look into this further.

@l46kok
Copy link
Collaborator

l46kok commented Oct 4, 2024

This is an issue on any code path in CEL that references the Builder's methods on protobuf messages. The earlier release of Protobuf v4 have removed GeneratedMessageV3, and shims were added afterwards to restore ABI compatibility in 4.27.x, but not fully. Particularly, any generated messages produced before protoc version 3.25.x are no longer compatible with Protobuf V4. Bazel's built-in proto toolchain (@com_google_protobuf//:protoc) happens to use 3.21. This unfortunately led to publishing a JAR with incompatible generated messages.

In short, CEL is not compatible with protobuf v4. The published JAR for 0.7.1 forces protobuf v4 as a dependency so it should not be used. I'll try to release a fix soon.

@bugs84
Copy link
Author

bugs84 commented Oct 4, 2024

Thank you.

Just note:
Downgrade to CEL 0.6.0 - has different issue NoClassDefFoundError: com/google/rpc/StatusProto
But it doesn't matter. For us is now enough to comment out ConstantFoldingOptimizer. And wait for proper fix.
Thank you again!

@l46kok
Copy link
Collaborator

l46kok commented Oct 4, 2024

Oh for that one, you just need to bring in proto-google-common-protos from maven. Not sure why that's not being resolved automatically in gradle, but that's a separate issue.

Btw, the issue in 0.7.1 is not just specific to constant folding. You'll run into issues in other places where an unknown value needs to be referenced at all internally.

@l46kok l46kok changed the title [BUG] ConstantFoldingOptimizer doesn't work for some expressions [BUG] CEL-Java is not fully compatible with Protobuf V4 Oct 9, 2024
copybara-service bot pushed a commit that referenced this issue Oct 9, 2024
Fixes #462

PiperOrigin-RevId: 684215593
copybara-service bot pushed a commit that referenced this issue Oct 9, 2024
Fixes #462

PiperOrigin-RevId: 684215593
copybara-service bot pushed a commit that referenced this issue Oct 10, 2024
Fixes #462

PiperOrigin-RevId: 684215593
@l46kok
Copy link
Collaborator

l46kok commented Oct 10, 2024

@bugs84 The latest release 0.8.0 should work now. Thank you for reporting.

@bugs84
Copy link
Author

bugs84 commented Oct 14, 2024

Thank you. I confirm, that sample project with cel 0.8.0 and
com.google.api.grpc:proto-google-common-protos:2.46.0
works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants