Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Apr 22, 2025

I think it has to be a global overload, a member overload would require a generic on the receiver which doesn't seem possible.

I'm proposing this as an eventual replacement (before 1.0) of our hack around the fact that the in identifier is reserved in CEL. This is especially urgent for protovalidate-cc which is currently carrying patches to the CEL implementation in order to enable it, since cel-cpp doesn't allow this sort of functionality to be added in at runtime.

Protovalidate PR: bufbuild/protovalidate#352

I think it has to be a global overload, a member overload would require a generic on the receiver which doesn't seem possible.

I'm proposing this as an eventual replacement (before 1.0) of our hack around the fact that the `in` identifier is reserved in CEL. This is especially urgent for protovalidate-cc which is currently carrying patches to the CEL implementation in order to enable it, since cel-cpp doesn't allow this sort of functionality to be added in at runtime.
@github-actions
Copy link

github-actions bot commented Apr 22, 2025

The latest Buf updates on your PR. Results from workflow Buf / validate-protos (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedApr 22, 2025, 11:52 PM

@ghost ghost marked this pull request as ready for review April 22, 2025 06:56
@ghost ghost requested a review from rodaine April 22, 2025 15:20
cel/library.go Outdated
cel.AnyType,
cel.FunctionBinding(func(values ...ref.Val) ref.Val {
message, ok := values[0].Value().(proto.Message)
message, ok := values[0].(interface{ Get(index ref.Val) ref.Val })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use traits.Indexer?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, yeah. Fixed

@rodaine
Copy link
Member

rodaine commented Apr 22, 2025

Also, re:

a member overload would require a generic on the receiver which doesn't seem possible.

If you make the receiver type dyn, I think it would work?

ghost pushed a commit to bufbuild/protovalidate that referenced this pull request Apr 22, 2025
This is a proposal to remove our hack around the fact the `in`
identifier is reserved in CEL. This is especially urgent for
protovalidate-cc which is currently carrying patches to the CEL
implementation in order to enable it, since cel-cpp doesn't allow this
sort of functionality to be added in at runtime.

Runtime PRs:
- Go: bufbuild/protovalidate-go#225
- C++: bufbuild/protovalidate-cc#90
- Python: bufbuild/protovalidate-python#290
- Java: bufbuild/protovalidate-java#271
@ghost
Copy link
Author

ghost commented Apr 22, 2025

Oops, I saw this after me and Nick moved to merge the protovalidate PR. I've been pondering but I think it's not worth losing too much sleep over; we're still pre-1.0 so we can always try to make this prettier later, especially since the protovalidate protobuf descriptors used for standard constraints are always strongly tied to the implementation version.

(I think if we try to do that, most likely we'd run into problems with at least some of the CEL runtimes, whereas this approach works on all five. That said, we can workshop it. If we really hate free functions we could always do something evil like "field".indexInto(message).)

@ghost ghost merged commit f6a8711 into main Apr 23, 2025
7 checks passed
@ghost ghost deleted the jchadwick/getfield branch April 23, 2025 01:10
This pull request was closed.
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 this pull request may close these issues.

1 participant