Introduce MessageReflector to enable alternative protobuf runtimes#132
Closed
andrewparmet wants to merge 0 commit intobufbuild:mainfrom
Closed
Introduce MessageReflector to enable alternative protobuf runtimes#132andrewparmet wants to merge 0 commit intobufbuild:mainfrom
andrewparmet wants to merge 0 commit intobufbuild:mainfrom
Conversation
|
|
andrewparmet
changed the title
andrewparmet
force-pushed
the
enable-lazy-protokt-evaluation
branch
3 times, most recently
from
35bf1b7 to
7928e60
Compare
pkwarren
reviewed
Jun 12, 2024
src/main/java/build/buf/protovalidate/internal/evaluator/EnumEvaluator.java
Outdated
Show resolved
Hide resolved
src/main/java/build/buf/protovalidate/internal/evaluator/MessageLike.java
Outdated
Show resolved
Hide resolved
src/main/java/build/buf/protovalidate/internal/evaluator/MessageLike.java
Outdated
Show resolved
Hide resolved
| return clazz.cast(value); | ||
| @Nullable | ||
| public <T> T jvmValue(Class<T> clazz) { | ||
| return null; |
Member
There was a problem hiding this comment.
Is this not used by anyone? Could we not keep it returning the underlying Message as before?
Do we need to make smaller interfaces and only implement jvmValue on Value types that provide a valid implementation?
Author
There was a problem hiding this comment.
Correct, this is not used. jvmValue is used for fields that protovalidate-java validates directly itself, like enums (as opposed to passing into CEL), and right now it does not do any validation on message types.
See also the changed doc on Value.jvmValue.
Author
There was a problem hiding this comment.
But even if it did, it doesn't make much sense to cast the message type to anything, does it? This hints at the narrower interface you mention.
src/main/java/build/buf/protovalidate/internal/evaluator/ObjectValue.java
Outdated
Show resolved
Hide resolved
src/main/java/build/buf/protovalidate/internal/evaluator/ObjectValue.java
Outdated
Show resolved
Hide resolved
andrewparmet
changed the title
andrewparmet
force-pushed
the
enable-lazy-protokt-evaluation
branch
from
88684a4 to
5efed01
Compare
andrewparmet
added a commit
to open-toast/protokt
that referenced
this pull request
Dec 20, 2024
Right now the validator eagerly converts the protokt message to a protobuf DynamicMessage. With bufbuild/protovalidate-java#132, this can [change to supply a wrapper](0cd3157) around the protokt message that only converts as requested by the validator. Not sure if this module belongs at the top level or in third-party. In that case, maybe the gRPC modules also belong in third-party. For example, supposing protokt one day supports the [Connect](https://connectrpc.com/) protocol, that would also go in third-party, but it feels like a sibling to gRPC. Fixes #207.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
see #80
see open-toast/protokt@0cd3157 for what the implementation looks like given this PR