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

grpc handler does not work with int64 type record ids #1355

Closed
clarkekawakami opened this issue Dec 29, 2020 · 16 comments
Closed

grpc handler does not work with int64 type record ids #1355

clarkekawakami opened this issue Dec 29, 2020 · 16 comments
Labels
stage/6-released The issue has been solved on a released version of the library

Comments

@clarkekawakami
Copy link

I am working with a grpc back end in which all record ids are typed as int64.

The ids in the request messages (when querying by id) is also a int64 which converts to a BigInt in the graphql-mesh schemas.

All Queries by id fail.

If I change the request message id to int32 in the proto, the query is successful.

@ardatan
Copy link
Owner

ardatan commented Dec 29, 2020

Could you create a minimal reproduction repo?

@clarkekawakami
Copy link
Author

I'd like to set something up but I'm not sure what would be helpful to you.

I suspect that the problem is simply that graphql does not natively support an int64 scalar and when you process the proto files to build the graphql schema, you treat int64 as a custom type.

As I explained, when I changed the original proto file id types in the request messages to int32 (from int64),
the queries worked fine.

Here are some screen shots of what I'm talking about:

Screen Shot 2020-12-31 at 10 44 30 AM

This shows the docs for the "get_account_by_id" query for the modified proto file. This query works. Note the "GetAccountByIdRequestInput" is asking for "accountId: Int" reflecting int32 in the proto file.

Screen Shot 2020-12-31 at 10 49 14 AM

This query works even though "Account" type still reflects "accountId: BigInt"

If I switch back to the original proto file, the query fails.

Screen Shot 2020-12-31 at 11 01 36 AM

Note that the "GetAccountByIdRequestInput" expects "accountId: BigInt".

@ardatan
Copy link
Owner

ardatan commented Feb 24, 2021

Sorry I couldn't reproduce it. Could you share a full reproduction in a repo or CodeSandbox? Thanks!
I'm closing this for now. I'll reopen it when you share the reproduction.

@ardatan ardatan closed this as completed Feb 24, 2021
@cometkim
Copy link
Contributor

cometkim commented Mar 16, 2021

@clarkekawakami @ardatan I have exactly same problem with this. and solve this by patching graphql-scalars's BigInt

function coerceBigIntValue(value) {
   if (isBigIntAvailable()) {
       patchBigInt();
-      return BigInt(value);
+      return BigInt(value).toString();
   }
   else {
       return Number(value);
   }
}

cc @Urigo

@Urigo
Copy link
Collaborator

Urigo commented Mar 20, 2021

Thanks @cometkim !
I'm reopening, any chance of helping us creating a failing test?
(maybe both on GraphQL Scalars and GraphQL Mesh?)

@Urigo Urigo reopened this Mar 20, 2021
@cometkim
Copy link
Contributor

Oh... I'm not sure at which layer this is causing the problem.

The int64 field in the input was completely missing in the actual network request, so I assumed the payload was wrong and fixed it, but the result of BigInt .toString() is fine actually.

I patched the parseLiteral's result to explicitly call toString() to return a string rather than an object, and everything works fine.

@cometkim
Copy link
Contributor

cometkim commented Mar 30, 2021

I cannot share the full my environment, but here's some brief

I have grpc service I wanna integrate, and the proto file look like this

service RegionService {
    // Output a region information based on given region id
    rpc GetRegionByID (GetRegionByIDReq) returns (GetRegionByIDRes);
} 

message Region {
    int64 id = 1; // represent region_id
    // ...
}

message GetRegionByIDReq {
    string code = 1; // country code
    int64 region_id = 2; // region_id is the id of the region information that the client wants
}

message GetRegionByIDRes {
    Region region = 1; // region entity
}

And I tried to use GraphQL api generated by graphql-mesh with this query

{
  GetRegionByID(input: {
    code: "KR",
    regionId: 1,
  }) {
    region {
      id
    }
  }
}

The query runs successfully, but the request failed on the server side and I returned an error like:

{
  "errors": [
    {
      "locations": [],
      "message": "5 NOT_FOUND: warning: db: region not found",
      "name": "GraphQLError",
      "nodes": [ // AST of the query definition

That because, the regionId (the BigInt field) is actually not passed to the network payload.

request-dump

I patched the graphql-scalars code like:

image

And successfully resolves this issue

image

I've filed a PR to graphql-scalars Urigo/graphql-scalars#795

This certainly solves my problem, but It's a breaking change to upstream and I couldn't make a minimal reproduction.

@ardatan
Copy link
Owner

ardatan commented Mar 30, 2021

@cometkim You're right it is a breaking change. First we need to see a minimal reproduction as a repo or CodeSandbox.

@cometkim
Copy link
Contributor

cometkim commented Mar 30, 2021

I tried to trace BigInt.prototype.toJSON and BigInt.prototype.toString but both don't run while graphql executing and coerceBigIntValue is called.

@ardatan
Copy link
Owner

ardatan commented Mar 31, 2021

@cometkim Any chance for a minimal reproduction as a repo or CodeSandbox?

@cometkim
Copy link
Contributor

cometkim commented Apr 6, 2021

@ardatan @Urigo I suspected protobufjs so I planted the tracing log there.

see here:

so far, If an int64 value is passed as an object, protobufjs expects it to be encoded in the format { low: number, high: number, unsigned: boolean }.

If the format does not match, the actual input is converted to a zero object no matter what value it is. And it seems to be treated as an empty value in my environment.

image

This can be verified by adding the log in grpc-examples as well.

console.log(call.request.movie.year);
// -> Long { low: 0, high: 0, unsigned: false }

So, we already have the minimal reproduction

@ardatan
Copy link
Owner

ardatan commented Apr 20, 2021

I created a PR on protobuf.js. Now I am investigating a workaround for gRPC handler.

@ardatan
Copy link
Owner

ardatan commented Apr 20, 2021

@cometkim Could you try with this version?
@graphql-mesh/grpc@0.12.2-alpha-ec9a81fd8.0

@Urigo Urigo added the stage/5-alpha-release-testing The pull request is merged, an alpha release is available, to be tested label Apr 20, 2021
@ardatan
Copy link
Owner

ardatan commented Apr 22, 2021

The recent changes are now available in the latest version. Let us know if it still doesn't work so we can reopen the issue.

@ardatan ardatan closed this as completed Apr 22, 2021
@ardatan ardatan added stage/6-released The issue has been solved on a released version of the library and removed stage/5-alpha-release-testing The pull request is merged, an alpha release is available, to be tested labels Apr 22, 2021
@cometkim
Copy link
Contributor

I couldn't test it because I was stuck with other issues after upgrade.

#1884

@casuallyhostile
Copy link

I ran into the same issue (sending an int64 argument to a grpc service, it ended up as 0) when running graphql-mesh via createBuiltMeshHTTPHandler on a nextjs route. Solved it by monkeypatching the long package

// int64 issue TLDR: GraphQL treats int64-proto type as BigInt native js type
// When graphql-mesh grpc handler sends the value to the grpc service it sends it as BigInt
// BigInt not supported by underlying stack (protobuf.js / long lib)
//     __
//     w  c(..)o   (
//      \__(-)    __)
//          /\   (
//         /(_)___)
//         w /|
//          | \
//         m  m
//
// Monkeypatching long lib inside protobuf.js to correctly convert BigInt to Long
// Stack of unresolved issues with int64/bigint:
// 1. "Graphql-mesh: grpc handler does not work with int64 type record ids"
//    https://github.com/ardatan/graphql-mesh/issues/1355
// 2. "Protobuf.js: Support bigint as input"
//    https://github.com/protobufjs/protobuf.js/pull/1592
// 3. "Protobuf.js: Use BigInt over Number for int64"
//    https://github.com/protobufjs/protobuf.js/pull/1557
// 4. "Long: Support BigInt"
//    https://github.com/dcodeIO/long.js/issues/82
const Long = require('long');

const originalFromValueFn = Long.fromValue;
Long.fromValue = (val: any, unsigned: boolean) => {
  // eslint-disable-next-line no-param-reassign
  if (typeof val === 'bigint') { val = val.toString(); }
  return originalFromValueFn(val, unsigned);
};

// Monkeypatching BigInt to support serialization
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt#use_within_json
// eslint-disable-next-line func-names
(BigInt.prototype as any).toJSON = function () {
  return this.toString();
};

export default createBuiltMeshHTTPHandler();

Any feedback appreciated! I'm pretty sure there should be more elegant way to solve this.

Didn't reproduce it when running an grpc-handler example with mesh serve - guessing that's because of some extra magic like await import('json-bigint-patch') in
https://github.com/ardatan/graphql-mesh/blob/master/packages/cli/src/commands/serve/serve.ts#L131

PS: json-bigint-patch didn't work for me in my next.js setup though.

klippx pushed a commit to klippx/graphql-mesh that referenced this issue Oct 9, 2024
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/6-released The issue has been solved on a released version of the library
Projects
None yet
Development

No branches or pull requests

5 participants