Skip to content
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.

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

[Bug]: Protobuf emitter incorrect rpc request type #3534

Closed
4 tasks done
RobertVillalba opened this issue Jun 6, 2024 · 1 comment · Fixed by #3561
Closed
4 tasks done

[Bug]: Protobuf emitter incorrect rpc request type #3534

RobertVillalba opened this issue Jun 6, 2024 · 1 comment · Fixed by #3561
Assignees
Labels
bug Something isn't working needs-area

Comments

@RobertVillalba
Copy link

Describe the bug

The protobuf emitter drops the package prefix from the request type for services that use an imported model as the args.

Reproduction

https://typespec.io/playground?c=aW1wb3J0ICJAdHlwZXNwZWMvcHJvdG9idWYiOw0KDQp1c2luZyBUeXBlU3BlYy5Qxx3FHEBwYWNrYWdlKHsNCiAgbmFtZTogIm15c2hhcmVkIiwNCn0pDQrEF3NwYWNlIE15U8UaIMUvbW9kZWwgRHVtbXnGESAgQGZpZWxkKDEpIHg6IHN0cmluZzvEGn0NCn3ccGVydmljZdVxxhvGYUDoAL0uxzLEFWludGVyZs0uc8YvICBlY2hvKC4uLugAuS7lAK4pOukAysYR6wCk&e=%40typespec%2Fprotobuf&options=%7B%7D

service MyServices {
  rpc Echo(Dummy) returns (myshared.Dummy);
}

Checklist

@witemple-msft
Copy link
Member

Thanks for pointing this out. A bit of missing logic in the emitter was causing the parameters to be considered a local model rather than an external model. The following PR addresses this issue: #3561

github-merge-queue bot pushed a commit that referenced this issue Jun 11, 2024
…ses. (#3561)

Closes #3534
Closes #3556 

This PR adds cross-package resolution in some cases where it was
missing.

In the case of operation inputs, missing effective model type
calculation was causing some input types not to be resolved to external
packages.

In the case of map value types, the calculation of the right hand side
type did not correctly account for the possibility of the type being in
another package, and it now uses the same machinery as other type
references to instrument imports and cross-package references.

The existing scenario test has been extended to prevent regressions and
also extended to test arrays for good measure, though those were working
as expected.

---------

Co-authored-by: Will Temple <will@wtemple.net>
Co-authored-by: Timothee Guerin <tiguerin@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-area
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants