Skip to content

Fix nested Request & Response messages #203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
antonkomarev wants to merge 5 commits into from
Closed

Fix nested Request & Response messages #203

antonkomarev wants to merge 5 commits into from

Conversation

@antonkomarev
Copy link
Contributor

@antonkomarev antonkomarev commented Jan 6, 2025 •
edited
Loading

Overview

This PR allows to use nested Request messages in service classes (fixes a bug linked below).

What this PR does / why we need it

syntax = "proto3";

package org.example;

service EmployeeService {
  rpc ListEmployee(ListEmployee.Request) returns (ListEmployee.Response);
}

message ListEmployee {
  message Request {
    string organization_id = 1;
  }

  message Response {
    oneof response {
      Result result = 1;
      string access_denied = 2;
    }

    message Result {
      repeated Employee employees = 1;
    }

    message Employee {
      string id = 1;
      string name = 2;
    }
  }
}

Will generate:

interface EmployeeService
{
    public function ListEmployee(
        array $ctx,
        \Org\Example\ListEmployee\Request $req
    ): \Org\Example\ListEmployee\Response;
}

Instead of:

interface EmployeeService
{
    public function ListEmployee(
        array $ctx,
        \Org\Example\Request $req
    ): \Org\Example\Response;
}

Special notes for your reviewer

Does this PR introduce a user-facing change?

Added support of nested Request & Response messages: https://github.com/twirphp/twirp/issues/201

@antonkomarev
Copy link
Contributor Author

antonkomarev commented Jan 6, 2025 •
edited
Loading

@sagikazarmark I'm not good at Go, I'll be glad to get feedback, maybe there are better ways to do this.

@antonkomarev antonkomarev mentioned this pull request Jan 6, 2025
Closed
2 tasks
@antonkomarev
Copy link
Contributor Author

antonkomarev commented Jan 6, 2025

I didn't found any tests, so haven't covered this case :(

@antonkomarev antonkomarev changed the title Fix nested request messages Fix nested Request & Response messages Jan 6, 2025
@sagikazarmark sagikazarmark mentioned this pull request Jan 13, 2025
Merged
@sagikazarmark
Copy link
Member

sagikazarmark commented Jan 13, 2025 •
edited
Loading

Yeah, we should probably add some better test cases for generated code.

In the meantime, can you give this patch a try? #208

(Updated PHP name generation based on the original compiler code: https://github.com/protocolbuffers/protobuf/blob/6e393fd79667597aac2bb42511fb30c31ff1611d/src/google/protobuf/compiler/php/names.cc)

(You can download a prebuilt artifact from this build: https://github.com/twirphp/twirp/actions/runs/12750830294)

@sagikazarmark sagikazarmark added kind/bug Something isn't working area/code-generator labels Jan 13, 2025
@antonkomarev
Copy link
Contributor Author

antonkomarev commented Jan 15, 2025

@sagikazarmark sure, will try to test your patch.

@sagikazarmark
Copy link
Member

sagikazarmark commented Jan 18, 2025

@antonkomarev have you had a chance to test the patch?

@antonkomarev
Copy link
Contributor Author

antonkomarev commented Jan 18, 2025

@sagikazarmark not yet, plan to try it in the following days.

@antonkomarev
Copy link
Contributor Author

antonkomarev commented Jan 18, 2025 •
edited
Loading

@sagikazarmark Yes, it's working as expected!

@antonkomarev antonkomarev deleted the fix-nested-request-messages branch January 18, 2025 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

area/code-generator kind/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Import in service class is broken for nested request and response messages

2 participants

@antonkomarev @sagikazarmark