Sort tags by explicit Swagger annotation first

[same-id]

I believe it what most people would expect, since the order of tags extracted from methods is harder to control, especially if swagger file is merged from multiple proto files.

In addition, allow merging properties.

References to other Issues or PRs

Have you read the Contributing Guidelines?

Brief description of what is fixed or changed

Other comments

Summary by CodeRabbit

• New Feature: Introduced a mergeTags function in template.go to merge existing tags with rendered service tags, enhancing flexibility in handling service tags.
• Test: Added a new test function TestMergeTags in template_test.go to validate the correct merging of tag objects under various scenarios.
• Test: Implemented a helper function MustMarshal in template_test.go for marshalling objects into JSON format, improving test data manipulation.
• Refactor: Updated the applyTemplate function in template.go to use the new mergeTags function, ensuring consistent and efficient tag merging.

[coderabbitai]

Walkthrough

The changes focus on enhancing the handling of service tags in the OpenAPI specification. The code that appends service tags to the Swagger object is replaced with a mergeTags function, which provides more flexibility. Additionally, new test cases are introduced to ensure the correct merging of tag objects.

Changes

File	Summary
.../genopenapi/template.go	Replaces the code appending service tags with a call to mergeTags, which merges existing and rendered service tags, updating description, external docs, and extensions if they're empty in the existing tags.
.../genopenapi/template_test.go	Adds TestMergeTags to test the merging of tag objects and MustMarshal for marshalling an object into JSON format. Modifies assertions and introduces new test cases.

🐇💻

In the land of code where the shadows lie,

Merge tags here, there, low and high.

Tests run fast, and tests run slow,

Ensuring our code has a healthy glow.

So raise your cups, let's toast this feat,

For every bug we did defeat! 🥂🎉

---

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 abc1a23 and c8111e2 commits.

Files selected for processing (2)

• protoc-gen-openapiv2/internal/genopenapi/template.go (3 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template_test.go (1 hunks)

Files skipped from review due to trivial changes (1)

• protoc-gen-openapiv2/internal/genopenapi/template_test.go

Additional comments (Suppressed): 2

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

• 1746-1751: The removal of the s.Tags = append(s.Tags, renderServiceTags(p.Services, p.reg)...) line in this hunk is a significant change. Ensure that all calls to this function throughout the codebase have been updated to match the new implementation.

• 2016-2018: This is a good improvement as it merges existing tags with rendered service tags, updating the description, external docs, and extensions if they are not present in the existing tags. This allows for more control over the order and properties of tags, especially when the Swagger file is merged from multiple proto files.

[coderabbitai]

Review Status

Actionable comments generated: 2

Commits

Files that changed from the base of the PR and between abc1a23 and 24e3116 commits.

Files ignored due to filter (4)

• examples/internal/clients/abe/api/swagger.yaml
• examples/internal/clients/unannotatedecho/api/swagger.yaml
• examples/internal/proto/examplepb/a_bit_of_everything.swagger.json
• examples/internal/proto/examplepb/unannotated_echo_service.swagger.json

Files selected for processing (2)

• protoc-gen-openapiv2/internal/genopenapi/template.go (3 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template_test.go (1 hunks)

Additional comments (Suppressed): 2

protoc-gen-openapiv2/internal/genopenapi/template_test.go (1)

• 10108-10299: The TestMergeTags function is well structured and covers different scenarios for merging tags. It's good to see that you're using table-driven tests which make it easy to add new test cases. However, consider adding more edge cases such as when both existingTags and newTags are empty or when they have duplicate entries.

protoc-gen-openapiv2/internal/genopenapi/template.go (1)

• 2016-2018: Ensure that the mergeTags function is correctly implemented and tested. Also, verify that all calls to this function throughout the codebase have been updated to match the new usage.

[coderabbitai]

Review Status

Actionable comments generated: 3

Commits

Files that changed from the base of the PR and between 3f5aeb6 and 4e534b3 commits.

Files ignored due to filter (4)

• examples/internal/clients/abe/api/swagger.yaml
• examples/internal/clients/unannotatedecho/api/swagger.yaml
• examples/internal/proto/examplepb/a_bit_of_everything.swagger.json
• examples/internal/proto/examplepb/unannotated_echo_service.swagger.json

Files selected for processing (2)

• protoc-gen-openapiv2/internal/genopenapi/template.go (3 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template_test.go (2 hunks)

Additional comments (Suppressed): 3

protoc-gen-openapiv2/internal/genopenapi/template_test.go (1)

• 10105-10296: The new test cases for the mergeTags function are comprehensive and cover various scenarios including merging descriptions, external docs, and extensions. This is good practice as it ensures the function behaves as expected under different conditions.

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

• 1746-1751: The removal of the service tag appending logic from this section and its replacement with a call to mergeTags in lines 2016-2018 is a good change. It ensures that existing tags are not blindly overwritten, but rather updated if necessary, preserving any existing data.

• 2013-2022: This change correctly introduces the call to mergeTags only when service tags are not disabled. This preserves the original behavior of the code while adding the new functionality of merging tags instead of simply appending them.

[coderabbitai]

Review Status

Actionable comments generated: 1

Commits

Files that changed from the base of the PR and between 5074ccc and 310f852 commits.

Files ignored due to filter (4)

• examples/internal/clients/abe/api/swagger.yaml
• examples/internal/clients/unannotatedecho/api/swagger.yaml
• examples/internal/proto/examplepb/a_bit_of_everything.swagger.json
• examples/internal/proto/examplepb/unannotated_echo_service.swagger.json

Files selected for processing (2)

• protoc-gen-openapiv2/internal/genopenapi/template.go (3 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template_test.go (2 hunks)

Additional comments (Suppressed): 4

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

• 10101-10107: The MustMarshal function uses panic to handle errors. While this is acceptable in a testing context, it's generally better to avoid panics in your code whenever possible. Panics are often indicative of programmer error and can make debugging more difficult due to the abrupt termination of the program. However, given that this is a test utility function and the MustFoo pattern is common in Go for initialization functions where failure should halt execution, this usage is acceptable.

• 10109-10300: The new test cases for the mergeTags function are comprehensive and cover various scenarios including simple merge, merging descriptions, external docs, and extensions. This will help ensure that the mergeTags function behaves as expected under different conditions.

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

• 1746-1751: The removal of the s.Tags = append(s.Tags, renderServiceTags(p.Services, p.reg))... line is consistent with the changes made in lines 2016-2018 where the tags are now merged instead of simply appended. This change should provide more flexibility in handling service tags as described in the PR summary.

• 2016-2018: This new code replaces the previous appending of service tags with a merge operation. Ensure that the mergeTags function behaves as expected and doesn't introduce any unintended side effects.

[johanbrandhorst]

This seems a harmless enough change to me. What was the old behavior? I agree this seems to be what users would expect.

[johanbrandhorst]

Thanks for your contribution!

[same-id]

This seems a harmless enough change to me. What was the old behavior? I agree this seems to be what users would expect.

The old behavior was that the order of tags was not controllable if you had multiple files. Unless you disable the service tags and then specify a tag for each method by hand.
If you would set the same service tags manually, by order, it would not affect anything (would also duplicate the tags emitted)