Marshal non proto fields with indents

[gknw]

Fixes #4026

Summary by CodeRabbit

• Tests
  • Enhanced JSON marshaling tests to cover more scenarios including indentation, maps, and structs.

[coderabbitai]

Walkthrough

This update addresses a specific issue with the grpc-gateway library, where JSONPb marshaler was not applying indentation to non-proto fields during JSON marshaling. The changes include adding and updating logic to ensure that indentation is applied correctly to all fields, including non-proto fields. The modifications also involve adding new test cases to cover various scenarios, including indentation for maps and structs, to ensure the desired behavior is achieved.

Changes

File	Change Summary
.../marshal_json_test.go	- Added buf and err declarations. - Updated logic for conditional marshaling with or without indentation. - Added logic for setting indentation and defaultIndent constant. - Updated test cases with indentation scenarios, and for maps and structs.

Assessment against linked issues

Objective	Addressed	Explanation
JSONPb marshaler doesn't use indent on non-proto fields (#4026)	✅

Poem

In the code where bytes align,
A rabbit hopped, fixing lines.
Indents now in their proper place,
Structs and maps, with elegant grace.
🐇 With every hop, a bug's defeat,
In JSON lands, indents so neat.
A tale of code, now finely spun,
By a rabbit's work, now deftly done.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository from git and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[coderabbitai]

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between be8ce5e and 8b853cf.

Files selected for processing (1)

• runtime/marshal_jsonpb.go (1 hunks)

[coderabbitai]

The changes introduced to apply conditional indentation using j.Indent in the marshalNonProtoField function are a direct response to the issue of inconsistent indentation between proto and non-proto fields. This approach ensures that all fields, regardless of their proto status, can be indented consistently if the Indent option is set. This is a crucial improvement for readability and consistency in the JSON output produced by the JSONPb marshaler.

However, it's important to ensure that the indentation logic is thoroughly tested across various data types (e.g., slices, maps, enums) to confirm that the indentation is applied correctly in all scenarios. Additionally, considering the performance implications of using json.MarshalIndent for every non-proto field when indentation is enabled, it would be beneficial to assess the impact on marshaling performance, especially for large or deeply nested data structures.

Consider adding comprehensive tests to cover various data types and scenarios to ensure the indentation is applied correctly and consistently. Additionally, evaluate the performance impact of these changes, especially for large or complex JSON structures, to ensure that the benefits of improved readability do not come at a significant cost to performance.

[johanbrandhorst]

Thanks for the PR! Any chance we could add a test that exhibits the broken behavior fixed by this? Thanks!

[coderabbitai]

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 8b853cf and f8cd1b7.

Files selected for processing (4)

• runtime/marshal_json.go (1 hunks)
• runtime/marshal_json_test.go (3 hunks)
• runtime/marshal_jsonpb.go (3 hunks)
• runtime/marshal_jsonpb_test.go (1 hunks)

Files skipped from review as they are similar to previous changes (1)

• runtime/marshal_jsonpb.go

Additional comments: 4

runtime/marshal_json.go (1)

• 27-30: The implementation of MarshalIndent in the JSONBuiltin struct correctly utilizes json.MarshalIndent from the standard library to format the output with indentation. This addition aligns with the objective of improving the consistency and readability of the JSON output for non-protobuf fields. The method signature and its usage of json.MarshalIndent are appropriate and follow Go's standard practices for JSON marshaling with indentation.

runtime/marshal_json_test.go (3)

• 41-52: The addition of local variables buf and err and the conditional logic to test both Marshal and MarshalIndent methods in TestJSONBuiltinMarshalField are correctly implemented. This ensures that the tests cover scenarios with and without indentation, effectively validating the new functionality. It's good practice to test both paths to ensure the new method behaves as expected under different conditions.
• 152-159: The logic added to TestJSONBuiltinEncoderFields for setting indentation on the JSON encoder when fixt.indent is not empty is correctly implemented. This ensures that the encoder tests also cover scenarios with indentation, aligning with the changes made to support indentation in marshaling. Using type assertion to check if the encoder is of type *json.Encoder and then calling SetIndent is the correct approach for this scenario.
• 210-286: The update to builtinFieldFixtures with new test cases, including scenarios for indentation, is comprehensive and well-thought-out. These test cases effectively cover a wide range of data types and structures, ensuring that the new MarshalIndent method and the existing Marshal method behave correctly across different inputs. The inclusion of various data types and indentation scenarios enhances the test coverage and ensures the robustness of the implementation.

[coderabbitai]

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between f8cd1b7 and e447298.

Files selected for processing (2)

• runtime/marshal_json_test.go (3 hunks)
• runtime/marshal_jsonpb_test.go (1 hunks)

Files skipped from review as they are similar to previous changes (1)

• runtime/marshal_jsonpb_test.go

Additional comments: 4

runtime/marshal_json_test.go (4)

• 41-44: The addition of buf and err declarations at the function scope in TestJSONBuiltinMarshalField is a good practice. It ensures that these variables are reused across different iterations of the loop, which can slightly improve performance by reducing allocations.
• 48-52: The conditional logic for calling m.Marshal or m.MarshalIndent based on the presence of indentation is correctly implemented. However, it's important to ensure that the indent field in builtinFieldFixtures is properly set for all test cases that require indentation. This change effectively tests the new functionality introduced in the PR.
• 210-210: The introduction of the defaultIndent constant is a good practice. It centralizes the definition of the default indentation used across multiple test cases, making the code more maintainable and easier to update if the indentation requirements change in the future.
• 212-294: The update to builtinFieldFixtures with new test cases, including those for indentation scenarios, is comprehensive and well-structured. It covers a wide range of data types and scenarios, ensuring thorough testing of the marshaling functionality with and without indentation. This is crucial for validating the changes made to the marshaling logic in the PR. However, ensure that all edge cases and potential data types that could be encountered in real-world usage are covered by these tests to prevent any regressions or unexpected behavior.

Verification successful

---

The update to builtinFieldFixtures with new test cases, including those for indentation scenarios, has been acknowledged as comprehensive and well-structured. It aims to ensure thorough testing of the marshaling functionality with and without indentation. While the suggestion to cover all edge cases and potential data types is a general best practice, the specific verification of this aspect through automated scripts is not feasible without detailed criteria for what constitutes "all edge cases and potential data types." Therefore, the original approval and suggestion for manual verification stand as a reasonable approach to ensuring the robustness of the test suite.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Placeholder for a deeper analysis script if needed. In this case, manual verification is suggested.
echo "Review the test cases to ensure coverage of all edge cases and data types."

Length of output: 158

[coderabbitai]

The logic to conditionally set indentation for the JSON encoder in TestJSONBuiltinEncoderFields is correctly implemented. Using type assertion to check if enc is a *json.Encoder and then calling SetIndent is the right approach. However, consider adding a comment explaining why the continue statement is used in the else block, as it might not be immediately clear to future maintainers that this case is expected when enc is not a *json.Encoder.

Consider adding a comment explaining the use of continue for clarity.

[coderabbitai]

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between e447298 and 6b320e8.

Files selected for processing (1)

• runtime/marshal_json_test.go (3 hunks)

Additional comments: 3

runtime/marshal_json_test.go (3)

• 41-44: The addition of buf and err declarations at the beginning of the TestJSONBuiltinMarshalField function improves readability and maintains consistency in error handling across different test cases.
• 212-212: The introduction of the defaultIndent constant is a good practice, promoting consistency and maintainability of indentation values across tests.
• 214-296: The update to builtinFieldFixtures with new test cases, including those for indentation scenarios, maps, and structs, is comprehensive and ensures thorough testing of the marshaling functionality. It's important to verify that all new test cases are relevant and accurately reflect the intended use cases for JSON marshaling and unmarshaling, especially with the added complexity of indentation.

Ensure all new test cases in builtinFieldFixtures are relevant and accurately test the marshaling and unmarshaling functionalities, including the handling of indentation.

[coderabbitai]

The updated logic to conditionally call m.Marshal or m.MarshalIndent based on the presence of indentation in the fixture is a good approach to test both marshaling functionalities. However, ensure that the error messages in the t.Errorf calls provide enough context to distinguish between failures in Marshal and MarshalIndent.

- t.Errorf("m.Marshal(%v) failed with %v; want success", fixt.data, err)
+ if len(fixt.indent) == 0 {
+   t.Errorf("m.Marshal(%v) failed with %v; want success", fixt.data, err)
+ } else {
+   t.Errorf("m.MarshalIndent(%v, \"\", \"%s\") failed with %v; want success", fixt.data, fixt.indent, err)
+ }

---

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change

	if len(fixt.indent) == 0 {
	buf, err = m.Marshal(fixt.data)
	} else {
	buf, err = m.MarshalIndent(fixt.data, "", fixt.indent)
	}
	if len(fixt.indent) == 0 {
	buf, err = m.Marshal(fixt.data)
	} else {
	buf, err = m.MarshalIndent(fixt.data, "", fixt.indent)
	}

[coderabbitai]

The logic to set indentation in TestJSONBuiltinEncoderFields by checking if enc is a *json.Encoder and then calling SetIndent is correct and follows best practices for type assertion in Go. However, the error message in line 159 could be more specific about the consequences of not being able to set indentation, such as how it affects the test's ability to verify the correctness of the output.

- t.Errorf("enc is not *json.Encoder, unable to set indentation settings; it is necessary to implement the encoder configuration")
+ t.Errorf("enc is not *json.Encoder, unable to set indentation settings. This failure prevents testing the correctness of indentation in JSON output.")

---

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change

	if fixt.indent != "" {
	if e, ok := enc.(*json.Encoder); ok {
	e.SetIndent("", fixt.indent)
	} else {
	// By default, JSONBuiltin.NewEncoder returns *json.Encoder as runtime.Encoder. Otherwise it's better to fail the tests
	// than skip fixtures with non empty indent
	t.Errorf("enc is not *json.Encoder, unable to set indentation settings; it is necessary to implement the encoder configuration")
	}
	if fixt.indent != "" {
	if e, ok := enc.(*json.Encoder); ok {
	e.SetIndent("", fixt.indent)
	} else {
	// By default, JSONBuiltin.NewEncoder returns *json.Encoder as runtime.Encoder. Otherwise it's better to fail the tests
	// than skip fixtures with non empty indent
	t.Errorf("enc is not *json.Encoder, unable to set indentation settings. This failure prevents testing the correctness of indentation in JSON output.")
	}

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 6b320e8 and 780e50a.

Files selected for processing (1)

• runtime/marshal_json_test.go (3 hunks)

Additional comments: 5

runtime/marshal_json_test.go (5)

• 41-44: The addition of buf and err declarations at the beginning of the TestJSONBuiltinMarshalField function improves readability and reduces redundancy in variable declarations within the loop. This change aligns with best practices for variable scoping and management within test functions.
• 48-57: The updated logic to conditionally call m.Marshal or m.MarshalIndent based on the presence of indentation settings is a clear and effective way to handle different marshaling scenarios within the same test function. This approach ensures that both marshaling methods are adequately tested across various data fixtures.
• 156-166: The logic added to TestJSONBuiltinEncoderFields for setting indentation on the JSON encoder when fixt.indent is not empty is a necessary step to test the indentation functionality thoroughly. The error handling for cases where the encoder is not a *json.Encoder is appropriate, ensuring that the test fails informatively if the setup is incorrect.
• 217-217: The introduction of the defaultIndent constant is a good practice, promoting consistency and maintainability in the test code. Using a constant for repeated values like indentation settings makes future changes easier and less error-prone.
• 219-301: The updates to builtinFieldFixtures with new test cases, including scenarios for indentation, maps, and structs, significantly enhance the test coverage for JSON marshaling and unmarshaling functionalities. These additions ensure that various data types and structures are correctly handled by the marshaling logic, especially with respect to indentation.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 780e50a and 5e47796.

Files selected for processing (1)

• runtime/marshal_json_test.go (3 hunks)

Files skipped from review as they are similar to previous changes (1)

• runtime/marshal_json_test.go

[johanbrandhorst]

Thank you for your contribution!