Skip to content

fix(rest_over_grpc): correct transcoder edge cases and harden responses#603

Open
geeknoid wants to merge 1 commit into
mainfrom
rest_over_grpc
Open

fix(rest_over_grpc): correct transcoder edge cases and harden responses#603
geeknoid wants to merge 1 commit into
mainfrom
rest_over_grpc

Conversation

@geeknoid

Copy link
Copy Markdown
Member

Correctness fixes for the REST/JSON <-> gRPC transcoder, response hardening, doc/OpenAPI cleanup, and a 0.2.0 version bump.

  • response_body naming a default-valued field now emits the proto3-JSON default instead of returning 500.
  • Duplicate JSON body keys are rejected (400) on both the fast and query overlay paths; the query overlay now overrides body fields across snake_case/camelCase spellings.
  • Nested additional_bindings are diagnosed as an error per google.api.http.
  • The streaming tonic bridge now forwards initial response metadata, mirroring the unary path.
  • Framing and hop-by-hop headers are stripped from transcoded unary and streaming responses.
  • OpenAPI error schema renamed to google.rpc.Status (collision-proof) with the missing details field added.
  • Removed narrative/historical comments across the crate family.
  • Bumped rest_over_grpc to 0.2.0.
  • README title renders "gRPC" via a template filter.

Copilot AI review requested due to automatic review settings July 24, 2026 14:42
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

✅ Version increments look sufficient

cargo semver-checks compared the 1 crate(s) this PR publishes against their previous version-bump commit in git history. Every version increment is sufficient for the detected API changes.

Crate Baseline Baseline commit This PR Minimum required Status
rest_over_grpc 0.1.0 5b39c89 0.2.0 0.1.1 ✅ ok

This check is informational and does not block the merge.

View the check run

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens correctness and hardens behavior across the rest_over_grpc REST/JSON ↔ gRPC transcoding pipeline, including request overlay decoding, response field selection defaults, route/descriptor validation, response-header sanitization, OpenAPI output, and a crate version bump to 0.2.0.

Changes:

  • Make response_body field selection emit a supplied proto3-JSON default literal when the selected field is omitted by serialization (proto3 default omission), instead of failing.
  • Reject duplicate JSON body keys (including snake_case vs lowerCamelCase equivalents) and improve query/body overlay behavior and efficiency.
  • Harden response metadata/headers (unary + streaming), improve descriptor validation (path variables, nested additional_bindings), and update OpenAPI error schema naming.

Reviewed changes

Copilot reviewed 54 out of 55 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/rest_over_grpc/tests/value_types.rs Adjusts tests to match updated response header/content-type behavior.
crates/rest_over_grpc/tests/streaming.rs Updates streaming tests for new ResponseBodyKind::Field { .. } shape and contract cleanup.
crates/rest_over_grpc/tests/streaming_serving.rs Updates serving tests/import paths and clarifies mid-stream error expectations.
crates/rest_over_grpc/tests/build_api.rs Cleans up generator tests while keeping behavior assertions.
crates/rest_over_grpc/tests/axum_support.rs Updates axum integration tests/import paths.
crates/rest_over_grpc/src/transcode/response_body_kind.rs Extends response body selection to include a proto3-JSON default literal.
crates/rest_over_grpc/src/transcode/overlay.rs Adds duplicate-body-key rejection and case-normalized query/body overlay semantics.
crates/rest_over_grpc/src/transcode/mod.rs Emits supplied default literal when selected field is absent from serialization; updates tests/docs.
crates/rest_over_grpc/src/transcode/field_serializer.rs Reclassifies scalar/sequence shapes as “unsupported” for field selection to drive fallback paths.
crates/rest_over_grpc/src/transcode/error.rs Avoids boxing backtraces when backtraces are disabled/unsupported.
crates/rest_over_grpc/src/transcode_response.rs Strips uncontrolled headers from streaming header merges; doc import updates.
crates/rest_over_grpc/src/transcode_api.rs Minor test comment cleanup.
crates/rest_over_grpc/src/stream.rs Refactors Accept q-value parsing for correctness; updates tests and docs.
crates/rest_over_grpc/src/lib.rs Re-exports StreamEncoding via transcoding module.
crates/rest_over_grpc/src/http_response.rs Strips uncontrolled framing/hop-by-hop headers when merging handler headers.
crates/rest_over_grpc/src/context.rs Introduces helper to strip uncontrolled response headers (framing + hop-by-hop).
crates/rest_over_grpc/src/build/service_method.rs Carries per-route response_body default literals through codegen inputs.
crates/rest_over_grpc/src/build/service_definition.rs Forwards initial streaming response metadata; aligns identifier sanitization with prost.
crates/rest_over_grpc/src/build/openapi.rs Renames shared error schema to google.rpc.Status and adds details; fixes body-field filtering.
crates/rest_over_grpc/src/build/http_rule.rs Stores/propagates response-body default literals through lowering/bindings.
crates/rest_over_grpc/src/build/descriptor.rs Validates path-variable targets, rejects nested additional_bindings, computes response-body defaults.
crates/rest_over_grpc/src/build/descriptor_options.rs Minor test comment cleanup.
crates/rest_over_grpc/src/build/descriptor_error.rs Broadens “unknown field” classification to include path variables.
crates/rest_over_grpc/src/build/binding.rs Stores response-body default per binding and clarifies template parsing intent.
crates/rest_over_grpc/README.md Regenerated README (title/logo casing + links for 0.2.0).
crates/rest_over_grpc/examples/tower_service.rs Comment cleanup in tower-service example.
crates/rest_over_grpc/examples/layered_service.rs Comment cleanup in layered-service example.
crates/rest_over_grpc/examples/generate_service.rs Comment cleanup in codegen example.
crates/rest_over_grpc/Cargo.toml Bumps crate version to 0.2.0.
crates/rest_over_grpc_tests/tests/serving.rs Comment cleanup in end-to-end serving tests.
crates/rest_over_grpc_tests/tests/openapi.rs Updates OpenAPI tests for google.rpc.Status schema key.
crates/rest_over_grpc_tests/tests/e2e.rs Adds coverage for defaulted response_body and duplicate-body-key rejection; comment cleanup.
crates/rest_over_grpc_tests/tests/coverage.rs Comment cleanup in router coverage tests.
crates/rest_over_grpc_tests/tests/bridge.rs Adds test ensuring streaming tonic bridge forwards initial response metadata.
crates/rest_over_grpc_tests/src/tonic_bridge.rs Clarifies include/module expectations for generated code.
crates/rest_over_grpc_tests/src/custom.rs Clarifies include/module expectations for generated code; comment cleanup.
crates/rest_over_grpc_tests/build.rs Comment cleanup and clarifies output-dir separation requirement.
crates/rest_over_grpc_tests/benches/rog_transcode_cg.rs Comment cleanup in transcoding callgrind bench.
crates/rest_over_grpc_tests/benches/rog_router_cg.rs Comment cleanup in router callgrind bench.
crates/rest_over_grpc_tests/bench_routes.rs Replaces long narrative header with concise docstring.
crates/rest_over_grpc_examples/src/tonic_bridge.rs Clarifies include/module expectations for generated code.
crates/rest_over_grpc_examples/src/custom.rs Clarifies include/module expectations for generated code; comment cleanup.
crates/rest_over_grpc_examples/examples/transcoding/custom_fallback.rs Comment cleanup and minor formatting changes in example driver.
crates/rest_over_grpc_examples/examples/transcoding/basic_transcode.rs Collapses request list formatting; comment cleanup.
crates/rest_over_grpc_examples/examples/serving/tower_service.rs Comment cleanup in serving example.
crates/rest_over_grpc_examples/examples/serving/streaming_response.rs Comment cleanup in streaming serving example.
crates/rest_over_grpc_examples/examples/serving/custom_body_handling.rs Comment cleanup in custom body handling example.
crates/rest_over_grpc_examples/examples/serving/axum_app.rs Comment cleanup in axum app example.
crates/rest_over_grpc_examples/examples/handling/volo_bridge.rs Comment cleanup and minor formatting changes in volo bridge example.
crates/rest_over_grpc_examples/examples/handling/direct_service.rs Comment cleanup in direct-service example.
crates/rest_over_grpc_examples/examples/handling/client_streaming_upload.rs Comment cleanup in client-streaming upload example.
crates/rest_over_grpc_examples/build.rs Comment cleanup; clarifies descriptor preservation and generation steps.
crates/README.j2 Template tweak so rendered titles say “gRPC” consistently.
Cargo.toml Updates workspace dependency version for rest_over_grpc to 0.2.0.
Cargo.lock Lockfile updated for rest_over_grpc 0.2.0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/rest_over_grpc/src/context.rs
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (fa49a45) to head (c4a97e3).

Additional details and impacted files
@@           Coverage Diff            @@
##             main     #603    +/-   ##
========================================
  Coverage   100.0%   100.0%            
========================================
  Files         442      442            
  Lines       42616    43250   +634     
========================================
+ Hits        42616    43250   +634     
Flag Coverage Δ
linux 80.4% <100.0%> (?)
linux-arm 80.4% <100.0%> (?)
windows 80.4% <100.0%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

Copilot AI review requested due to automatic review settings July 24, 2026 15:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 53 out of 54 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

crates/rest_over_grpc/src/context.rs:220

  • strip_uncontrolled_response_headers removes the Connection header but does not remove the header fields named by its comma-separated tokens. Those token-named fields are also hop-by-hop and must be stripped (RFC 9110); otherwise a handler can still inject hop-by-hop headers onto the wire via Connection: x-hop + x-hop: ....
pub(crate) fn strip_uncontrolled_response_headers(headers: &mut HeaderMap) {
    use http::header;

    const HOP_BY_HOP: &[HeaderName] = &[
        header::CONTENT_LENGTH,

Comment thread crates/rest_over_grpc/src/transcode/overlay.rs
Comment thread crates/rest_over_grpc/src/transcode/overlay.rs Outdated
Copilot AI review requested due to automatic review settings July 24, 2026 15:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 53 out of 54 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

crates/rest_over_grpc/src/transcode/overlay.rs:100

  • decode_flat builds query_norms as a SmallVec and then calls contains() for every body entry. That makes filtering O(body_len * query_len) and can regress significantly for larger query strings compared to the prior O(1) HashMap membership check. Consider using a HashSet of normalized query keys so the retain pass stays O(body_len).
    let query_norms: SmallVec<[Cow<'_, str>; 8]> = overlay.iter().map(|(key, _)| normalized_key(key)).collect();
    body_entries.retain(|(key, _)| {
        let normalized = normalized_key(key);
        !query_norms.contains(&normalized)
    });

Copilot AI review requested due to automatic review settings July 24, 2026 15:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI review requested due to automatic review settings July 24, 2026 16:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI review requested due to automatic review settings July 24, 2026 16:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 53 out of 54 changed files in this pull request and generated 1 comment.

Comment thread crates/rest_over_grpc/src/transcode/error.rs
Copilot AI review requested due to automatic review settings July 24, 2026 18:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 55 out of 56 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

crates/rest_over_grpc/src/transcode/error.rs:271

  • backtrace_capture_honors_runtime_support uses unsafe { std::env::set_var(..) }, which is only sound if no other threads exist in the process. That assumption holds under nextest’s process-per-test model, but this unit test can also be run under libtest (e.g. cargo test), where tests run on multiple threads by default, making this unsafe block potentially UB / flaky.

You can keep the behavioral assertion without mutating RUST_BACKTRACE at runtime by just comparing MaybeBacktrace::capture() against Backtrace::capture().status() in the current process environment.

    fn backtrace_capture_honors_runtime_support() {
        // Nextest isolates each test in its own process, so enabling backtraces
        // here does not leak into other tests. On platforms where capture is
        // supported this exercises the `Captured` arm; elsewhere it falls back
        // to `Disabled`.
        //
        // SAFETY: set before the first backtrace capture in this process, with
        // no other threads running.
        unsafe {
            std::env::set_var("RUST_BACKTRACE", "1");
        }

Copilot AI review requested due to automatic review settings July 24, 2026 18:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 55 out of 56 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

crates/rest_over_grpc/src/transcode/error.rs:271

  • This test uses unsafe { std::env::set_var(...) }. In cargo test, unit tests can run in parallel threads within the same process, so mutating environment variables can be unsound/flaky (the function is unsafe for a reason). The assertion can be written without changing RUST_BACKTRACE, by simply comparing MaybeBacktrace::capture() with Backtrace::capture().status() in the current environment.
        // SAFETY: set before the first backtrace capture in this process, with
        // no other threads running.
        unsafe {
            std::env::set_var("RUST_BACKTRACE", "1");
        }

Comment thread codecov.yml Outdated
Comment thread .github/workflows/main.yml Outdated
Comment thread crates/rest_over_grpc/src/transcode/response_body_kind.rs
Copilot AI review requested due to automatic review settings July 24, 2026 20:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 53 out of 54 changed files in this pull request and generated no new comments.

Correctness fixes for the REST/JSON <-> gRPC transcoder, response
hardening, doc/OpenAPI cleanup, and a 0.2.0 version bump. Each fix has a
focused regression test and the crate keeps 100% line coverage.

- response_body naming a default-valued field now emits the proto3-JSON
  default instead of returning 500.
- Duplicate JSON body keys are rejected (400) on both the fast and query
  overlay paths; the query overlay now overrides body fields across
  snake_case/camelCase spellings.
- Nested additional_bindings are diagnosed as an error per google.api.http.
- The streaming tonic bridge now forwards initial response metadata,
  mirroring the unary path.
- Framing and hop-by-hop headers are stripped from transcoded unary and
  streaming responses, including headers named by Connection tokens.
- OpenAPI error schema renamed to google.rpc.Status (collision-proof)
  with the missing details field added.
- Removed narrative/historical comments across the crate family.
- Bumped rest_over_grpc to 0.2.0.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3f38f565-ad78-413e-ae01-e84277669a87
Copilot AI review requested due to automatic review settings July 25, 2026 02:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 53 out of 54 changed files in this pull request and generated 1 comment.

Comment on lines +61 to +73
/// Normalizes a proto3-JSON field key for identity comparison.
///
/// proto3 JSON accepts a field under both its `snake_case` proto name and its
/// `lowerCamelCase` JSON name, which denote the same field. Folding case and
/// dropping `_` maps both spellings to one key, so the overlay can treat them as
/// the same field when deduplicating body entries and applying query overrides.
fn normalized_key(key: &str) -> Cow<'_, str> {
if key.bytes().all(|byte| byte != b'_' && !byte.is_ascii_uppercase()) {
Cow::Borrowed(key)
} else {
Cow::Owned(key.chars().filter(|&ch| ch != '_').map(|ch| ch.to_ascii_lowercase()).collect())
}
}
@ralfbiedert

Copy link
Copy Markdown
Collaborator

Second review pass at c4a97e35d3bf30871e188889da483e19cc883288.

Findings:

  1. crates/rest_over_grpc/src/build/service_definition.rs:966-975 falls back to JSON null for manual scalar response_body rules without descriptor metadata. This differs from protobuf defaults such as 0, false, and "". The manual APIs need validated type/default metadata or should require descriptor-backed generation.
  2. crates/rest_over_grpc/src/transcode/overlay.rs:61-72,91-92,159 approximates ProtoJSON aliases by lowercasing and removing underscores. This cannot represent arbitrary json_name values and incorrectly makes names case-insensitive. Generate exact proto-name/JSON-name aliases from descriptors.
  3. crates/rest_over_grpc/src/http_response.rs:327-340 allows unary custom responses to retain stale framing or hop-by-hop headers such as Content-Length, Transfer-Encoding, and Connection. Apply the same unconditional stripping used by the streaming path.
  4. Query-only requests allocate a HashSet at crates/rest_over_grpc/src/transcode/overlay.rs:80-99 even when no body fields need filtering. Avoid constructing normalization state for an empty body.

No unsafe code or affected unsafe invariant was found. All-feature docs, focused tests, Clippy, rustfmt, and benchmark compilation succeeded.

@ralfbiedert ralfbiedert left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved after the detailed review comment above.

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.

4 participants