Urigo / SOFA

The best way to create REST APIs - Generate RESTful APIs from your GraphQL Server
https://www.the-guild.dev/graphql/sofa-api
MIT License
1.07k stars 87 forks source link

Maximum call stack size exceeded #21

Closed lnmunhoz closed 5 years ago

lnmunhoz commented 5 years ago

Hey Urigo,

First of all, thanks for this library. the concept is really cool, but... I've just follow the simple configuration and tried to run, got this error:

RangeError: Maximum call stack size exceeded

This is the schema I am passing. It works fine with apollo-server:

import { makeExecutableSchema } from "apollo-server"
import typeDefs from "./typeDefs"
import resolvers from "./resolvers"
import {
  isAuthDirective,
  hasScopeDirective,
  isAdminDirective,
} from "./directives"

export default makeExecutableSchema({
  typeDefs,
  resolvers,
  schemaDirectives: {
    isAuth: isAuthDirective,
    hasScope: hasScopeDirective,
    isAdmin: isAdminDirective,
  }
})

And the full stack trace:

RangeError: Maximum call stack size exceeded
    at resolveField (/foo/node_modules/sofa-api/src/operation.ts:278:22)
    at /foo/node_modules/sofa-api/src/operation.ts:218:16
    at Array.map (<anonymous>)
    at resolveSelectionSet (/foo/node_modules/sofa-api/src/operation.ts:217:39)
    at resolveField (/foo/node_modules/sofa-api/src/operation.ts:328:21)
    at /foo/node_modules/sofa-api/src/operation.ts:218:16
    at Array.map (<anonymous>)
    at resolveSelectionSet (/foo/node_modules/sofa-api/src/operation.ts:217:39)
    at resolveField (/foo/node_modules/sofa-api/src/operation.ts:328:21)
    at /foo/node_modules/sofa-api/src/operation.ts:218:16
    at Array.map (<anonymous>)
    at resolveSelectionSet (/foo/node_modules/sofa-api/src/operation.ts:217:39)
    at resolveField (/foo/node_modules/sofa-api/src/operation.ts:328:21)
    at /foo/node_modules/sofa-api/src/operation.ts:218:16
    at Array.map (<anonymous>)
    at resolveSelectionSet (/foo/node_modules/sofa-api/src/operation.ts:217:39)

Let me know what else I can provide to help you. Thank you!

Urigo commented 5 years ago

Can you try to create a reproduction?

birkir commented 5 years ago

I have the same thing, only when using OpenAPI.

openApi.addRoute(info, {
  basePath: '/api',
});

FYI: I have endless recursion in my schema, so something like this (totally valid of course)

type A {
  b: B
}

type B {
  a: A
}

EDIT oh wait, I provided different schema into openApi than sofa, that may be your case?

lnmunhoz commented 5 years ago

@birkir I do have recursion in my schema. But no, I've passed the same. @Urigo I will to create a reproduction and update here soon 👍

birkir commented 5 years ago

Here is my stack trace. It is definitely related to recursion of types.

My stacktrace:

(node:9251) UnhandledPromiseRejectionWarning: RangeError: Maximum call stack size exceeded
    at String.replace (<anonymous>)
    at module.exports (/Volumes/Data/Projects/prime/node_modules/no-case/no-case.js:32:6)
    at Object.module.exports (/Volumes/Data/Projects/prime/node_modules/camel-case/camel-case.js:12:16)
    at getArgumentName (/Volumes/Data/Projects/prime/node_modules/sofa-api/src/operation.ts:275:21)
    at /Volumes/Data/Projects/prime/node_modules/sofa-api/src/operation.ts:300:32
    at Array.map (<anonymous>)
    at resolveField (/Volumes/Data/Projects/prime/node_modules/sofa-api/src/operation.ts:297:23)
    at /Volumes/Data/Projects/prime/node_modules/sofa-api/src/operation.ts:218:16
    at Array.map (<anonymous>)
    at resolveSelectionSet (/Volumes/Data/Projects/prime/node_modules/sofa-api/src/operation.ts:217:39)
verdverm commented 5 years ago

Got a REPRO CASE! It does seem to be related to: (currently editing more)

type A {
  b: B
}

type B {
  a: A
}

Steps to reproduce: (sorry for the very non-minimal example)

1: get the working branch

git clone https://github.com/verdverm/apollo-universal-starter-kit ausk
cd ausk
git co -b feature-sofa-rest-api

(this makes use of reproduction case uses https://github.com/Urigo/SOFA/pull/22)

  1. verify that the code works as is
    yarn
    yarn seed
    yarn watch
  2. cause the infinite error to occur

In https://github.com/verdverm/apollo-universal-starter-kit/blob/master/modules/post/server-ts/schema.graphql the very first lines should look like

type Post {
  id: Int!
  title: String!
  content: String!
  comments: [Comment]
  user: User
}

extend type User {
  posts: [Post]
}

I figured out a way to catch the issue too, it's a GraphQLNamedType cycle

here, put (https://github.com/Urigo/SOFA/blob/master/src/operation.ts#L142)

const visitedTypes: Record<string,bool> = {};

here, put (https://github.com/Urigo/SOFA/blob/master/src/operation.ts#L212)

let found = visitedTypes[type.name];
if (found) {
  return undefined;
}
visitedTypes[type.name] = true;

Results in

Error: Field "currentUser" of type "User" must have a selection of subfields. Did you mean "currentUser { ... }"?

GraphQL request (2:3)
1: query currentUserQuery {
2:   currentUser
     ^
3: }

    at /home/tony/gocode/src/github.com/hofstadter-io/ausk-sofa/dist/express.js:265:1
    at step (/home/tony/gocode/src/github.com/hofstadter-io/ausk-sofa/dist/express.js:43:1)
    at Object.next (/home/tony/gocode/src/github.com/hofstadter-io/ausk-sofa/dist/express.js:24:1)
    at fulfilled (/home/tony/gocode/src/github.com/hofstadter-io/ausk-sofa/dist/express.js:15:1)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)

Maybe returning something besides undefined would be proper. It seems to be missing the field selection for the types. Maybe we could just artificially limit recursion? (clearing the visited within a function somewhere further up the call chain?)

I'm just trying to get to my beta version for now :]

verdverm commented 5 years ago

getting closer, managed the recursion by memoizing types that had been seen, not recursing into another types field (of the previously seen typ) in the stack, and then unmemoizing as recursion unwinds. This means we can avoid recursing into a type twice. so something like this...

query {
  a {
    b {
      a {     STOP HERE, don't recursively enter this field (actually stop in the previous b, when the a field is encountered
        b {
         ...
        }
      }
    }
 }
}

In actuality, this stopping point could be limited to some maximum depth.


Now having a problem, but I think this is more in my resolvers and scheme, and not this library.

Will push some code up tomorrow

verdverm commented 5 years ago

You can see the changes here: https://github.com/hofstadter-io/SOFA/pull/1

You can try them out by using "@hofstadter-io/sofa-api" : "0.3.0" in your projects

(note, the winston logger was removed because it was having issues with the format being undefined on import...)

kamilkisiela commented 5 years ago

Okay, should be fixed by #23 - I published it as 0.2.3-alpha.0, could you guys try it out?

birkir commented 5 years ago

Works like a charm @kamilkisiela, thanks for the update!

I am shipping it with prime this week!

Urigo commented 5 years ago

thank everyone for such detailed bug report!

@birkir that is awesome!

verdverm commented 5 years ago

@Urigo found another infinite recursion for many-to-many relations (and maybe some others):

edge case:

type A {
  bs: [B]
}

type B {
  as: [A]
}

These 5 lines should resolve the issue:

https://github.com/hofstadter-io/SOFA/pull/2/files#diff-2317c71e030318850aad89d8379c14cbR220

basically need to trim the array portion [ and ] from the type, although I don't know if this applies to your ancestor method

kamilkisiela commented 5 years ago

@verdverm @Urigo The issue here is that if a type has only one field then we get:

{
  b {
    a {
      b {
        a {...}
      }
    }
  }
}

If we want to omit the first circular reference then we get:

{
  b {
    a {
      // empty :(
    }
  }
}

Since we can't execute such a query because of this empty block and all types has only one field then we should remove the first b and a too. Which leave us with an empty document.

I knew the limitation when I was fixing circular references but I assumed it's a very rare use-case when people has two types that uses themself through a single field.

Right now, I have no idea what could be the solution.

@verdverm Your solution turns an array into a single entity which is incorrect but maybe I don't see something, could you correct me?

verdverm commented 5 years ago

@kamilkisiela can you try my fork https://github.com/hofstadter-io/SOFA https://www.npmjs.com/package/@hofstadter-io/sofa-api

"@hofstadter-io/sofa-api": "^0.3.1"

the code is different, and has more/different fixes that were discovered after the initial changes.

WRT your last question, I think the changes here are missing the case to deal with sarrays, it's not actually changing the array into a single entity, but detecting that arrays and singles are the same type during memoization.

kamilkisiela commented 5 years ago

@verdverm our logic does the same, getNamedType() resolves with a named type, so it doesn't matter if it's list or a nullable node.

I also added depthLimit option (set to 1 by default) which enables to "cut off" the circular reference at n level deep.

I checked your code and the only difference is that you're passing res object into the context, which is something that could be done here too. Maybe you could create a Pull Request for that?

kamilkisiela commented 5 years ago

Actually, we pass res too

akshay-nm commented 5 years ago

Hi, I want to use SOFA to generate a rest API. What kind of things should I avoid when putting together a schema inorder to circumvent this issue?