Skip to content

test: verify synthetic fields are skipped #2393

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

coryan
Copy link
Collaborator

@coryan coryan commented Jun 7, 2025

Synthetic fields need to be skipped during serialization. We had no
explicit tests for this functionality.

Motivated by #2376

Synthetic fields need to be skipped during serialization. We had no
explicit tests for this functionality.
Copy link

codecov bot commented Jun 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.57%. Comparing base (56d472f) to head (7833ad8).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2393   +/-   ##
=======================================
  Coverage   95.57%   95.57%           
=======================================
  Files          75       75           
  Lines        2940     2940           
=======================================
  Hits         2810     2810           
  Misses        130      130           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coryan coryan marked this pull request as ready for review June 7, 2025 16:07
@coryan coryan requested a review from a team as a code owner June 7, 2025 16:07
Copy link
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

Food for thought, but nothing blocking this PR.

It sort of affects my current work which involves building the paths.

FTR, my plan is to keep taking references to fields used in the path and copying them into the path. I am not gonna block on this.

fn skip_synthetic_fields() -> Result<()> {
use smo::model::{CreateSecretRequest, Secret};
let input = CreateSecretRequest::new()
.set_project("a-synthetic-field")
Copy link
Member

Choose a reason for hiding this comment

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

I am late to the party and confused. I read:

// The OpenAPI specifications have incomplete `*Request` messages. We inject
// some helper fields. These need to be marked so they can be excluded
// from serialized messages and in other places.
Synthetic bool

So these synthetic fields are part of the path we would use to make the RPC, not part the request? That makes sense. But don't things work the same way with gRPC transcoding?

Hmm, I thought fields in the path need not be repeated in the body (even when the body is *). But I don't see that stated in https://google.aip.dev/127.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, 2.4.2 in go/actools-regapic-grpc-transcoding says fields from the path should not be included in the body, but this is optional. So what we are doing is fine.

Still the way this is unified feels weird to me. Seems like we could serialize the fields, even if they are synthetic. But then in the transport.rs layer, we could std::mem::take the fields out of the request for use in the path.

Thoughts?

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.

2 participants