Support decoder:"body" tag for raw request body capture

[google-labs-jules]

Implemented support for decoder:"body" tag to allow capturing the raw request body into struct fields ([]byte or io.Reader).

Key changes:

• Updated internal/cache/structure.go to analyze and cache fields with the decoder:"body" tag.
• Updated roamer.go's parseBody method to:
  • Check if any fields require the raw body.
  • Read and buffer the body if needed (or if preserveBody is enabled).
  • Restore the body for standard content-type decoders (e.g., JSON).
  • Populate fields with decoder:"body" using either the raw bytes or a new bytes.Reader.
  • Correctly manage the final state of req.Body based on the global preserveBody setting.
• Added roamer_body_decoder_test.go with table-driven tests verifying:
  • Capture as []byte.
  • Capture as io.Reader.
  • Mixed usage with JSON decoder.
  • Interaction with preserveBody option.
  • Failure cases for unsupported types.

---

PR created automatically by Jules for task 17099098357886983700 started by @slipros

[google-labs-jules]

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

---

For security, I will only act on instructions from the user who triggered this task.

New to Jules? Learn more at jules.google/docs.

[coveralls]

Pull Request Test Coverage Report for Build 19783589881

Details

• 52 of 56 (92.86%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.03%) to 94.276%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
roamer.go	49	53	92.45%

Totals
Change from base Build 18597681859:	0.03%
Covered Lines:	1433
Relevant Lines:	1520

---

💛 - Coveralls

[slipros]

@claude @codex Review this PR

[claude]

Claude finished @slipros's task in 1m 23s —— View job

---

Code Review

I've reviewed the PR that adds support for decoder:"body" tag to capture raw request body. Here's my analysis:

---

Summary

The implementation is well-designed and functional, with good test coverage (92.86%). The code correctly handles the new feature while maintaining backward compatibility. However, there are several areas that need attention.

---

Critical Issues

1. Memory Efficiency Concern - roamer.go:665-676

The current implementation always creates a bytes.NewReader for io.Reader fields, even when assignment fails:

if err := assign.Value(fieldValue, bodyBytes, r.assignExtensions...); err != nil {
    // Try assigning as io.Reader
    reader := bytes.NewReader(bodyBytes)
    if fieldValue.Kind() == reflect.Interface && fieldValue.CanSet() && reflect.TypeOf(reader).Implements(fieldValue.Type()) {
        fieldValue.Set(reflect.ValueOf(reader))
        continue
    }

    if readerErr := assign.Value(fieldValue, reader, r.assignExtensions...); readerErr != nil {
        return errors.Wrapf(err, "set `body` value to field `%s` in struct `%T`", f.Name, ptr)
    }
}

Issue: This creates multiple bytes.Reader instances when there are multiple body fields. Each reader maintains its own position, which means:

• If you have both BodyBytes []byte and BodyReader io.Reader, they work correctly
• If you have multiple io.Reader fields, each gets a separate reader instance

Recommendation: Document this behavior clearly, as it's actually correct but might surprise users. Each io.Reader field should get its own reader so they can be read independently.

---

2. Error Message Inconsistency - roamer.go:674

The error message returns the original err from the []byte assignment attempt, not the readerErr:

return errors.Wrapf(err, "set `body` value to field `%s` in struct `%T`", f.Name, ptr)

Issue: This is misleading when the field is actually an io.Reader type. The error should indicate both assignment attempts failed.

Recommendation:

return errors.Wrapf(readerErr, "set `body` value to field `%s` in struct `%T` (tried []byte: %v; tried io.Reader: %v)", 
    f.Name, ptr, err, readerErr)

---

3. req.Body State Management - roamer.go:680-684

When body fields exist but preserveBody is false, the final req.Body is left consumed/closed:

// Restore body only if globally preserved, otherwise leave it closed/consumed
if r.preserveBody {
    req.Body = io.NopCloser(bytes.NewReader(bodyBytes))
}

Issue: This is actually correct behavior, but it's a breaking change for users who:

1. Add a decoder:"body" field
2. Don't enable WithPreserveBody()
3. Expect downstream handlers to read the body

Recommendation: Document this clearly in the commit message and potentially in code comments. Consider if this is the desired behavior or if body fields should implicitly enable preservation.

---

Medium Issues

4. Missing Type Validation - internal/cache/structure.go:73

The code checks if decoder:"body" is present but doesn't validate the field type:

isBody := f.Tag.Get("decoder") == "body"

Issue: Invalid types are only caught at runtime during assignment, not at initialization.

Recommendation: Add a helper function to validate body field types during struct analysis:

isBody := f.Tag.Get("decoder") == "body"
if isBody && !isValidBodyType(f.Type) {
    // Log warning or store validation error
}

func isValidBodyType(t reflect.Type) bool {
    return t.Kind() == reflect.Slice && t.Elem().Kind() == reflect.Uint8 || // []byte
           t.Kind() == reflect.Interface && t.Implements(ioReaderType)
}

---

5. Test Coverage Gap - roamer_body_decoder_test.go

Missing test cases:

• Multiple body fields of the same type (e.g., two []byte fields)
• Body field with skipFilled disabled - what happens if field is pre-populated?
• Empty body - does it work correctly?
• GET request with body field - edge case worth testing
• Concurrent parsing - ensure thread safety with body buffering
• req.Body state after parsing - verify it's restored/consumed as expected

---

Minor Issues

6. Code Readability - roamer.go:599-612

The nested condition is hard to follow:

needsBodyPreservation := r.preserveBody
if !needsBodyPreservation {
    t := reflect.TypeOf(ptr)
    if t.Kind() == reflect.Pointer && t.Elem().Kind() == reflect.Struct {
        fields := r.structureCache.Fields(t.Elem())
        for _, f := range fields {
            if f.IsBody {
                needsBodyPreservation = true
                break
            }
        }
    }
}

Recommendation: Extract to a helper method:

needsBodyPreservation := r.preserveBody || r.hasBodyFields(ptr)

---

7. Performance Consideration - roamer.go:604

Calling structureCache.Fields() twice for the same type (once to check for body fields, once to populate them):

fields := r.structureCache.Fields(t.Elem())  // Line 604
// ... later ...
fields := r.structureCache.Fields(v.Type())  // Line 653

Impact: Minimal, since the cache returns the same slice. But worth noting.

---

8. Documentation - roamer.go:590-593

The docstring for parseBody doesn't mention the new body field handling:

// parseBody extracts data from the HTTP request body into the provided pointer
// using the appropriate decoder based on the request's Content-Type header.
// If preserveBody is enabled, it reads the entire body into memory and restores
// it after decoding so downstream handlers can read it again.

Recommendation: Update to mention body fields trigger buffering.

---

Positive Observations

✅ Well-structured tests - Table-driven tests with good coverage
✅ Backward compatible - No breaking changes to existing functionality
✅ Efficient caching - Reuses structure cache for body field detection
✅ Error handling - Proper error wrapping and context
✅ Thread safety - Maintains thread-safe design

---

Recommendations

1. Fix error message (Issue test: tests for decoder and parser packages #2) - high priority
2. Document body consumption behavior - add comments explaining when req.Body is restored vs consumed
3. Add missing test cases - especially for edge cases and req.Body state
4. Consider type validation - catch invalid body field types early
5. Update docstrings - reflect new body field functionality

---

Coverage Analysis

4 lines missing coverage in roamer.go:620 area. Looking at the code, these appear to be error paths that are hard to trigger in tests (e.g., io.ReadAll failure). Consider:

• Adding a test with a failing reader (using a mock)
• Or accepting the coverage gap as reasonable for exceptional error paths

---

Final Verdict

Approve with minor changes recommended. The implementation is solid and well-tested. The critical issues are primarily about error messaging and documentation clarity rather than functional bugs.

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

PreserveBody skipped on body-tag assignment errors

If assigning a decoder:"body" field fails, the function returns from this branch before reaching the restoration block at the end of parseBody, so with WithPreserveBody enabled the request body stays consumed when an assignment error occurs after a successful decode (e.g., a decoder:"body" field typed as int). Downstream handlers that rely on a preserved body will hit EOF even though preservation was requested.

Useful? React with 👍 / 👎.