tailcallhq / tailcall

High Performance GraphQL Runtime
https://tailcall.run
Apache License 2.0
1.28k stars 253 forks source link

bug(grpc): Enums with aliases don't work in GRPC #1626

Closed tusharmath closed 4 months ago

tusharmath commented 6 months ago

Proto File

syntax = "proto3";

package greetings;

service Greeter {
  rpc SayHello (HelloRequest) returns (HelloReply) {}
}

message HelloRequest {
  string name = 1;
}

message HelloReply {
  string message = 1;
  repeated OBJECT_TYPE objects = 2;
}

enum OBJECT_TYPE {
  option allow_alias   = true;
  ONE = 0;
  TWO = 1;
  O_NE = 0;
  T_WO = 1;
}

Configuration

schema
  @server(port: 8000, graphiql: true)
  @upstream(baseURL: "...", httpCache: true, batch: {delay: 10})
  @link(src: "./enums.proto", type: Protobuf) {
  query: Query
}

type Query {
  hello(name: JSON): HelloReply!
    @grpc(
      method: "greetings.Greeter.SayHello"
      body: "{{args.name}}"
    )
}

type HelloReply {
  message: String
  objects: [ObjectType]!
}

enum ObjectType {
  ONE
  TWO
}

Actual

ERROR Invalid Configuration
Caused by:
  • expected Enum type [at Query.hello.@grpc.HelloReply.objects]

Expected Should start the server without errors and successfully query the GRPC endpoint.

tusharmath commented 6 months ago

/bounty 100$

algora-pbc[bot] commented 6 months ago

## 💎 $100 bounty • Tailcall Inc.

### Steps to solve: 1. Start working: Comment /attempt #1626 with your implementation plan 2. Submit work: Create a pull request including /claim #1626 in the PR body to claim the bounty 3. Receive payment: 100% of the bounty is received 2-5 days post-reward. Make sure you are eligible for payouts

🙏 Thank you for contributing to tailcallhq/tailcall! 🧐 Checkout our guidelines before you get started.

Attempt Started (GMT+0) Solution
🟡 @webbdays Apr 2, 2024, 10:07:10 PM WIP
b4s36t4 commented 6 months ago

@tusharmath I have doubt, in the grpc schema we have 4 values inside enum but in the graphql we have only two values which is causing this issue, you want to run the server even there is a mismatch between grpc and graphql?

How you want this to be handled?

ssddOnTop commented 6 months ago

kickstart:

from_proto_file function in protobuf.rs converts descriptor to all fields and messages and it does not preserve options in EnumDescriptorProto.

Along with EnumDescriptorProto, there can be options in: DescriptorProto, FieldDescriptorProto, EnumValueDescriptorProto and FileDescriptorProto. So I think we should have some flow that either separately handles options or some kind of map which can be used directly with ProtobufService

webbdays commented 6 months ago

Hi @tusharmath,

I guess the issue will be resolved with maintaining the enum variants order and checking if enum defined in graphql file is subset of enum values defined in proto file. By default, if number is not given, we take defined order. And then compare. /attempt Raised a pull request. Please review. Thanks.

Algora profile Completed bounties Tech Active attempts Options
@webbdays 1 tailcallhq bounty
Python, Rust,
HTML & more
Cancel attempt
algora-pbc[bot] commented 6 months ago

@webbdays: Reminder that in 1 days the bounty will become up for grabs, so please submit a pull request before then 🙏

webbdays commented 6 months ago

@tusharmath the enums ONE, TWO

here: enum ObjectType { ONE TWO }

should be one of the enums ONE, TWO, O_NE, T_WO

here enum OBJECT_TYPE { option allow_alias = true; ONE = 0; TWO = 1; O_NE = 0; T_WO = 1; }

right?

webbdays commented 6 months ago

i mean subset.

webbdays commented 6 months ago

graphql defined enums should be subset of the enums defined in proto files?

tusharmath commented 6 months ago

In a way, basically all unique variants of the enum should be supported for.

ssddOnTop commented 6 months ago

graphql defined enums should be subset of the enums defined in proto files?

if any only if allow_alias is set to true. If it's set to true then all fields of enum in tailcall config must be subset of proto, else it must be equivalent to enum in proto.

webbdays commented 6 months ago

ok. what i am saying is subset handles both cases A is subset to A which is equivalent case. we dont need to check for allow_alias here.

it was being handled in code: i have tried this


syntax = "proto3";

package greetings;

service Greeter {
  rpc SayHello (HelloRequest) returns (HelloReply) {}
}

message HelloRequest {
  string name = 1;
}

message HelloReply {
  string message = 1;
  repeated OBJECT_TYPE objects = 2;
}

enum OBJECT_TYPE {
  ONE = 0;
  TWO = 1;
  O_NE = 0;
  T_WO = 1;
}

then tailcall currently raises Invalid configuration error:

ERROR Invalid Configuration
Caused by:
  • enum number '0' has already been used [at Query.hello.@grpc]
webbdays commented 6 months ago

if we set allow_alias option to false and the aliases are there for enum variants then it also throws error.

so what we left to handle is handle if it graphql is subset of proto.

webbdays commented 6 months ago
syntax = "proto3";

package greetings;

service Greeter {
  rpc SayHello (HelloRequest) returns (HelloReply) {}
}

message HelloRequest {
  string name = 1;
}

message HelloReply {
  string message = 1;
  repeated OBJECT_TYPE objects = 2;
}

enum OBJECT_TYPE {
  option allow_alias = false;
  ONE = 0;
  TWO = 1;
  O_NE = 0;
  T_WO = 1;
}

error:


ERROR Invalid Configuration
Caused by:
  • enum number '0' has already been used [at Query.hello.@grpc]
webbdays commented 6 months ago
syntax = "proto3";

package greetings;

service Greeter {
  rpc SayHello (HelloRequest) returns (HelloReply) {}
}

message HelloRequest {
  string name = 1;
}

message HelloReply {
  string message = 1;
  repeated OBJECT_TYPE objects = 2;
}

enum OBJECT_TYPE {
  option allow_alias = true;
  ONE = 0;
  TWO = 1;
  O_NE = 0;
  T_WO = 1;
}

Error:


ERROR Invalid Configuration
Caused by:
  • expected {"ONE", "TWO"} but found {"ONE", "O_NE", "TWO", "T_WO"} [at Query.hello.@grpc.HelloReply.objects]

in this case, we just need to validate properly. need to check if it is subset or not. like is {"ONE"} subset to {"ONE", "O_NE"}? is {"TWO"} subset to {"TWO", "T_WO"}?

github-actions[bot] commented 4 months ago

Action required: Issue inactive for 30 days. Status update or closure in 7 days.

github-actions[bot] commented 4 months ago

Issue closed after 7 days of inactivity.