Fix incorrectly generated repeated_path_param_separator

[500poundbear]

References to other Issues or PRs

Fixes #3696

Have you read the Contributing Guidelines?

Yes

Brief description of what is fixed or changed

collectionFormat specifies the array format (a single parameter with multiple parameter or multiple parameters with the same name) and the separator for array items. source: swagger doc - describing parameters

In the swagger generator of grpc-gateway, collectionFormat was always set to "multi" which disregarded the override from the repeated_path_param_separator option.

This MR fixes that behaviour and amends the relevant existing test cases.

Assumptions: default repeated path parameter separator is multi

Other comments

Question: Should multi be the default repeated path parameter separator?
Question: Should multi be accepted as a new repeated path parameter separator option?

Thank you

Summary by CodeRabbit

• New Features

  • Enhanced parameter handling to support multiple collection formats in API requests.

• Bug Fixes

  • Adjusted the default collection format for array parameters to improve consistency with API standards.

• Tests

  • Added new test cases to validate the handling of different collection formats for query parameters.

[coderabbitai]

Walkthrough

The changes involve updating the handling of repeated path parameters in a codebase, likely related to API endpoint definitions. The default separator for repeated path parameters has been changed from "csv" to "multi", and the logic for setting the collection format of parameters has been adjusted to use this new default. Additionally, tests have been updated to reflect these changes and to cover new scenarios with different collection formats.

Changes

File Path	Change Summary
internal/descriptor/registry.go	Updated repeatedPathParamSeparator from "csv" to "multi" in NewRegistry function.
protoc-gen-openapiv2/internal/genopenapi/template.go	Modified logic to set CollectionFormat using GetRepeatedPathParamSeparatorName.
protoc-gen-openapiv2/internal/genopenapi/template_test.go	Added RepeatedPathParam field and new test cases to TestMessageToQueryParametersWithRepeatedPathParamSeparator.

Poem

🐇 "In the land of code, where paths intertwine,
A rabbit hopped through, making endpoints align.
From CSV to multi, the parameters dance,
A change so swift, it enhanced the expanse." 🌟

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• If you reply to a review comment from CodeRabbit, the bot will automatically respond.
• To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
• Note: Review comments are made on code diffs or files, not on the PR overview.
• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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.

Note: For conversation with the bot, please use the review comments on code diffs or files.

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

[coderabbitai]

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 2671bfc and 8e06127.

Files selected for processing (3)

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

Additional comments: 6

internal/descriptor/registry.go (1)

• 182-187: The change from "csv" to "multi" for the repeatedPathParamSeparator field's name property aligns with the PR objectives to respect the repeated_path_param_separator option specified by the user.

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

• 330-334: The summary indicates a change in the default repeatedPathParamSeparator from "csv" to "multi", but this change is not visible in the provided hunk. Please verify if this change is implemented elsewhere in the codebase.

• 330-334: The update to use reg.GetRepeatedPathParamSeparatorName() for setting the CollectionFormat aligns with the PR objective to respect the user's configuration for the repeated path parameter separator.

• 330-334: The summary mentions a typo in the test struct field RepatedPathParam which should be RepeatedPathParam, but this is not visible in the provided hunk. Please verify if this typo is corrected elsewhere in the codebase.

• 330-334: The summary mentions new test cases added to verify the changes to the CollectionFormat logic, but these test cases are not visible in the provided hunk. Please verify if these test cases are included elsewhere in the codebase.

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

• 497-550: The addition of new test cases to verify different collection formats is confirmed and aligns with the PR objectives.

[johanbrandhorst]

Thanks for fixing this, that's pretty bad 😬. To answer your questions:

Question: Should multi be the default repeated path parameter separator?
We'll have to use whatever we used before as the default, changing it would be breaking, regardless of original intentions.

Question: Should multi be accepted as a new repeated path parameter separator option?
Don't see why not, for users who want to explicitly set the default behavior. Is it not?