feat: partial message created as named definitions #3743

nkcr · 2023-11-15T16:45:11Z

I'm taking over work from #3717

References to other Issues or PRs

Brief description of what is fixed or changed

Useful for proto Messages that are used in a POST method for example where the properties of the proto message are split between path params and body params:
For downstream code generator, you want the body params to be wrapped inside a named swagger definitions object.

Other comments

The name definition uses the method FQN, which is made to be unique (It uses the same mechanism as for the message definitions). It will use the same naming strategy.

RESULT:

Before, with a proto like:

service Test{
    rpc MethodX (Request) returns (Reply) {
        option (google.api.http) = {
            post: "/v1/projects/someotherpathX/{id}"
            body: "*"
        };
    }
    rpc MethodY (Request) returns (Reply) {
        option (google.api.http) = {
            post: "/v1/projects/someotherpathY"
            body: "*"
        };
    }
    rpc MetodZ (RequestZ) returns (Reply) {
        option (google.api.http) = {
            post: "/v1/projects/someotherpathZ/{id}"
            body: "*"
        };
    }
}
message Request {
    string id = 1;
    string other = 2;
}
message Reply {
    string result = 1;
}
message RequestZ {
    string id = 1;
}

you would get a swagger like that

{
...
    "/v1/projects/someotherpathX/{id}": {
...
        "parameters": [
          {
            "name": "id",
            "in": "path",
            "required": true,
            "type": "string"
          },
          {
            "name": "body",
            "in": "body",
            "required": true,
            "schema": {
              "type": "object",
              "properties": {
                "other": {
                  "type": "string"
                }
              }
            }
          }
        ],
...
    "/v1/projects/someotherpathY": {
...
        "parameters": [
          {
            "name": "body",
            "in": "body",
            "required": true,
            "schema": {
              "$ref": "#/definitions/mypackageRequest"
            }
          }
        ],
...
    "/v1/projects/someotherpathZ/{id}": {
...
        "parameters": [
          {
            "name": "id",
            "in": "path",
            "required": true,
            "type": "string"
          },
          {
            "name": "body",
            "in": "body",
            "required": true,
            "schema": {
              "type": "object"
            }
          }
        ],
...
}

Now you get

...
  "paths": {
    "/v1/projects/someotherpathX/{id}": {
...
        "parameters": [
          {
            "name": "id",
            "in": "path",
            "required": true,
            "type": "string"
          },
          {
            "name": "body",
            "in": "body",
            "required": true,
            "schema": {
              "$ref": "#/definitions/mypackageTestMethodXBody"
            }
        ],
...
    "/v1/projects/someotherpathY": {
...
        "parameters": [
          {
            "name": "body",
            "in": "body",
            "required": true,
            "schema": {
              "$ref": "#/definitions/mypackageRequest"
            }
          }
        ],
...
    "/v1/projects/someotherpathZ/{id}": {
...
        "parameters": [
          {
            "name": "id",
            "in": "path",
            "required": true,
            "type": "string"
          }
        ],
...
}

Summary by CodeRabbit

New Features
- Enhanced search capabilities within the app, enabling more precise schema definition referencing.
- Expanded API functionality with updated method signatures for improved integration and usability.
Improvements
- Refined data structures for better handling of nested and complex types.
Bug Fixes
- Adjusted API method parameters to correctly handle various data inputs.
Documentation
- Added comprehensive tests to ensure template functionality and API consistency.
Refactor
- Streamlined the internal registry to better manage method descriptors.
Chores
- Included new source files to support extended API features and data models.

coderabbitai · 2023-11-15T16:45:18Z

Walkthrough

The changes encompass enhancements to the swagger generation for RPCs with body and path parameters. By introducing named schema definitions instead of anonymous inline schemas, the updated code aligns with best practices for compatibility with code generators. This refactoring facilitates downstream code generation by using a clear naming convention for body schemas.

Changes

File Path	Change Summary
`protoc-gen-openapiv2/.../template.go`	Added imports, a new variable, updated function signatures and logic to handle named schema definitions.
`examples/internal/clients/abe/...`	Multiple files added or modified to reflect new method signatures, updated structs, and new complex data structures.
`internal/descriptor/registry.go`	Added fields and methods to `Registry` to manage method descriptors and naming.

Assessment against linked issues

Objective	Addressed	Explanation
Address the issue with anonymous inline schemas in swagger generation for RPCs with body and path parameters (#3706)	✅
Ensure that swagger references named objects in `definitions` for body schemas (#3706)	✅
Implement a naming convention for body schema to improve compatibility with code generators (#3706)	✅

Poem

In the land of code, where the snippets compile,
A rabbit hopped through, with a refactor's smile.
No more inline chaos in swagger's abode,
Just named schemas dancing in clean, structured code. 🐇✨

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 X ?

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

You can reply to a review comment made by CodeRabbit.
You can tag CodeRabbit on specific lines of code or files in the PR by tagging @coderabbitai in a comment.
You can tag @coderabbitai in a PR comment and ask one-off questions about the PR and the codebase. Use quoted replies to pass the context for follow-up questions.

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

coderabbitai

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 18c472a and 243819e.

Files selected for processing (2)

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

Additional comments: 5

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

7-13: The import of path/filepath is new and should be used in the code. Verify that the new import is used appropriately and that there are no conflicts with existing code.

43-43: The new global variable nonAlphanumeric is a compiled regular expression. Ensure that it is used correctly to replace non-alphanumeric characters where needed, and that it is not recompiled in a loop or frequently called function for performance reasons.

1120-1123: The renderServices function has been updated to include an additional parameter defs openapiDefinitionsObject. This change is significant as it affects the function signature. Ensure that all calls to renderServices are updated to pass the new parameter. Additionally, verify that the defs parameter is used correctly within the function to handle the creation and referencing of schema definitions.

1279-1304: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [1258-1304]

The logic for handling the body of a POST method has been updated. The code now checks if the body is empty and if so, it uses the request type as the body schema. If the body is not empty, it uses the specified field. This logic is crucial for generating accurate Swagger definitions. Ensure that the logic correctly handles all cases, including nested messages and well-known types, and that the Swagger output is as expected.

1766-1771: The renderServices function is called with the new defs parameter. This is a critical change and should be verified to ensure that the defs parameter is being used correctly within the function to populate the Swagger definitions.

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

johanbrandhorst

Thanks for this! I think you need to regenerate the files with the changes, please see CONTRIBUTING.md for a guide.

protoc-gen-openapiv2/internal/genopenapi/template.go

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

nkcr · 2023-11-22T10:18:10Z

@johanbrandhorst thank you for the review. I'm in a busy sprint right now but should be able to address your comments next week.

Useful for proto Message that are used in a POST method for example where the properties of the messages are split between path params and body params. For downstream code generator, you want the body params to be wrapped inside a name swagger definitions object

coderabbitai

Review Status

Actionable comments generated: 7

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 4899774 and ef380ea.

Files ignored due to filter (2)

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

Files selected for processing (10)

examples/internal/clients/abe/BUILD.bazel (1 hunks)
examples/internal/clients/abe/api_a_bit_of_everything_service.go (8 hunks)
examples/internal/clients/abe/model_a_bit_of_everything_1.go (1 hunks)
examples/internal/clients/abe/model_a_bit_of_everything_2.go (2 hunks)
examples/internal/clients/abe/model_a_bit_of_everything_service_deep_path_echo_body.go (1 hunks)
examples/internal/clients/abe/model_a_bit_of_everything_service_deep_path_echo_body_single_nested.go (1 hunks)
examples/internal/clients/abe/model_a_bit_of_everything_service_update_body.go (1 hunks)
examples/internal/clients/abe/model_a_bit_of_everything_service_update_v2_body.go (1 hunks)
protoc-gen-openapiv2/internal/genopenapi/template.go (5 hunks)
protoc-gen-openapiv2/internal/genopenapi/template_test.go (1 hunks)

Files skipped from review due to trivial changes (2)

examples/internal/clients/abe/model_a_bit_of_everything_service_update_body.go
examples/internal/clients/abe/model_a_bit_of_everything_service_update_v2_body.go

Additional comments: 18

examples/internal/clients/abe/BUILD.bazel (1)

22-31: The addition of new source files to the go_library rule in the BUILD.bazel file is consistent with the PR objectives and the AI-generated overview. These files are expected to introduce new functionality or logic as part of the service handling or updates within the project.

examples/internal/clients/abe/api_a_bit_of_everything_service.go (6)

3060-3060: The method signature for ABitOfEverythingServiceDeepPathEcho has been updated to accept a new parameter type ABitOfEverythingServiceDeepPathEchoBody. Ensure all calls to this method are updated to pass the correct type.

5821-5821: The method ABitOfEverythingServicePostWithEmptyBody no longer accepts a body parameter, which aligns with the PR's objective to handle cases with an empty body. Verify that this change is reflected wherever the method is called.

6132-6132: The method ABitOfEverythingServiceUpdate now takes a ABitOfEverythingServiceUpdateBody type for the body parameter. This should be verified across the codebase to ensure compatibility.

6467-6467: The method ABitOfEverythingServiceUpdateV2 has a modified parameter type for abe, changing from ABitOfEverything2 to ABitOfEverything. This is a significant change and should be verified for correct usage throughout the codebase.

6629-6629: The method ABitOfEverythingServiceUpdateV22 now accepts ABitOfEverything1 instead of ABitOfEverything3. This change in parameter type must be verified for correct usage.

6788-6788: The method ABitOfEverythingServiceUpdateV23 has been updated to take ABitOfEverythingServiceUpdateV2Body for the body parameter. This change aligns with the PR's objectives and should be verified for correct usage.
examples/internal/clients/abe/model_a_bit_of_everything_1.go (2)
19-19: The change in the type of SingleNested from V1exampledeepPathsingleNestedNameSingleNested to ABitOfEverythingNested should be verified across the codebase to ensure that all references and usages of this field are updated accordingly.

Verification successful

The verification process has not revealed any issues with the changes made to the ABitOfEverything1 struct. The type change for the SingleNested field and the removal of the Uuid field appear to be consistent with the rest of the codebase. No unresolved references or broken functionality related to these changes have been detected.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for the old type to find any remaining usages that need to be updated.
rg --type go 'V1exampledeepPathsingleNestedNameSingleNested'
# Search for the new type to validate its usage.
rg --type go 'ABitOfEverythingNested'
Length of output: 9426

Script:
#!/bin/bash
# Search for any remaining usages of the `Uuid` field that might need to be updated.
rg --type go 'Uuid'
Length of output: 12489
* 19-19: The removal of the `Uuid` field from `ABitOfEverything1` requires a thorough search throughout the codebase to identify any code that may be referencing this field and update or remove such references to prevent runtime errors.
examples/internal/clients/abe/model_a_bit_of_everything_2.go (1)

57-72: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [22-72]

The addition of omitempty to JSON tags for struct fields in ABitOfEverything2 indicates that these fields are now optional in the JSON output. Ensure that this change aligns with the intended usage of the API and that any client code consuming this API can handle the absence of these fields.

examples/internal/clients/abe/model_a_bit_of_everything_service_deep_path_echo_body_single_nested.go (1)

13-18: The struct ABitOfEverythingServiceDeepPathEchoBodySingleNested is well-defined with appropriate JSON tags and documentation comments. The use of omitempty for optional fields follows Go best practices.
protoc-gen-openapiv2/internal/genopenapi/template.go (6)
1117-1120: The function renderServices has been updated to include a new parameter defs openapiDefinitionsObject. This change aligns with the PR objectives to improve the generation of Swagger specifications. Ensure that all calls to this function have been updated to pass the new parameter.

Verification successful

The verification process has confirmed that the renderServices function has been updated with the new parameter defs openapiDefinitionsObject and that all calls to this function within the codebase have been updated accordingly to include this parameter.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Verify that all calls to renderServices include the new parameter.
ast-grep --lang go --pattern $'renderServices($$$, $$$, $$$, $$$, $$$, $$$, $$$)'
Length of output: 271

Script:
#!/bin/bash
# Search for calls to renderServices that do not include the new parameter.
ast-grep --lang go --pattern $'renderServices($$$, $$$, $$$, $$$, $$$, $$$)'
Length of output: 266
* 1255-1263: The handling of the body parameter in the `renderServices` function has been updated. It now checks if the `Body` is non-nil and then proceeds to handle the body schema generation. This change seems to be part of the PR objectives to handle named schema definitions for message parts used in the body of a POST method.

1284-1295: The code now includes logic to create a named definition for the body schema if the message has properties, and it logs a warning if the message has no properties. This is a significant change that should be verified to ensure it does not introduce any regressions or unexpected behavior.

1315-1325: The renderFieldAsDefinition function is used to generate the schema for a field that is specified as the body of the request. This change seems to be part of the PR objectives to handle named schema definitions for message parts used in the body of a POST method.

1331-1339: The addition of the body parameter to the openapiParameterObject list is conditional on the schema being non-nil. This is a logical change that prevents the addition of an empty or undefined body parameter to the OpenAPI specification.

1763-1763: The call to renderServices within the applyTemplate function has been updated to include the new defs parameter, which is consistent with the changes made to the renderServices function signature.
protoc-gen-openapiv2/internal/genopenapi/template_test.go (1)

3586-3765: The addition of the new test function TestApplyTemplateWithRequestAndBodyParameters is comprehensive and appears to cover the necessary assertions to validate the template application. Ensure that the test cases cover various scenarios, including more complex paths as suggested by johanbrandhorst in the previous comments.

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

examples/internal/clients/abe/model_a_bit_of_everything_2.go

examples/internal/clients/abe/model_a_bit_of_everything_service_deep_path_echo_body.go

johanbrandhorst · 2023-12-19T05:01:50Z

protoc-gen-openapiv2/internal/genopenapi/template.go

+										panic(fmt.Errorf("could not find OpenAPI naming lookup naming strategy '%s'", reg.GetOpenAPINamingStrategy()))
+									}
+									strats := strategyFn([]string{meth.FQMN(), svc.FQSN()})
+									defName := fmt.Sprintf("%sBody", strats[meth.FQMN()])


I'm afraid I don't think this will be sufficient. Services are unique per file, but we support merging files into a single swagger file. I think we'll need to keep track of names and just add something to the end when we discover a collision. It'd need to be carried across all file generations so it'll be a bit more invasive than this. What do you think?

I changed to use the same mechanism as for messages: the FQN of the method coupled with the fullyQualifiedNameToOpenAPIName function. The function will generate the smallest non-ambiguous FQN, based on the naming strategy.

Sounds great, thanks

johanbrandhorst

See comment on name generation

coderabbitai

Review Status

Actionable comments generated: 8

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between ef380ea and 80c0c54.

Files ignored due to filter (7)

examples/internal/clients/abe/api/swagger.yaml
examples/internal/clients/echo/api/swagger.yaml
examples/internal/proto/examplepb/a_bit_of_everything.swagger.json
examples/internal/proto/examplepb/echo_service.swagger.json
examples/internal/proto/examplepb/flow_combination.swagger.json
examples/internal/proto/examplepb/non_standard_names.swagger.json
runtime/internal/examplepb/non_standard_names.swagger.json

Files selected for processing (13)

examples/internal/clients/abe/BUILD.bazel (2 hunks)
examples/internal/clients/abe/api_a_bit_of_everything_service.go (11 hunks)
examples/internal/clients/abe/api_echo_rpc.go (1 hunks)
examples/internal/clients/abe/model_a_bit_of_everything.go (2 hunks)
examples/internal/clients/abe/model_a_bit_of_everything_service_post_with_empty_body_body.go (1 hunks)
examples/internal/clients/abe/model_a_bit_of_everything_service_update_v2_body.go (1 hunks)
examples/internal/clients/abe/model_examplepb_a_bit_of_everything_service_update_body.go (1 hunks)
examples/internal/clients/abe/model_examplepb_required_message_type_request.go (1 hunks)
examples/internal/clients/abe/model_protoexamplepb_foo.go (1 hunks)
examples/internal/clients/echo/api_echo_service.go (3 hunks)
internal/descriptor/registry.go (3 hunks)
internal/descriptor/services.go (1 hunks)
protoc-gen-openapiv2/internal/genopenapi/template.go (6 hunks)

Additional comments: 27

examples/internal/clients/abe/BUILD.bazel (2)
22-36: The inclusion of new Go source files in the go_library target aligns with the PR objectives to support new named definitions for partial messages. Ensure that these files are correctly placed in the repository and that their corresponding Go packages are properly referenced.

Verification successful

The executed shell scripts have verified that the new Go source files mentioned in the PR are indeed present in the examples/internal/clients/abe directory. Additionally, the rg command confirms that these files declare the package abe, which suggests they are correctly placed within the repository and are part of the expected Go package.

Based on the information provided by the scripts, it appears that the changes to the BUILD.bazel file are consistent with the PR objectives and the AI-generated summary. The new files are present and correctly declare their package, which aligns with the expected modifications for supporting new named definitions for partial messages in the Swagger/OpenAPI specifications.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Verify that the new Go files are present in the expected directory.
fd 'model_a_bit_of_everything_service_.*\.go' examples/internal/clients/abe
# Verify that the import paths in the new Go files are correct.
rg 'package abe' examples/internal/clients/abe --files-with-matches
Length of output: 4218
* 49-55: The addition of `model_protoexamplepb_foo.go` is consistent with the PR's aim to enhance Swagger specification generation. Verify that this file is correctly integrated into the build process and that it does not introduce any build issues.
examples/internal/clients/abe/api_a_bit_of_everything_service.go (11)

1210-1210: The function ABitOfEverythingServiceCheckPostQueryParams now accepts interface{} for the singleNested parameter instead of a specific type. This increases flexibility but may introduce type safety issues. Ensure that runtime type assertions are properly handled where this function is called.

2158-2158: The ABitOfEverythingServiceCreateBook function's book parameter has been changed to interface{}. Similar to the previous comment, verify that type assertions are in place to maintain type safety.

3060-3060: The ABitOfEverythingServiceDeepPathEcho function has a new parameter type ABitOfEverythingServiceDeepPathEchoBody for body. Confirm that all invocations of this function have been updated to pass the correct type.

4269-4269: The ABitOfEverythingServiceGetMessageWithBody function now returns interface{}. This change should be accompanied by updates to the calling code to handle the dynamic return type.

5507-5507: The ABitOfEverythingServicePostOneofEnum function's parameter exampleEnum has been changed to interface{}. As with other similar changes, ensure that the calling code is updated to handle the dynamic type.

5822-5822: The ABitOfEverythingServicePostWithEmptyBody function now has a new parameter type ABitOfEverythingServicePostWithEmptyBodyBody for body. Verify that the calling code is updated accordingly.

6135-6135: The ABitOfEverythingServiceUpdate function's body parameter type has been changed to ExamplepbABitOfEverythingServiceUpdateBody. Ensure that all calls to this function are updated to use the new type.

6301-6301: The ABitOfEverythingServiceUpdateBook function's book parameter is now of type interface{}. Verify that the calling code properly handles the dynamic type and performs necessary type assertions.

6470-6470: The ABitOfEverythingServiceUpdateV2 function's abe parameter is now of type interface{}. Ensure that the calling code is updated to handle the dynamic type and that type safety is not compromised.

6632-6632: The ABitOfEverythingServiceUpdateV22 function's abe parameter is now of type interface{}. As with other similar changes, verify that the calling code is updated to handle the dynamic type.

6791-6791: The ABitOfEverythingServiceUpdateV23 function has a new parameter type ABitOfEverythingServiceUpdateV2Body for body. Confirm that all invocations of this function have been updated to pass the correct type.

examples/internal/clients/abe/model_a_bit_of_everything.go (1)

57-72: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [22-69]

The omitempty JSON tag has been added to several fields of the ABitOfEverything struct. This change will cause these fields to be omitted from the JSON output if they are empty, which could affect the behavior of any clients that expect these fields to be present. Ensure that this change is communicated to all clients and that it does not break any existing functionality.

examples/internal/clients/abe/model_a_bit_of_everything_service_update_v2_body.go (1)

13-17: The ABitOfEverythingServiceUpdateV2Body struct includes an omitempty tag for the Abe field. Ensure that it is acceptable for the Abe field to be omitted in the JSON representation when it is empty, as this could affect the behavior of the API if the field is expected to be present.

examples/internal/clients/abe/model_examplepb_a_bit_of_everything_service_update_body.go (1)

18-78: The ExamplepbABitOfEverythingServiceUpdateBody struct has fields without the omitempty JSON tag, which differs from the ABitOfEverything struct where omitempty was added. Ensure consistency in the use of omitempty across similar structs unless there is a specific reason for this discrepancy.

examples/internal/clients/abe/model_examplepb_required_message_type_request.go (1)

15-15: The Foo field in the ExamplepbRequiredMessageTypeRequest struct has been changed to a pointer to ProtoexamplepbFoo. Ensure that all usages of this struct have been updated to handle the pointer type correctly and that the ProtoexamplepbFoo type is properly defined and imported.

examples/internal/clients/abe/model_protoexamplepb_foo.go (1)

13-15: The ProtoexamplepbFoo struct contains a single field Bar of type ExamplepbBar. Ensure that this struct is used appropriately and that the ExamplepbBar type is properly defined and imported. If additional fields are expected in this struct, they should be added.

internal/descriptor/registry.go (3)

31-32: The addition of the meths field to the Registry struct is intended to map fully-qualified method names to their descriptors. Ensure that this new field is populated correctly wherever methods are registered in the registry and that it is utilized appropriately throughout the codebase.

478-483: The new method GetAllFQMethNs has been added to the Registry type. This method should be used wherever a list of all fully-qualified method names is required. Ensure that this method is called instead of manually iterating over the meths map.

486-488: The method SetAllowDeleteBody has been added to the Registry type. This method should be used to configure whether HTTP DELETE methods may have a body. Ensure that this setting is respected in all relevant parts of the codebase.

internal/descriptor/services.go (1)

68-68: The addition of the method descriptor to the meths map in the Registry type ensures that method descriptors can be looked up by their fully-qualified method name. Verify that this map is used appropriately throughout the codebase and that the fully-qualified method name is unique.

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

913-919: The function fullyQualifiedNameToOpenAPIName has been updated to include reg.GetAllFQMethNs() in the mapping resolution. This change ensures that fully qualified method names are also considered when generating OpenAPI names, which is important for accurately representing gRPC service methods in the Swagger/OpenAPI output.

1117-1120: The function renderServices now accepts an additional parameter defs of type openapiDefinitionsObject. This change is necessary to support the creation of named schema definitions for message parts used in the body of a POST method, as mentioned in the PR description. The function's logic will need to ensure that these definitions are correctly populated and referenced in the generated Swagger/OpenAPI output.

1285-1297: The new logic block handles the case when meth.Name is not nil, which is used to modify the behavior of the schema variable and update the defs map. This is part of the new functionality to create named schema definitions. It's important to ensure that the defs map is updated correctly to avoid any potential issues with missing or incorrect schema references in the Swagger/OpenAPI output.

1317-1320: The call to renderFieldAsDefinition within the if b.Body != nil block is correctly placed to handle the rendering of fields as definitions. However, the error handling for this function call is not visible within the provided hunk. It is assumed that any errors returned by renderFieldAsDefinition are handled appropriately elsewhere in the code.

1758-1764: The call to renderServices within applyTemplate function correctly passes the new defs parameter to the function. This is in line with the changes made to the renderServices function signature and is necessary for the correct generation of Swagger/OpenAPI specifications.

protoc-gen-openapiv2/internal/genopenapi/template.go

examples/internal/clients/abe/api_echo_rpc.go

examples/internal/clients/abe/model_a_bit_of_everything.go

examples/internal/clients/abe/model_a_bit_of_everything_service_post_with_empty_body_body.go

examples/internal/clients/echo/api_echo_service.go

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between ef380ea and a12166b.

Files ignored due to filter (7)

examples/internal/clients/abe/api/swagger.yaml
examples/internal/clients/echo/api/swagger.yaml
examples/internal/proto/examplepb/a_bit_of_everything.swagger.json
examples/internal/proto/examplepb/echo_service.swagger.json
examples/internal/proto/examplepb/flow_combination.swagger.json
examples/internal/proto/examplepb/non_standard_names.swagger.json
runtime/internal/examplepb/non_standard_names.swagger.json

Files selected for processing (13)

examples/internal/clients/abe/BUILD.bazel (2 hunks)
examples/internal/clients/abe/api_a_bit_of_everything_service.go (11 hunks)
examples/internal/clients/abe/api_echo_rpc.go (1 hunks)
examples/internal/clients/abe/model_a_bit_of_everything.go (2 hunks)
examples/internal/clients/abe/model_a_bit_of_everything_service_post_with_empty_body_body.go (1 hunks)
examples/internal/clients/abe/model_a_bit_of_everything_service_update_v2_body.go (1 hunks)
examples/internal/clients/abe/model_examplepb_a_bit_of_everything_service_update_body.go (1 hunks)
examples/internal/clients/abe/model_examplepb_required_message_type_request.go (1 hunks)
examples/internal/clients/abe/model_protoexamplepb_foo.go (1 hunks)
examples/internal/clients/echo/api_echo_service.go (3 hunks)
internal/descriptor/registry.go (3 hunks)
internal/descriptor/services.go (1 hunks)
protoc-gen-openapiv2/internal/genopenapi/template.go (4 hunks)

Files skipped from review due to trivial changes (3)

examples/internal/clients/abe/model_a_bit_of_everything_service_post_with_empty_body_body.go
examples/internal/clients/abe/model_a_bit_of_everything_service_update_v2_body.go
examples/internal/clients/abe/model_protoexamplepb_foo.go

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

examples/internal/clients/abe/api_a_bit_of_everything_service.go
examples/internal/clients/abe/api_echo_rpc.go
examples/internal/clients/abe/model_a_bit_of_everything.go
examples/internal/clients/abe/model_examplepb_required_message_type_request.go
examples/internal/clients/echo/api_echo_service.go
internal/descriptor/registry.go
internal/descriptor/services.go

Additional comments: 6

examples/internal/clients/abe/BUILD.bazel (1)

22-36: The addition of new Go source files and a protobuf file to the go_library in BUILD.bazel is noted. Ensure that these new files are correctly integrated into the build process and that their dependencies are managed appropriately.

examples/internal/clients/abe/model_examplepb_a_bit_of_everything_service_update_body.go (1)

1-78: The Go struct ExamplepbABitOfEverythingServiceUpdateBody is well-defined with appropriate JSON tags and omitempty where necessary. However, ensure that the fields with omitempty are optional in your business logic, as their absence won't cause any runtime issues.

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

913-919: The function fullyQualifiedNameToOpenAPIName has been updated to include method names (reg.GetAllFQMethNs()) in the mapping process. Verify that this change is consistent with the intended use of method names in the OpenAPI naming strategy.

1117-1120: The signature of renderServices now includes defs openapiDefinitionsObject as a parameter. This change aligns with the PR's objective to handle named schema definitions. Ensure that the downstream usage of renderServices is updated to pass this new parameter.

1285-1297: The logic to create named schema definitions for message parts used in the body of a POST method is implemented. Ensure that the naming convention for defName (methFQN + "Body") is consistent and does not conflict with existing definitions in the OpenAPI spec.

1758-1764: The applyTemplate function's call to renderServices now includes the s.Definitions parameter. This is a critical change that should be thoroughly tested to ensure that the Swagger output is generated correctly with the new named definitions.

johanbrandhorst

Something strange is happening to the generated swagger files. Can you tell where the missing schema definitions have gone?

examples/internal/proto/examplepb/echo_service.swagger.json

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between a12166b and bc320e0.

Files ignored due to filter (2)

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

Files selected for processing (2)

examples/internal/clients/abe/api_a_bit_of_everything_service.go (6 hunks)
examples/internal/clients/abe/model_a_bit_of_everything_service_update_v2_body.go (1 hunks)

Additional comments: 8

examples/internal/clients/abe/api_a_bit_of_everything_service.go (6)

3060-3060: The method ABitOfEverythingServiceDeepPathEcho has been updated to accept a new parameter type. Ensure that all calls to this method are updated to pass the correct type. Additionally, check if the localVarPostBody is properly assigned before making the HTTP request, as the assignment is not visible in the provided hunk.

5822-5822: The method ABitOfEverythingServicePostWithEmptyBody has been updated to accept a new parameter type. Verify that the localVarPostBody is correctly handled within the method, especially since the method is intended to handle an empty body scenario.

6135-6135: The method ABitOfEverythingServiceUpdate now takes a different body type. Confirm that the localVarPostBody is set appropriately and that the method's logic correctly handles the new body type.

6470-6470: The method ABitOfEverythingServiceUpdateV2 has a modified signature. It is crucial to ensure that the localVarPostBody is correctly populated with the abe parameter and that the optional parameters are handled as expected.

6632-6632: The method ABitOfEverythingServiceUpdateV22 has been updated to accept ABitOfEverything1 as a parameter. Ensure that the localVarPostBody is correctly assigned and that the method's logic is consistent with the new parameter type.

6791-6791: The method ABitOfEverythingServiceUpdateV23 has a new parameter type in its signature. Verify that the localVarPostBody is being correctly utilized within the method and that the new body type is handled properly.
examples/internal/clients/abe/model_a_bit_of_everything_service_update_v2_body.go (2)
13-17: The ABitOfEverythingServiceUpdateV2Body struct is well-defined with appropriate JSON tags. The use of omitempty allows for optional fields in the JSON payload, which is a common practice in API design to avoid sending null or default values. Ensure that the omitempty behavior aligns with the API's requirements for these fields.

14-14: Verify that the ABitOfEverything2 type is correctly defined and that its usage here is consistent with the rest of the codebase.

Verification successful

The shell script has successfully located the definition of ABitOfEverything2 within the Go files of the project. It is defined in the file examples/internal/clients/abe/model_a_bit_of_everything_2.go. This confirms that the type ABitOfEverything2 is indeed defined in the codebase, and its usage in the file model_a_bit_of_everything_service_update_v2_body.go is consistent with the rest of the codebase, as it references an existing type.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for the definition of ABitOfEverything2 in the Go files.
rg --type go 'type ABitOfEverything2 '
Length of output: 133

johanbrandhorst

This looks good to me, thanks so much! I do worry if we've missed any subtleties, but at this point I'm hoping we can get it into the hands of some users to iron out any issues.

johanbrandhorst · 2023-12-20T18:23:25Z

Looks like we got a test failure 🤔.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between bc320e0 and a6e4cd8.

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/template_test.go (9 hunks)

Additional comments: 9

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

3586-3765: The new test function TestApplyTemplateWithRequestAndBodyParameters is comprehensive and covers the setup of protobuf descriptors, messages, and services. It also includes assertions to verify the Swagger generation output. Ensure that the test cases cover all new scenarios introduced by the PR, including edge cases and error handling.

3587-3626: The protobuf descriptor setup for Book and CreateBookRequest appears to be correct and complete. It defines the necessary fields with appropriate types and labels.

3627-3635: The service and method descriptor setup is correct and aligns with the expected structure for a gRPC service definition. The method CreateBook is properly linked to its request and response types.

3637-3656: The message and field descriptor setup is correct. The fields are associated with their respective messages, and the FieldMessage is set for the bookField, which is necessary for nested message types.

3660-3711: The file descriptor and service binding setup is correct. The file descriptor includes all necessary components, such as message types and services, and the service binding specifies the HTTP method and path template.

3713-3728: The registry setup and template application are correctly implemented. The registry is loaded with the file descriptor, and the template is applied to generate the Swagger output.

3729-3758: The assertions in the test function are thorough and check the Swagger version, base path, schemes, consumes, produces, paths, and definitions. Ensure that these assertions align with the expected Swagger output for the changes introduced in the PR.

3760-3764: The debugging section provided in the test function is useful for diagnosing issues when the test fails. It prints out the input and the result, which can aid in troubleshooting.

6860-6860: The registry loading code is repeated across multiple test functions. Ensure that this is necessary for each test case and that any setup specific to each test is handled appropriately.

johanbrandhorst · 2023-12-21T17:07:33Z

Thanks for your contribution!

coderabbitai bot reviewed Nov 15, 2023

View reviewed changes

protoc-gen-openapiv2/internal/genopenapi/template_test.go Outdated Show resolved Hide resolved

johanbrandhorst mentioned this pull request Nov 17, 2023

feat: partial message created as named definitions #3717

Closed

johanbrandhorst requested changes Nov 17, 2023

View reviewed changes

nkcr added 6 commits December 18, 2023 12:52

test: partial message definition

78ecdad

feat: don't render empty body

1de0d45

feat: use package and RPC instead of filepath

87394ec

feat: use FQMN and FQSN instead of package and RPC

e029629

chore: regenerate protos

ef380ea

nkcr force-pushed the 3706_partial_message_as_definitions branch from 243819e to ef380ea Compare December 18, 2023 14:47

coderabbitai bot reviewed Dec 18, 2023

View reviewed changes

johanbrandhorst reviewed Dec 19, 2023

View reviewed changes

johanbrandhorst requested changes Dec 19, 2023

View reviewed changes

feat: use method FQN

7bfd525

coderabbitai bot reviewed Dec 19, 2023

View reviewed changes

nkcr added 2 commits December 19, 2023 15:25

feat: revert don't render empty body

ec84c18

chore: regenerate protos

a12166b

nkcr force-pushed the 3706_partial_message_as_definitions branch from 80c0c54 to a12166b Compare December 19, 2023 14:26

coderabbitai bot reviewed Dec 19, 2023

View reviewed changes

johanbrandhorst requested changes Dec 19, 2023

View reviewed changes

examples/internal/proto/examplepb/echo_service.swagger.json Outdated Show resolved Hide resolved

chore: regenerate clean protos

bc320e0

coderabbitai bot reviewed Dec 20, 2023

View reviewed changes

johanbrandhorst approved these changes Dec 20, 2023

View reviewed changes

johanbrandhorst mentioned this pull request Dec 20, 2023

[Bug #3829] [protoc-gen-openapiv2] consider openapiv2_tag.name attribute when generating ope… #3830

Merged

test: fix unit tests

a6e4cd8

coderabbitai bot reviewed Dec 21, 2023

View reviewed changes

johanbrandhorst approved these changes Dec 21, 2023

View reviewed changes

johanbrandhorst enabled auto-merge (squash) December 21, 2023 17:00

johanbrandhorst merged commit 45648c4 into grpc-ecosystem:main Dec 21, 2023
17 checks passed

renovate bot mentioned this pull request Jan 3, 2024

chore(deps): update all (major) moul/grpcbin#149

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: partial message created as named definitions #3743

feat: partial message created as named definitions #3743

nkcr commented Nov 15, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 15, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

coderabbitai bot left a comment

johanbrandhorst left a comment

nkcr commented Nov 22, 2023

coderabbitai bot left a comment

johanbrandhorst Dec 19, 2023

nkcr Dec 19, 2023

johanbrandhorst Dec 19, 2023

johanbrandhorst left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

johanbrandhorst left a comment

coderabbitai bot left a comment

johanbrandhorst left a comment

johanbrandhorst commented Dec 20, 2023

coderabbitai bot left a comment

johanbrandhorst commented Dec 21, 2023

feat: partial message created as named definitions #3743

feat: partial message created as named definitions #3743

Conversation

nkcr commented Nov 15, 2023 • edited by coderabbitai bot Loading

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Summary by CodeRabbit

coderabbitai bot commented Nov 15, 2023 • edited Loading

Walkthrough

Changes

Assessment against linked issues

Poem

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

coderabbitai bot left a comment

Choose a reason for hiding this comment

johanbrandhorst left a comment

Choose a reason for hiding this comment

nkcr commented Nov 22, 2023

coderabbitai bot left a comment

Choose a reason for hiding this comment

johanbrandhorst Dec 19, 2023

Choose a reason for hiding this comment

nkcr Dec 19, 2023

Choose a reason for hiding this comment

johanbrandhorst Dec 19, 2023

Choose a reason for hiding this comment

johanbrandhorst left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

johanbrandhorst left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

johanbrandhorst left a comment

Choose a reason for hiding this comment

johanbrandhorst commented Dec 20, 2023

coderabbitai bot left a comment

Choose a reason for hiding this comment

johanbrandhorst commented Dec 21, 2023

nkcr commented Nov 15, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 15, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)