Feat/check field optional proto #6137

Q00 · 2023-10-25T09:24:41Z

🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.

Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.

Description

I want to make proto field not nullable when the proto field doesn't have optional keyword.
Because always proto field has default value ( Int32 -> 0, string -> "").
I saw the #5590 issue and I made the version of fixing it.

This PR makes graphql field required when proto field doesn't have 'optional'.

Fixes #5590 (issue)

Discussion: #6134

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Screenshots/Sandbox (if appropriate/relevant):

after bob build command ( graphql-mesh repository )
after mesh build command ( service )
api.rpto

syntax = "proto3";

package api;

message TestReq {
  optional int32 id = 1;
  optional string id2 = 2;
  string id3 = 3;

}
message TestResp {
  string id4 = 1;
  optional string id5 = 2;
  repeated string raa = 3;
  TestReq tr = 4;
  repeated TestReq ra2 = 5;
}

service EventsService {
  rpc createEvent(TestReq) returns (TestResp);
}

schema.graphql

schema {
  query: Query
  mutation: Mutation
}

directive @grpcMethod(rootJsonName: String, objPath: String, methodName: String, responseStream: Boolean) on FIELD_DEFINITION

directive @grpcConnectivityState(rootJsonName: String, objPath: String) on FIELD_DEFINITION

directive @grpcRootJson(name: String, rootJson: ObjMap, loadOptions: ObjMap) repeatable on OBJECT

"Directs the executor to stream plural fields when the `if` argument is true or undefined."
directive @stream(
  """Stream when true or undefined."""
  if: Boolean! = true
  """Unique name"""
  label: String
  """Number of items to return immediately"""
  initialCount: Int = 0
) on FIELD

type Query @grpcRootJson(name: "Root0", rootJson: "{\"nested\":{\"api\":{\"nested\":{\"TestReq\":{\"oneofs\":{\"_id\":{\"oneof\":[\"id\"]},\"_id2\":{\"oneof\":[\"id2\"]}},\"fields\":{\"id\":{\"type\":\"int32\",\"id\":1,\"options\":{\"proto3_optional\":true},\"comment\":null},\"id2\":{\"type\":\"string\",\"id\":2,\"options\":{\"proto3_optional\":true},\"comment\":null},\"id3\":{\"type\":\"string\",\"id\":3,\"comment\":null}},\"comment\":null},\"TestResp\":{\"oneofs\":{\"_id5\":{\"oneof\":[\"id5\"]}},\"fields\":{\"id4\":{\"type\":\"string\",\"id\":1,\"comment\":null},\"id5\":{\"type\":\"string\",\"id\":2,\"options\":{\"proto3_optional\":true},\"comment\":null},\"raa\":{\"rule\":\"repeated\",\"type\":\"string\",\"id\":3,\"comment\":null},\"tr\":{\"type\":\"TestReq\",\"id\":4,\"comment\":null},\"ra2\":{\"rule\":\"repeated\",\"type\":\"TestReq\",\"id\":5,\"comment\":null}},\"comment\":null},\"EventsService\":{\"methods\":{\"createEvent\":{\"requestType\":\"TestReq\",\"responseType\":\"TestResp\",\"comment\":null}},\"comment\":null}}}}}") {
  api_EventsService_connectivityState(tryToConnect: Boolean): ConnectivityState @grpcConnectivityState(rootJsonName: "Root0", objPath: "api.EventsService")
}

enum ConnectivityState {
  IDLE
  CONNECTING
  READY
  TRANSIENT_FAILURE
  SHUTDOWN
}

type Mutation {
  api_EventsService_createEvent(input: api_TestReq_Input): api_TestResp @grpcMethod(rootJsonName: "Root0", objPath: "api.EventsService", methodName: "createEvent", responseStream: false)
}

type api_TestResp {
  id4: String!
  id5: String
  raa: [String!]!
  tr: api_TestReq!
  ra2: [api_TestReq!]!
}

type api_TestReq {
  id: Int
  id2: String
  id3: String!
}

input api_TestReq_Input {
  id: Int
  id2: String
  id3: String!
}

scalar ObjMap

How Has This Been Tested?

Acceptance Test
Snapshot Test

❯ npx cross-env \"JEST=1\" jest --detectOpenHandles --no-watchman -u
 PASS  packages/handlers/grpc/test/handler.spec.ts
 › 14 snapshots updated.
 PASS  packages/loaders/openapi/tests/schemas.test.ts
 PASS  packages/loaders/soap/test/soap.test.ts
  ● Console

    console.warn
      element.ref isn't supported yet. s:schema

      923 |             } else {
      924 |               if (elementObj.attributes?.ref) {
    > 925 |                 console.warn(`element.ref isn't supported yet.`, elementObj.attributes?.ref);
          |                         ^
      926 |               } else {
      927 |                 console.warn(`Element doesn't have a name in ${complexTypeName}. Ignoring...`);
      928 |               }

      at SOAPLoader.warn [as getOutputTypeForComplexType] (packages/loaders/soap/src/SOAPLoader.ts:925:25)
      at SOAPLoader.getOutputTypeForComplexType [as getOutputTypeForTypeNameInNamespace] (packages/loaders/soap/src/SOAPLoader.ts:1040:19)
      at SOAPLoader.getOutputTypeForTypeNameInNamespace [as getOutputFieldTypeFromElement] (packages/loaders/soap/src/SOAPLoader.ts:843:29)
      at getOutputFieldTypeFromElement (packages/loaders/soap/src/SOAPLoader.ts:909:61)
      at ThunkComposer._thunk (node_modules/graphql-compose/src/TypeMapper.ts:322:21)
      at ThunkComposer.get ofType [as ofType] (node_modules/graphql-compose/src/ThunkComposer.ts:21:34)
      at ThunkComposer.getUnwrappedTC (node_modules/graphql-compose/src/ThunkComposer.ts:41:17)
      at unwrapTC (node_modules/graphql-compose/src/utils/typeHelpers.ts:360:31)
      at unwrapOutputTC (node_modules/graphql-compose/src/utils/typeHelpers.ts:373:10)
      at ObjectTypeComposer.getFieldTC (node_modules/graphql-compose/src/ObjectTypeComposer.ts:635:26)
      at node_modules/graphql-compose/src/SchemaComposer.ts:268:26
          at Array.forEach (<anonymous>)
      at SchemaComposer.removeEmptyTypes (node_modules/graphql-compose/src/SchemaComposer.ts:267:24)
      at node_modules/graphql-compose/src/SchemaComposer.ts:278:18
          at Array.forEach (<anonymous>)
      at SchemaComposer.removeEmptyTypes (node_modules/graphql-compose/src/SchemaComposer.ts:267:24)
      at SchemaComposer.buildSchema (node_modules/graphql-compose/src/SchemaComposer.ts:197:12)
      at SOAPLoader.buildSchema (packages/loaders/soap/src/SOAPLoader.ts:1101:32)
      at Object.buildSchema (packages/loaders/soap/test/soap.test.ts:26:31)

    console.warn
      By default, BigInts are not serialized to JSON as numbers but instead as strings which may lead an unintegrity in your data. To fix this, you can use "json-bigint-patch" to enable correct serialization for BigInts.

      162 |       rootValue = createRootValue(schema, fetchFn, operationHeaders);
      163 |     }
    > 164 |     return execute({
          |                   ^
      165 |       schema,
      166 |       document,
      167 |       rootValue,

      at isBigIntSerializable (node_modules/graphql-scalars/cjs/scalars/BigInt.js:25:21)
      at GraphQLScalarType.parseLiteral (node_modules/graphql-scalars/cjs/scalars/BigInt.js:85:39)
      at valueFromAST (node_modules/graphql/utilities/valueFromAST.js:158:21)
      at valueFromAST (node_modules/graphql/utilities/valueFromAST.js:139:26)
      at getArgumentValues (node_modules/graphql/execution/values.js:246:57)
      at executeField (node_modules/graphql/execution/execute.js:483:48)
      at node_modules/graphql/execution/execute.js:377:22
      at promiseReduce (node_modules/graphql/jsutils/promiseReduce.js:23:9)
      at executeFieldsSerially (node_modules/graphql/execution/execute.js:373:43)
      at executeOperation (node_modules/graphql/execution/execute.js:347:14)
      at execute (node_modules/graphql/execution/execute.js:136:20)
      at soapExecutor (packages/loaders/soap/src/executor.ts:164:19)
      at Object.executor (packages/loaders/soap/test/soap.test.ts:28:31)

 PASS  packages/transforms/cache/test/cache.spec.ts
 PASS  packages/loaders/openapi/tests/example_api.test.ts
 PASS  packages/handlers/graphql/test/handler.spec.ts
 PASS  packages/loaders/openapi/tests/cloudflare.test.ts
 PASS  packages/transforms/encapsulate/test/encapsulate.spec.ts
 PASS  packages/plugins/rate-limit/tests/rate-limit.spec.ts
 PASS  packages/transforms/rate-limit/tests/rate-limit.spec.ts
 PASS  packages/loaders/openapi/tests/looker.test.ts
 PASS  packages/loaders/openapi/tests/docusign.test.ts
 PASS  packages/loaders/raml/tests/query-params.test.ts
 PASS  packages/loaders/json-schema/test/getComposerFromSchema.test.ts
 PASS  packages/plugins/mock/tests/mocking.spec.ts
 PASS  packages/loaders/openapi/tests/example_api7.test.ts
 PASS  packages/handlers/odata/test/handler.spec.ts
 PASS  packages/loaders/json-schema/test/timeout.test.ts
 PASS  packages/loaders/openapi/tests/calendly.test.ts
 PASS  packages/transforms/replace-field/test/replace-field.spec.ts
 PASS  packages/handlers/thrift/test/handler.spec.ts
 PASS  packages/config/test/processConfig.test.ts
 PASS  packages/loaders/openapi/tests/example_api5.test.ts
 PASS  packages/loaders/openapi/tests/authentication.test.ts
 PASS  packages/loaders/openapi/tests/query-params-with-post.test.ts
 PASS  packages/loaders/openapi/tests/spotify.test.ts
 PASS  packages/apollo-link/test/apollo-link.test.ts
 PASS  packages/loaders/openapi/tests/file-upload.test.ts
 PASS  packages/cache/redis/test/cache.spec.ts
 PASS  packages/loaders/openapi/tests/government_social_work.test.ts
 PASS  packages/transforms/filter-schema/test/transform.spec.ts
 PASS  packages/loaders/openapi/tests/example_api6.test.ts
 PASS  packages/transforms/rename/test/wrapRename.spec.ts
 PASS  packages/loaders/json-schema/test/execution.test.ts
 PASS  packages/urql/test/urql-exchange.test.ts
 PASS  packages/loaders/openapi/tests/int64-input.test.ts
  ● Console

    console.warn
      By default, BigInts are not serialized to JSON as numbers but instead as strings which may lead an unintegrity in your data. To fix this, you can use "json-bigint-patch" to enable correct serialization for BigInts.

      25 |
      26 |   it('bigint in input object should be parsed', async () => {
    > 27 |     const result = await execute({
         |                                 ^
      28 |       schema: createdSchema,
      29 |       document: parse(/* GraphQL */ `
      30 |         mutation {

      at isBigIntSerializable (node_modules/graphql-scalars/cjs/scalars/BigInt.js:25:21)
      at GraphQLScalarType.parseLiteral (node_modules/graphql-scalars/cjs/scalars/BigInt.js:85:39)
      at valueFromAST (node_modules/graphql/utilities/valueFromAST.js:158:21)
      at valueFromAST (node_modules/graphql/utilities/valueFromAST.js:139:26)
      at valueFromAST (node_modules/graphql/utilities/valueFromAST.js:69:12)
      at getArgumentValues (node_modules/graphql/execution/values.js:246:57)
      at executeField (node_modules/graphql/execution/execute.js:483:48)
      at node_modules/graphql/execution/execute.js:377:22
      at promiseReduce (node_modules/graphql/jsutils/promiseReduce.js:23:9)
      at executeFieldsSerially (node_modules/graphql/execution/execute.js:373:43)
      at executeOperation (node_modules/graphql/execution/execute.js:347:14)
      at execute (node_modules/graphql/execution/execute.js:136:20)
      at Object.<anonymous> (packages/loaders/openapi/tests/int64-input.test.ts:27:33)

 PASS  packages/plugins/deduplicate-request/tests/useDeduplicateRequest.test.ts
 PASS  packages/loaders/openapi/tests/basket.test.ts
 PASS  packages/runtime/test/getMesh.test.ts
 PASS  packages/loaders/openapi/tests/example_api4.test.ts
 PASS  packages/http/test/http.spec.ts
 PASS  packages/loaders/openapi/tests/nested_objects.test.ts
 PASS  packages/transforms/prefix/test/wrapPrefix.spec.ts
 PASS  packages/loaders/openapi/tests/example_api8.test.ts
 PASS  packages/loaders/openapi/tests/nested-one-of.test.ts
 PASS  packages/loaders/openapi/tests/discriminator.test.ts
 PASS  packages/loaders/openapi/tests/example_api2.test.ts
 PASS  packages/loaders/openapi/tests/query-arguments.test.ts
 PASS  packages/loaders/openapi/tests/example_api_combined.test.ts
 PASS  packages/loaders/openapi/tests/unknown-properties-union.test.ts
 PASS  packages/loaders/openapi/tests/pet.test.ts
 PASS  packages/loaders/openapi/tests/additionalProperties.test.ts
 PASS  packages/loaders/openapi/tests/oneof-without-descriminator.test.ts
 PASS  packages/transforms/naming-convention/test/bareNamingConvention.spec.ts
 PASS  packages/transforms/prefix/test/barePrefix.spec.ts
 PASS  packages/loaders/openapi/tests/escaped-values.test.ts
 PASS  packages/loaders/openapi/tests/cloudfunction.test.ts
 PASS  packages/loaders/openapi/tests/content-type.test.ts
 PASS  packages/loaders/openapi/tests/non_string_links.test.ts
 PASS  packages/loaders/openapi/tests/explode.test.ts
 PASS  packages/loaders/openapi/tests/multiple-responses-swagger.test.ts
 PASS  packages/loaders/openapi/tests/cloudfunction_expanded.test.ts
 PASS  packages/loaders/json-schema/test/query-params.test.ts
 PASS  packages/transforms/naming-convention/test/wrapNamingConvention.spec.ts
 PASS  packages/loaders/soap/test/examples.test.ts
 PASS  packages/transforms/rename/test/bareRename.spec.ts
 PASS  packages/utils/test/get-headers-obj.spec.ts
 PASS  packages/transforms/transfer-schema/test/transfer-schema.spec.ts
 PASS  packages/plugins/http-details-extensions/test/http-details-extensions.test.ts
 PASS  packages/transforms/resolvers-composition/test/transform.spec.ts
 PASS  packages/runtime/test/useSubschema.test.ts
 PASS  packages/transforms/hoist-field/test/transform.spec.ts
 PASS  packages/transforms/federation/test/federation.spec.ts
 PASS  packages/handlers/grpc/test/utils.spec.ts
 PASS  packages/store/test/mesh-store.spec.ts
 PASS  packages/json-machete/tests/dereferenceObject.test.ts
 PASS  packages/json-machete/tests/compareJSONSchemas.test.ts
 PASS  packages/json-machete/tests/referenceJSONSchema.test.ts
 PASS  packages/json-machete/tests/healJSONSchema.test.ts
 PASS  packages/utils/test/pubsub.spec.ts
 PASS  packages/utils/test/read-file-or-url.spec.ts
 PASS  packages/handlers/grpc/test/scalars.spec.ts
 PASS  packages/cache/localforage/test/cache.spec.ts
 PASS  packages/cache/file/test/cache.spec.ts
 PASS  packages/utils/test/with-filter.spec.ts
 PASS  packages/utils/test/sanitize-name-for-graphql.test.ts
 PASS  packages/handlers/tuql/test/handler.spec.ts
 PASS  packages/string-interpolation/tests/parse-interpolation-strings.test.ts
 PASS  packages/cli/test/cli.spec.ts
 PASS  packages/transforms/extend/test/extend.spec.ts
 PASS  packages/handlers/soap/test/handler.spec.ts
 PASS  packages/handlers/postgraphile/test/cli.spec.ts
 PASS  packages/handlers/neo4j/test/handler.spec.ts
 PASS  packages/handlers/mongoose/test/handler.spec.ts
 PASS  packages/string-interpolation/tests/resolver-data-factory.test.ts
 PASS  packages/handlers/mysql/test/handler.spec.ts

Snapshot Summary
 › 14 snapshots updated from 1 test suite.

Test Suites: 1 skipped, 96 passed, 96 of 97 total
Tests:       7 skipped, 567 passed, 574 total
Snapshots:   14 updated, 105 passed, 119 total
Time:        36.874 s
Ran all test suites.

Test Environment:

OS: sonoma, macos
"@graphql-mesh/cli": "^0.83.3",
"@graphql-mesh/grpc": "^0.94.4",
NodeJS: v16.14.2

Checklist:

I have followed the
CONTRIBUTING doc and the
style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests and linter rules pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Further comments

I made a new snapshot of required field schema version. Please check it.
If you want to see optional test, I will make it.

changeset-bot · 2023-10-25T09:24:45Z

🦋 Changeset detected

Latest commit: 65d1844

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@graphql-mesh/grpc	Minor
grpc-example	Patch
grpc-reflection-example	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

* commit 'e0ac7512a': refactor: refactor checking optional for grpc reflection

visma-alexander-maslov · 2023-12-18T12:52:11Z

@Q00 would it be possible to get some insides why did you close this PR ?

Q00 · 2023-12-19T09:03:00Z

@visma-alexander-maslov
I made this to custom module in npm.
I discuss this issue with maintainer of this source. #5590
They want it to keep "protobuf well-formed message". So I decided to make custom npm module and I use it in production level. And I realized that making non-nullable type needs custom convention per each programming language from grpc server.

I made summary in my linkedin. I give you an english version below. m

If you look at the official documentation for protobuf (https://protobuf.dev/), there are a lot of tips and tricks, and one of the best practices is not to make fields require. https://protobuf.dev/programming-guides/dos-donts/#add-required

This also means that a well-formed message should be optional by default.
"A well-formed message can have zero or one of this field"

I think it's in line with this statement that "optional" was present in proto2 and disappeared for a while in proto3.

However, starting with v3.15, the story is different: explicit optional appears in proto3.
See: https://protobuf.dev/programming-guides/field_presence/#how-to-enable-explicit-presence-in-proto3

This new concept changed the way we checked the default field and the field of explicit optional: we checked if a null value exists in the field, which redefined the meaning of null and default value. The problem was that when we communicated with explicit optional and no presence, there was a round trip loss because we couldn't check if it existed as a default value or if it was null.

Therefore, I think that after 3.15, we should check the optional and non-optional fields of protobuf and map them to (explicit optional) graphql nullable if proto3 optional, and non-nullable if not.

For nodejs servers, there is a PR that allows you to check proto3 optional in protobufjs to return null or undefined. protobufjs/protobuf.js#1611

In GraphQL, nullable fields on the input part map well to proto3 optional. This is especially useful in cases like "patch".

On the other hand, in the output part, you need to follow the conventions for each programming language to return the default value. For example, for Node.js, the protobufjs library allows you to return the default value as is. Additionally, you need the proto3 optional to return null. However, if you use the Go language, you need proto3 optional to return the default value.

Q00 added 2 commits October 25, 2023 17:44

feat: make nullable when proto field optional

396536f

test: update snapshot of new version

128dfa9

Q00 mentioned this pull request Oct 25, 2023

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

Open

4 tasks

Q00 and others added 5 commits October 25, 2023 18:30

chore: add changeset

65d1844

chore: update dependency

5b350ea

refactor: refactor checking optional for grpc reflection

e0ac751

Merge commit 'e0ac7512a' into feat/check-field-optional-proto

bba3de9

* commit 'e0ac7512a': refactor: refactor checking optional for grpc reflection

Merge branch 'master' into feat/check-field-optional-proto

3aab755

Q00 closed this Dec 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/check field optional proto #6137

Feat/check field optional proto #6137

Q00 commented Oct 25, 2023

changeset-bot bot commented Oct 25, 2023 •

edited

visma-alexander-maslov commented Dec 18, 2023

Q00 commented Dec 19, 2023 •

edited

Feat/check field optional proto #6137

Feat/check field optional proto #6137

Conversation

Q00 commented Oct 25, 2023

Description

Type of change

Screenshots/Sandbox (if appropriate/relevant):

How Has This Been Tested?

Checklist:

Further comments

changeset-bot bot commented Oct 25, 2023 • edited

🦋 Changeset detected

visma-alexander-maslov commented Dec 18, 2023

Q00 commented Dec 19, 2023 • edited

changeset-bot bot commented Oct 25, 2023 •

edited

Q00 commented Dec 19, 2023 •

edited