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

GRPCProtobufPayload issue with import proto in another proto #738

Closed
Banck opened this issue Mar 7, 2020 · 1 comment
Closed

GRPCProtobufPayload issue with import proto in another proto #738

Banck opened this issue Mar 7, 2020 · 1 comment
Labels
kind/bug Feature doesn't work as expected.

Comments

@Banck
Copy link

Banck commented Mar 7, 2020

Hello!
Let's imagine we have 2 (or more) protos. And in one proto we imported another one.
In this case, in every *.grpc.swift will be generated extensions for the same messages with conform GRPCProtobufPayload protocol.
For example:
Devices has RegisterRequest and ResponseRequest.
And Video looks like:

syntax = "proto3";

option java_package = "*";

import "google/protobuf/empty.proto";
import "devices_services.proto";

service Videos {
    rpc test (RegisterRequest) returns (RegisterResponse);
}

Then in devices_services.grpc will be

/// Provides conformance to `GRPCPayload` for the request and response messages
extension RegisterRequest: GRPCProtobufPayload {}
extension RegisterResponse: GRPCProtobufPayload {}

and in videos_services.grpc will be the same lines:

/// Provides conformance to `GRPCPayload` for the request and response messages
extension RegisterRequest: GRPCProtobufPayload {}
extension RegisterResponse: GRPCProtobufPayload {}
@glbrntt glbrntt added kind/bug Feature doesn't work as expected. nio labels Mar 7, 2020
@glbrntt
Copy link
Collaborator

glbrntt commented Mar 7, 2020

Hey @Banck -- thanks for the report; I'm aware of this and will look into it.

As a precautionary note: as the grpc-swift API hasn't fully stabilised yet you should prefer pinning to a tagged version, i.e. 1.0.0-alpha.9, and build the protoc plugin at that same tag. (I say this as the issue you mention here hasn't yet been tagged.)

@glbrntt glbrntt closed this as completed Mar 10, 2020
glbrntt added a commit to glbrntt/grpc-swift that referenced this issue Jul 10, 2020
…rver

Motivation:

To support payloads other than `SwiftProtobuf.Message` we required that
all messages conform to `GRPCPayload`. For protobuf messages we added
`GRPCProtobufPayload` which provides a default implemenation of
`GRPCPayload` for protobuf messages. We generated this conformance for
all protobuf messages we saw. This lead to a number issues and
workarounds including: grpc#738, grpc#778, grpc#801, grpc#837, grpc#877, grpc#881.

The intention is to continue to support `GRPCPayload` in addition to
protobuf, however, support for protobuf will not be via the
`GRPCProtobufPayload` protocol.

This PR adjust the server components such they only support
SwiftProtobuf. Once the client side has had the same treatment (and
`GRPCProtobufPayload` no longer inherits from `SwiftProtobuf.Message`),
support for `GRPCPayload` will be added back.

Modifications:

- The `HTTP1ToGRPCServerCodec` has had the message encoding and decoding
  removed. It now deals in `ByteBuffer`s rather than request/response
  messages.
- An additional `GRPCServerCodecHandler` which sits between the
  `HTTP1ToGRPCServerCodec` and `_BaseCallHandler` has been added which
  serializes/deserializes messages.
- Custom payload tests have been commented out. They will return when
  the transition has completed.

Result:

- Servers only support SwiftProtobuf
- Genertic constraints on the server have been removed; the constraints
  are place on the `init` of public handlers instead.
- `GRPCProtobufPayload` is no longer required on the server.
glbrntt added a commit to glbrntt/grpc-swift that referenced this issue Jul 10, 2020
…rver

Motivation:

To support payloads other than `SwiftProtobuf.Message` we required that
all messages conform to `GRPCPayload`. For protobuf messages we added
`GRPCProtobufPayload` which provides a default implemenation of
`GRPCPayload` for protobuf messages. We generated this conformance for
all protobuf messages we saw. This lead to a number issues and
workarounds including: grpc#738, grpc#778, grpc#801, grpc#837, grpc#877, grpc#881.

The intention is to continue to support `GRPCPayload` in addition to
protobuf, however, support for protobuf will not be via the
`GRPCProtobufPayload` protocol.

This PR adjusts server components such that they are not constrained to
`GRPCPayload`. At the moment only `SwiftProtobuf.Message` is supported.
Once the client side has had the same treatment and
`GRPCProtobufPayload` no longer inherits from `SwiftProtobuf.Message`,
support for `GRPCPayload` will be added back.

Modifications:

- The `HTTP1ToGRPCServerCodec` has had the message encoding and decoding
  removed. It now deals in `ByteBuffer`s rather than request/response
  messages.
- An additional `GRPCServerCodecHandler` which sits between the
  `HTTP1ToGRPCServerCodec` and `_BaseCallHandler` has been added which
  serializes/deserializes messages.
- Custom payload tests have been commented out. They will return when
  the transition has completed.

Result:

- Servers only support SwiftProtobuf
- Genertic constraints on the server have been removed; the constraints
  are place on the `init` of public handlers instead.
- `GRPCProtobufPayload` is no longer required on the server.
glbrntt added a commit to glbrntt/grpc-swift that referenced this issue Jul 10, 2020
…rver

Motivation:

To support payloads other than `SwiftProtobuf.Message` we required that
all messages conform to `GRPCPayload`. For protobuf messages we added
`GRPCProtobufPayload` which provides a default implemenation of
`GRPCPayload` for protobuf messages. We generated this conformance for
all protobuf messages we saw. This lead to a number issues and
workarounds including: grpc#738, grpc#778, grpc#801, grpc#837, grpc#877, grpc#881.

The intention is to continue to support `GRPCPayload` in addition to
protobuf, however, support for protobuf will not be via the
`GRPCProtobufPayload` protocol.

This PR adjusts server components such that they are not constrained to
`GRPCPayload`. At the moment only `SwiftProtobuf.Message` is supported.
Once the client side has had the same treatment and
`GRPCProtobufPayload` no longer inherits from `SwiftProtobuf.Message`,
support for `GRPCPayload` will be added back.

Modifications:

- The `HTTP1ToGRPCServerCodec` has had the message encoding and decoding
  removed. It now deals in `ByteBuffer`s rather than request/response
  messages.
- An additional `GRPCServerCodecHandler` which sits between the
  `HTTP1ToGRPCServerCodec` and `_BaseCallHandler` has been added which
  serializes/deserializes messages.
- Custom payload tests have been commented out. They will return when
  the transition has completed.

Result:

- Servers only support SwiftProtobuf
- Genertic constraints on the server have been removed; the constraints
  are place on the `init` of public handlers instead.
- `GRPCProtobufPayload` is no longer required on the server.
glbrntt added a commit to glbrntt/grpc-swift that referenced this issue Jul 10, 2020
…rver

Motivation:

To support payloads other than `SwiftProtobuf.Message` we required that
all messages conform to `GRPCPayload`. For protobuf messages we added
`GRPCProtobufPayload` which provides a default implemenation of
`GRPCPayload` for protobuf messages. We generated this conformance for
all protobuf messages we saw. This lead to a number issues and
workarounds including: grpc#738, grpc#778, grpc#801, grpc#837, grpc#877, grpc#881.

The intention is to continue to support `GRPCPayload` in addition to
protobuf, however, support for protobuf will not be via the
`GRPCProtobufPayload` protocol.

This PR adjusts server components such that they are not constrained to
`GRPCPayload`. At the moment only `SwiftProtobuf.Message` is supported.
Once the client side has had the same treatment and
`GRPCProtobufPayload` no longer inherits from `SwiftProtobuf.Message`,
support for `GRPCPayload` will be added back.

Modifications:

- The `HTTP1ToGRPCServerCodec` has had the message encoding and decoding
  removed. It now deals in `ByteBuffer`s rather than request/response
  messages.
- An additional `GRPCServerCodecHandler` which sits between the
  `HTTP1ToGRPCServerCodec` and `_BaseCallHandler` has been added which
  serializes/deserializes messages.
- Custom payload tests have been commented out. They will return when
  the transition has completed.

Result:

- Servers only support SwiftProtobuf
- Genertic constraints on the server have been removed; the constraints
  are place on the `init` of public handlers instead.
- `GRPCProtobufPayload` is no longer required on the server.
glbrntt added a commit to glbrntt/grpc-swift that referenced this issue Jul 10, 2020
…rver

Motivation:

To support payloads other than `SwiftProtobuf.Message` we required that
all messages conform to `GRPCPayload`. For protobuf messages we added
`GRPCProtobufPayload` which provides a default implemenation of
`GRPCPayload` for protobuf messages. We generated this conformance for
all protobuf messages we saw. This lead to a number issues and
workarounds including: grpc#738, grpc#778, grpc#801, grpc#837, grpc#877, grpc#881.

The intention is to continue to support `GRPCPayload` in addition to
protobuf, however, support for protobuf will not be via the
`GRPCProtobufPayload` protocol.

This PR adjusts server components such that they are not constrained to
`GRPCPayload`. At the moment only `SwiftProtobuf.Message` is supported.
Once the client side has had the same treatment and
`GRPCProtobufPayload` no longer inherits from `SwiftProtobuf.Message`,
support for `GRPCPayload` will be added back.

Modifications:

- The `HTTP1ToGRPCServerCodec` has had the message encoding and decoding
  removed. It now deals in `ByteBuffer`s rather than request/response
  messages.
- An additional `GRPCServerCodecHandler` which sits between the
  `HTTP1ToGRPCServerCodec` and `_BaseCallHandler` has been added which
  serializes/deserializes messages.
- Custom payload tests have been commented out. They will return when
  the transition has completed.

Result:

- Servers only support SwiftProtobuf
- Genertic constraints on the server have been removed; the constraints
  are place on the `init` of public handlers instead.
- `GRPCProtobufPayload` is no longer required on the server.
glbrntt added a commit that referenced this issue Jul 13, 2020
…rver (#886)

Motivation:

To support payloads other than `SwiftProtobuf.Message` we required that
all messages conform to `GRPCPayload`. For protobuf messages we added
`GRPCProtobufPayload` which provides a default implemenation of
`GRPCPayload` for protobuf messages. We generated this conformance for
all protobuf messages we saw. This lead to a number issues and
workarounds including: #738, #778, #801, #837, #877, #881.

The intention is to continue to support `GRPCPayload` in addition to
protobuf, however, support for protobuf will not be via the
`GRPCProtobufPayload` protocol.

This PR adjusts server components such that they are not constrained to
`GRPCPayload`. At the moment only `SwiftProtobuf.Message` is supported.
Once the client side has had the same treatment and
`GRPCProtobufPayload` no longer inherits from `SwiftProtobuf.Message`,
support for `GRPCPayload` will be added back.

Modifications:

- The `HTTP1ToGRPCServerCodec` has had the message encoding and decoding
  removed. It now deals in `ByteBuffer`s rather than request/response
  messages.
- An additional `GRPCServerCodecHandler` which sits between the
  `HTTP1ToGRPCServerCodec` and `_BaseCallHandler` has been added which
  serializes/deserializes messages.
- Custom payload tests have been commented out. They will return when
  the transition has completed.

Result:

- Servers only support SwiftProtobuf
- Genertic constraints on the server have been removed; the constraints
  are place on the `init` of public handlers instead.
- `GRPCProtobufPayload` is no longer required on the server.
glbrntt added a commit to glbrntt/grpc-swift that referenced this issue Jul 14, 2020
Motivation:

To support payloads other than `SwiftProtobuf.Message` we required that
all messages conform to `GRPCPayload`. For protobuf messages we added
`GRPCProtobufPayload` which provides a default implemenation of
`GRPCPayload` for protobuf messages. We generated this conformance for
all protobuf messages we saw. This lead to a number issues and
workarounds including: grpc#738, grpc#778, grpc#801, grpc#837, grpc#877, grpc#881.

The intention is to continue to support `GRPCPayload` in addition to
protobuf, however, support for protobuf will not be via the
`GRPCProtobufPayload` protocol.

This PR builds on grpc#886 by increasing the surface area of the client APIs
so that they are not constrained to `GRPCPayload`. The surface API now
has variants for `GRPCPayload` and `SwiftProtobuf.Message`. Internally
the client deals with serializers and deserializers.

Modifications:

- `GRPCClientChannelHandler` and `GRPCClientStateMachine` are no longer
  generic over a request and response type, rather they deal with the
  serialzed version of requests and response (i.e. `ByteBuffer`s) and
  defer the (de/)serialization to a separate handler.
- Added `GRCPClientCodecHandler` to handle (de/)serialization of
  messages
- Clients are no longer constrained to having their request/response
  payloads conform to `GRPCPayload`
- Conformance to `GRPCProtobufPayload` is no longer generated and the
  protocol is deprecated and has no requirements.
- Drop the 'GenerateConformance' option from the codegen since it is no
  longer required
- Reintroduce a filter to the codegen so that we only consider files
  which contain services, this avoids generating empty files
- Regenerate code where necessary

Result:

- `GRPCProtobufPayload` is no longer required
glbrntt added a commit to glbrntt/grpc-swift that referenced this issue Jul 14, 2020
Motivation:

To support payloads other than `SwiftProtobuf.Message` we required that
all messages conform to `GRPCPayload`. For protobuf messages we added
`GRPCProtobufPayload` which provides a default implemenation of
`GRPCPayload` for protobuf messages. We generated this conformance for
all protobuf messages we saw. This lead to a number issues and
workarounds including: grpc#738, grpc#778, grpc#801, grpc#837, grpc#877, grpc#881.

The intention is to continue to support `GRPCPayload` in addition to
protobuf, however, support for protobuf will not be via the
`GRPCProtobufPayload` protocol.

This PR builds on grpc#886 by increasing the surface area of the client APIs
so that they are not constrained to `GRPCPayload`. The surface API now
has variants for `GRPCPayload` and `SwiftProtobuf.Message`. Internally
the client deals with serializers and deserializers.

Modifications:

- `GRPCClientChannelHandler` and `GRPCClientStateMachine` are no longer
  generic over a request and response type, rather they deal with the
  serialzed version of requests and response (i.e. `ByteBuffer`s) and
  defer the (de/)serialization to a separate handler.
- Added `GRCPClientCodecHandler` to handle (de/)serialization of
  messages
- Clients are no longer constrained to having their request/response
  payloads conform to `GRPCPayload`
- Conformance to `GRPCProtobufPayload` is no longer generated and the
  protocol is deprecated and has no requirements.
- Drop the 'GenerateConformance' option from the codegen since it is no
  longer required
- Reintroduce a filter to the codegen so that we only consider files
  which contain services, this avoids generating empty files
- Regenerate code where necessary

Result:

- `GRPCProtobufPayload` is no longer required
glbrntt added a commit to glbrntt/grpc-swift that referenced this issue Jul 14, 2020
Motivation:

To support payloads other than `SwiftProtobuf.Message` we required that
all messages conform to `GRPCPayload`. For protobuf messages we added
`GRPCProtobufPayload` which provides a default implemenation of
`GRPCPayload` for protobuf messages. We generated this conformance for
all protobuf messages we saw. This lead to a number issues and
workarounds including: grpc#738, grpc#778, grpc#801, grpc#837, grpc#877, grpc#881.

The intention is to continue to support `GRPCPayload` in addition to
protobuf, however, support for protobuf will not be via the
`GRPCProtobufPayload` protocol.

This PR builds on grpc#886 by increasing the surface area of the client APIs
so that they are not constrained to `GRPCPayload`. The surface API now
has variants for `GRPCPayload` and `SwiftProtobuf.Message`. Internally
the client deals with serializers and deserializers.

Modifications:

- `GRPCClientChannelHandler` and `GRPCClientStateMachine` are no longer
  generic over a request and response type, rather they deal with the
  serialzed version of requests and response (i.e. `ByteBuffer`s) and
  defer the (de/)serialization to a separate handler.
- Added `GRCPClientCodecHandler` to handle (de/)serialization of
  messages
- Clients are no longer constrained to having their request/response
  payloads conform to `GRPCPayload`
- Conformance to `GRPCProtobufPayload` is no longer generated and the
  protocol is deprecated and has no requirements.
- Drop the 'GenerateConformance' option from the codegen since it is no
  longer required
- Reintroduce a filter to the codegen so that we only consider files
  which contain services, this avoids generating empty files
- Regenerate code where necessary

Result:

- `GRPCProtobufPayload` is no longer required
glbrntt added a commit that referenced this issue Jul 14, 2020
Motivation:

To support payloads other than `SwiftProtobuf.Message` we required that
all messages conform to `GRPCPayload`. For protobuf messages we added
`GRPCProtobufPayload` which provides a default implemenation of
`GRPCPayload` for protobuf messages. We generated this conformance for
all protobuf messages we saw. This lead to a number issues and
workarounds including: #738, #778, #801, #837, #877, #881.

The intention is to continue to support `GRPCPayload` in addition to
protobuf, however, support for protobuf will not be via the
`GRPCProtobufPayload` protocol.

This PR builds on #886 by increasing the surface area of the client APIs
so that they are not constrained to `GRPCPayload`. The surface API now
has variants for `GRPCPayload` and `SwiftProtobuf.Message`. Internally
the client deals with serializers and deserializers.

Modifications:

- `GRPCClientChannelHandler` and `GRPCClientStateMachine` are no longer
  generic over a request and response type, rather they deal with the
  serialzed version of requests and response (i.e. `ByteBuffer`s) and
  defer the (de/)serialization to a separate handler.
- Added `GRCPClientCodecHandler` to handle (de/)serialization of
  messages
- Clients are no longer constrained to having their request/response
  payloads conform to `GRPCPayload`
- Conformance to `GRPCProtobufPayload` is no longer generated and the
  protocol is deprecated and has no requirements.
- Drop the 'GenerateConformance' option from the codegen since it is no
  longer required
- Reintroduce a filter to the codegen so that we only consider files
  which contain services, this avoids generating empty files
- Regenerate code where necessary

Result:

- `GRPCProtobufPayload` is no longer required
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Feature doesn't work as expected.
Projects
None yet
Development

No branches or pull requests

2 participants