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

All GQL fields are nullable when source is a Protobuf schema. #5590

Open
2 of 4 tasks
Francismb opened this issue Jun 19, 2023 · 8 comments
Open
2 of 4 tasks

All GQL fields are nullable when source is a Protobuf schema. #5590

Francismb opened this issue Jun 19, 2023 · 8 comments

Comments

@Francismb
Copy link
Contributor

Francismb commented Jun 19, 2023

Issue workflow progress

Progress of the issue based on the
Contributor Workflow

Make sure to fork this template and run yarn generate in the terminal.

Please make sure Mesh package versions under package.json matches yours.

  • 2. A failing test has been provided
  • 3. A local solution has been provided
  • 4. A pull request is pending review

Describe the bug

All GraphQL fields are marked as nullable when using a Protobuf schema as a source. Protobuf has 'optional' and 'one of' types to indicate fields as nullable/required. The GraphQL schema generation should use these to indicate if a GraphQL field is nullable.

To Reproduce Steps to reproduce the behavior:
An example repository can be found here: https://github.com/Francismb/graphql-mesh-issue

The protobuf schema with a non-nullable field: https://github.com/Francismb/graphql-mesh-issue/blob/main/api.proto#L6
The generated GraphQL schema with a not required field: https://github.com/Francismb/graphql-mesh-issue/blob/main/.mesh/schema.graphql#L41

Expected behavior
Required fields in Protobuf schemas should ensure that its also required in GraphQL.

Environment:

  • OS: MacOSX
  • @graphql-mesh/...: latest
  • NodeJS: 18.6
@ardatan
Copy link
Owner

ardatan commented Jun 20, 2023

PRs are welcome!

@EmrysMyrddin
Copy link
Collaborator

EmrysMyrddin commented Aug 31, 2023

Hi, I'm not sure it is possible to rely on the optional feature of protobuf to mark every non optional fields as required in GraphQL.

From what I understand of the dedicated documentation section, the default implicit field presence is defined as "A well-formed message can have zero or one of this field". So the default behaviour seems to correspond to the "optional" field of GraphQL.

The difference between implicit field presence and optional is not very clear to me but it seems to be that an optional field is sent over the wire if it's set, wile the default implicit field presence will send the field value over the wire only if it's not the default value.

Don't hesitate to give more information if I've misunderstood something here, I'm not expert in Protobuf 😅

@Q00
Copy link

Q00 commented Oct 25, 2023

Hi @Francismb
I made a new version that makes required field of gql schema.

#6137 check my PR please.

cc. @ardatan @EmrysMyrddin

@ardatan
Copy link
Owner

ardatan commented Oct 25, 2023

I'm not sure non nullable fields are the same with non optional fields of protobuf like @EmrysMyrddin said;

From what I understand of the dedicated documentation section, the default implicit field presence is defined as "A well-formed message can have zero or one of this field". So the default behaviour seems to correspond to the "optional" field of GraphQL.

@Q00
Copy link

Q00 commented Oct 25, 2023

@ardatan Thanks for replying 👍
From the docs, Message fields can be One of the Following.

optional: An optional field is in one of two possible states:

the field is set, and contains a value that was explicitly set or parsed from the wire. It will be serialized to the wire.
the field is unset, and will return the default value. It will not be serialized to the wire.
You can check to see if the value was explicitly set.

repeated: this field type can be repeated zero or more times in a well-formed message. The order of the repeated values will be preserved.

map: this is a paired key/value field type. See Maps for more on this field type.

The well-formed message can have optional, non optional, repeated and map field.
I think A well-formed message can have zero or one of this field implicates a well-formed message can have optional fields which make zero field. So if the proto has optional field, mesh make nullable field otherwise non-nullable field.

@Q00
Copy link

Q00 commented Oct 26, 2023

I misunderstood about non-optional field
There is also no explicit field label and optional label in message.
https://protobuf.dev/programming-guides/field_presence/

In this docs,

Singular proto3 fields of basic types (numeric, string, bytes, and enums) which are defined with the optional label have explicit presence, like proto2 (this feature is enabled by default as release 3.15).

// no explicit field label
syntax = "proto3";
package example;
message Msg {
  int32 foo = 1;
}
// Optional
syntax = "proto3";
package example;
message Msg {
  optional int32 foo = 1;
}

With no explicit field label, let's see golang example. There is no nil. 0 or other value .

m := GetProto()
if m.Foo != 0 {
  // "Clear" the field:
  m.Foo = 0
} else {
  // Default value: field may not have been present.
  m.Foo = 1
}

With Optional field, there is a methd checking presence in its message. In that case, the default is the language-specific null value.

m := GetProto()
// check the field nil
if m.Foo != nil {
  // Clear the field:
  m.Foo = nil
} else {
  // Field is not present, so set it.
  m.Foo = proto.Int32(1)
}

after proto of 3.15 version, the optional field is default enabled. So I think we have to check its presence when we use optional field. I understand you because the non explicit field label indicates that the default value can be not presence. also I think it is very hard to decide the policy about proto. How do you think about this? @ardatan

@EmrysMyrddin
Copy link
Collaborator

For me, the real problem here is that "optional" doesn't really means the same thing in protobuf and graphql.

Proto allows to define a binary data format, so it needs to know which value is present or not. Optional here means that a field can be present or not in the message. But it doesn't say anything about the value itself. The field can be present with a nil value.

In GraphQL, there is not really such thing as "present" field. A field is always here, but can contain a value or not. Optional here means that the value can be null. So it you have a strict value policy here.

And one of the problem here is that it's not easy to map the concept of "null" to protobuf, which is mainly based on Go, where null doesn't exists. In Go, every type has a default value (0 for number, "" for strings...) and nil is just the default value of pointer types. This is why in documentation they talk about the "presence" of a field defaulting to the "default value" instead of null. Because in most cases, nil is not the default value, or can even not be represented in Go.

@Q00
Copy link

Q00 commented Nov 3, 2023

@EmrysMyrddin I agree with you about some point.

I made a brief overview of proto3 before v3.15.

  • All fields are optional: In proto3, there is no concept of required or optional fields. Every field in a proto3 message is optional by default, and you don't need to use the "optional" keyword to indicate this.
  • Default values: If a field is not explicitly set in a proto3 message, it will have its default value when serialized. The default value depends on the field's data type. For example, int32 fields have a default value of 0, string fields have a default value of an empty string (""), and so on.
  • Absence is indicated by default values: In proto3, the absence of a field is indicated by it having its default value. You can check whether a field was explicitly set or not by comparing it to its default value.

But In proto v3.15
You can see the docs proto3 explicit optional.

Let's see the "round trip" case between explicit and implicit presence.

// Client A:
Msg m_a;
m_a.set_foo(1);                  // non-default value
assert(m_a.has_foo());           // OK
Send(m_a.SerializeAsString());   // to client B

// Client B:
Msg m_b;
m_b.ParseFromString(Receive());  // from client A
assert(m_b.foo() == 1);          // OK
Send(m_b.SerializeAsString());   // to client A

// Client A:
m_a.ParseFromString(Receive());  // from client B
assert(m_a.foo() == 1);          // OK
assert(m_a.has_foo());           // OK
m_a.set_foo(0);                  // default value
Send(m_a.SerializeAsString());   // to client B

// Client B:
Msg m_b;
m_b.ParseFromString(Receive());  // from client A
assert(m_b.foo() == 0);          // OK
Send(m_b.SerializeAsString());   // to client A

// Client A:
m_a.ParseFromString(Receive());  // from client B
assert(m_a.foo() == 0);          // OK
assert(m_a.has_foo());           // FAIL

In the docs, it describes this situation that

"If client A depends on explicit presence for foo, then a “round trip” through client B will be lossy from the perspective of client A. In the example, this is not a safe change: client A requires (by assert) that the field is present; even without any modifications through the API, that requirement fails in a value- and peer-dependent case."

So protobuf propose the way to resolve the issue. explicit optional

In this case, there is a need to distinguish proto3 optional with implicit field.
Because the optional of proto3 has a little different meaning. So I think graphql mesh has a responsible for separating optional value with implicit field. If you worry about proto3 and proto2 presence meaning with graphql "presence",
it is a good idea to make an option to select the generation way in grpc.

In summary, the introduction of explicit optional in proto3 starting from version 3.15 addresses some of the challenges in field presence semantics, especially when integrating with technologies like GraphQL. Providing options for controlling how proto3 messages are generated or how GraphQL schemas are defined can be a valuable feature to ensure smooth integration and consistency in graphql-mesh.

This was referenced Apr 30, 2024
This was referenced May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants