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

Nested messages don't have conformance to GRPCProtobufPayload generated #877

Closed
davidpasztor opened this issue Jul 6, 2020 · 4 comments
Closed
Labels
kind/bug Feature doesn't work as expected.

Comments

@davidpasztor
Copy link

davidpasztor commented Jul 6, 2020

Describe the bug

After updating to 1.0.0-alpha.15, updating protobuf to the latest version and regenerating the proto files, the generated code doesn't compile anymore, due to missing extension ProtoType: GRPCProtobufPayload {} conformances for all nested types.

Dependency versions:
grpc-swift 1.0.0-alpha.15
protobuf 3.12.3
swift-protobuf 1.10.0

Xcode 11.5

To reproduce

  1. Create a proto file with messages that contain nested messages.

All.pb

message ProtoMarketData {
    oneof marketData {
        List list = 1;
        Detail detail = 2;
    }

    message List {
        int32 id = 1;
    }

    message Detail {
        int32 id = 1;
    }
}
  1. Generate the swift files
  2. Try to build the swift files
  3. Compiler error

Type 'ProtoMarketData.Detail' does not conform to protocol 'GRPCPayload'

Expected behaviour

The generated code should compile

Additional information

Looking at the All.grpc.swift file, it contains a line extension ProtoMarketData: GRPCProtobufPayload {}, but no extensions for its nested messages, List and Detail.

@davidpasztor davidpasztor added kind/bug Feature doesn't work as expected. nio labels Jul 6, 2020
@davidpasztor davidpasztor changed the title Updating to alpha.15 broken Updating to alpha.15 broke GRPCPayload conformance Jul 6, 2020
@glbrntt glbrntt removed the nio label Jul 6, 2020
@glbrntt glbrntt changed the title Updating to alpha.15 broke GRPCPayload conformance Nested messages don't have conformance to GRPCProtobufPayload generated Jul 6, 2020
@glbrntt
Copy link
Collaborator

glbrntt commented Jul 6, 2020

I don't think we ever explicitly supported this; I think you were just lucky if it worked before. Nevertheless, definitely a bug. I'll have a patch up fixing this shortly.

@davidpasztor
Copy link
Author

davidpasztor commented Jul 6, 2020

@glbrntt thanks for quickly jumping on this! I wasn't aware that this was never explicitly supported, since it was working fine before (now I understand that was just by coincidence).

@glbrntt
Copy link
Collaborator

glbrntt commented Jul 6, 2020

No problem. I don't like it when I break working code!

Feel free to checkout my branch with the fix here to unblock yourself: https://github.com/glbrntt/grpc-swift/tree/gb-conformance-nested-message

@glbrntt
Copy link
Collaborator

glbrntt commented Jul 7, 2020

This was fixed by #879

@glbrntt glbrntt closed this as completed Jul 7, 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