protobuf: refactor proto visitor pattern. #9807
Conversation
Move the proto traversal handling in version_converter.cc to a standalone library. This lets us replace existing proto visitor patterns in common/protobuf/utility.cc for unexpected field checks. The redaction code is actually a bit more involved, so I'm not refactoring this; it needs to recurse through Any/TypedStruct. Ultimately we might want something like this, but it doesn't seem super helpful given we only have a the single instance of this right now. Risk level: Low Testing: Existing tests continue to pass. Signed-off-by: Harvey Tuch <htuch@google.com>
|
CC @mergeconflict not tackling redact here for reasons articulated in commit message. |
| (!field->is_repeated() && !reflection->HasField(message, field))) { | ||
| continue; | ||
| if ((field.is_repeated() && reflection->FieldSize(message, &field) == 0) || | ||
| (!field.is_repeated() && !reflection->HasField(message, &field))) { |
There was a problem hiding this comment.
optional, but it seems like this might be useful enough to make part of the onField call. I'd think folks would either want to be traversing structure (not care) or looking at valid fields (care) and if they care it's easy enough to get this wrong.
There was a problem hiding this comment.
Surprisingly tricky, since what is counted as used/unused is subtle for scalar fields. This can be seen in this code, since it first performs the scalar enum check before conditioning on whether the field is set in some sense.
| continue; | ||
| if ((field.is_repeated() && reflection->FieldSize(message, &field) == 0) || | ||
| (!field.is_repeated() && !reflection->HasField(message, &field))) { | ||
| return nullptr; |
There was a problem hiding this comment.
do we ever return something which is used, or plan to?
There was a problem hiding this comment.
Yeah, in TypeAnnotatingProtoVisitor::onField().
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
not stale |
stale
bot
removed
the
stale
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Move the proto traversal handling in version_converter.cc to a
standalone library. This lets us replace existing proto visitor patterns in
common/protobuf/utility.cc for unexpected field checks.
The redaction code is actually a bit more involved, so I'm not
refactoring this; it needs to recurse through Any/TypedStruct.
Ultimately we might want something like this, but it doesn't seem super
helpful given we only have a the single instance of this right now.
Risk level: Low
Testing: Existing tests continue to pass.
Signed-off-by: Harvey Tuch htuch@google.com