Fix: path item objects become block scalars [#3557]

[qawatake]

References to other Issues or PRs

Fixes #3557

Have you read the Contributing Guidelines?

Yes.

Brief description of what is fixed or changed

Fixes a bug where the path item object is marshaled as a block scalar in openapiv2 v2.17.0 and later.

swagger: "2.0"
info:
  title: your/service/v1/your_service.proto
  version: version not set
tags:
  - name: YourService
consumes:
  - application/json
produces:
  - application/json
paths:
  /api/echo: |
    post:
        operationId: YourService_Echo
        responses:
            "200":
                description: A successful response.
                schema:
                    $ref: '#/definitions/v1StringMessage'
            default:
                description: An unexpected error response.
                schema:
                    $ref: '#/definitions/rpcStatus'
        parameters:
            - name: value
              in: query
              required: false
              type: string
        tags:
            - YourService
definitions:
~~ snip ~~

Other comments

Summary by CodeRabbit

• Bug Fix: Updated the MarshalYAML method in generator.go to use a new toYAMLNode method for converting PathItemObject to a YAML node. This change addresses a bug in the go-yaml library.
• Test: Added a new test function TestGenerateYAML in generator_test.go to validate the generation of YAML files from protobuf input.
• New Feature: Introduced a new protobuf file path_item_object.prototext for testing purposes, which includes a service definition and a message type.
• Chore: Modified the go_test rule in BUILD.bazel to include all files under the testdata directory during the build process.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[coderabbitai]

Walkthrough

This pull request introduces enhancements to the YAML generation process in the protoc-gen-openapiv2 package. It improves the MarshalYAML method, adds a new toYAMLNode method, and includes comprehensive tests for YAML generation. Additionally, it updates the Bazel build configuration to include test data files.

Changes

File	Summary
.../genopenapi/generator.go	Improved MarshalYAML method and added toYAMLNode method for better YAML generation.
.../genopenapi/generator_test.go	Introduced TestGenerateYAML function to ensure correct YAML generation.
.../genopenapi/testdata/generator/path_item_object.prototext	Added a new protobuf file for testing purposes.
.../genopenapi/BUILD.bazel	Updated Bazel build configuration to include test data files.

🐇💻

In the land of code where the shadows lie,
A rabbit hops, its gaze held high.
With each key press, a change is spun,
A dance of logic under the binary sun.
YAML nodes now sing with grace,
Tests ensure they're in their place.
The build grows strong, no room for strife,
Such are the joys of a coder's life. 🎉🥕

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• Mention @coderabbitai in any review comment for bot assistance.
• Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

• Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

[coderabbitai]

Review Status

Actionable comments generated: 1

Commits

Files that changed from the base of the PR and between e03da9d and 1c15701 commits.

Files selected for processing (1)

• protoc-gen-openapiv2/internal/genopenapi/generator.go (1 hunks)

Additional comments (Suppressed): 3

protoc-gen-openapiv2/internal/genopenapi/generator.go (3)

• 150-163: The error handling here is correct and the logic for appending to pathObjectNode.Content is improved compared to the old hunk. The new implementation correctly creates a YAML node from PathItemObject and appends it to the content of pathObjectNode.

• 166-190: This workaround for the go-yaml bug seems reasonable, but it's important to keep track of the issue at v3: Invalid output (bad indentation) when an indentation indicator/hint is used in a scalar block go-yaml/yaml#643. Once it's resolved, the commented-out simpler implementation should be reinstated.

- // We can simplify this implementation once the go-yaml bug is resolved. See: https://github.com/go-yaml/yaml/issues/643.
- //
- //	func (pio *openapiPathItemObject) toYAMLNode() (*yaml.Node, error) {
- //		var node yaml.Node
- //		if err := node.Encode(pio); err != nil {
- //			return nil, err
- //		}
- //		return &node, nil
- //	}

• 192-194: No issues found in these lines. The aliasing technique used here is a common way to handle JSON marshaling with embedded structs that also need to be marshaled.

[johanbrandhorst]

Hi, thanks for this PR! Any chance you could add a test file that exhibits the old, bad behaviour, so we know this is now fixed? Thanks!

[coderabbitai]

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between 1c15701 and 2d0feb6 commits.

Files ignored due to filter (1)

• protoc-gen-openapiv2/internal/genopenapi/testdata/generator/path_item_object.swagger.yaml

Files selected for processing (1)

• protoc-gen-openapiv2/internal/genopenapi/generator_test.go (2 hunks)

Additional comments (Suppressed): 2

protoc-gen-openapiv2/internal/genopenapi/generator_test.go (2)

• 1-13: The import of the os package is new and necessary for file operations in the newly added test function TestGenerateYAML. The rest of the imports are unchanged.

• 125-170: This new test function TestGenerateYAML is well written. It reads input from a file, performs code generation, and compares the generated content with an expected output file. This is a good way to ensure that the YAML generation is working as expected.

[coderabbitai]

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between 2d0feb6 and 3bd362b commits.

Files selected for processing (1)

• protoc-gen-openapiv2/internal/genopenapi/generator_test.go (2 hunks)

Additional comments (Suppressed): 2

protoc-gen-openapiv2/internal/genopenapi/generator_test.go (2)

• 1-13: The import of "os" is new in this hunk. Ensure that the os package is used appropriately and securely, especially when dealing with file operations.

• 125-171: This new test function TestGenerateYAML is well written and covers the case where a YAML file is generated from a .prototext file and compared with an expected output. It's good to see that error handling is properly done for file reading and unmarshalling operations. The use of table-driven tests also enhances the maintainability and readability of the test cases.

[coderabbitai]

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between 3bd362b and b67f7c2 commits.

Files selected for processing (1)

• protoc-gen-openapiv2/internal/genopenapi/generator_test.go (2 hunks)

Additional comments (Suppressed): 2

protoc-gen-openapiv2/internal/genopenapi/generator_test.go (2)

• 1-13: The import of the os package is new in this hunk. It's used for reading files in the newly added test function TestGenerateYAML. The cmp package is also imported, which is used for comparing the expected and actual output in the tests.

• 125-171: This new test function TestGenerateYAML validates the generation of YAML files. It reads input from a file, generates code, and compares the output with an expected result. The test ensures that the generated YAML matches the expected output, thereby validating the fix for the original issue. This is a good addition to the test suite as it directly verifies the functionality being addressed by this PR.

[coderabbitai]

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between b67f7c2 and b54c2f3 commits.

Files ignored due to filter (1)

• protoc-gen-openapiv2/internal/genopenapi/testdata/generator/path_item_object.swagger.yaml

Files selected for processing (1)

• protoc-gen-openapiv2/internal/genopenapi/testdata/generator/path_item_object.prototext (1 hunks)

Additional comments (Suppressed): 1

protoc-gen-openapiv2/internal/genopenapi/testdata/generator/path_item_object.prototext (1)

• 1-32: The protobuf definition seems to be correctly defined. The service YourService with the method Echo is well defined and the HTTP binding is correctly set up with a POST request to /api/echo. The message StringMessage is also correctly defined with an optional string field value. The go package option is correctly set to "github.com/yourorg/yourprotos/gen/go/your/service/v1".

[johanbrandhorst]

It looks like you need to regenerate one of the bazel files, there are instructions in CONTRIBUTING.md, thanks!

[johanbrandhorst]

Regenerate bazel files

[coderabbitai]

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between b54c2f3 and 6e3ca9c commits.

Files selected for processing (1)

• protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel (1 hunks)

Additional comments (Suppressed): 2

protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel (2)

• 52-52: The use of glob(["testdata/**"]) ensures that all files under the testdata directory are included during the build process. This is a good practice as it allows for automated testing with various input data.

• 53-53: The embed = [":genopenapi"] line indicates that the genopenapi library is embedded in the test. This is a standard practice in Bazel to include the library being tested.

[johanbrandhorst]

LGTM, thank you!