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

Missing Object Prototype #484

Closed
DxCx opened this issue Sep 9, 2016 · 8 comments
Closed

Missing Object Prototype #484

DxCx opened this issue Sep 9, 2016 · 8 comments

Comments

@DxCx
Copy link

DxCx commented Sep 9, 2016

Hi,

GraphQL response object is missing Javascript's prototype property,
This is causing errors like:
Uncaught TypeError: Cannot convert object to primitive value

for example (res.data.user):
example

Here is a small test shows it

it('GraphQL inherits', () => {
      const Thread = new GraphQLObjectType({
          name: 'Thread',
          fields: {
              id: {
                  type: GraphQLString,
                  resolve(thread, args, ctx) {
                      return thread.id;
                  },
              },
              name: {
                  type: GraphQLString,
                  resolve() {
                      return 'Lorem Lipsum';
                  },
              },
          },
      });
      const Query = new GraphQLObjectType({
          name: 'Query',
          fields: {
              thread: {
                  type: Thread,
                  args: {
                      id: {
                          type: GraphQLString,
                      },
                  },
                  resolve(root, args) {
                      return {id: args.id};
                  }
              },
          },
      });
      const jsSchema = new GraphQLSchema({
          query: Query,
      });
      const testQuery = `query abc{
          thread(id: "67"){
              id
                  name
          }
      }`;
      const expected = {
          thread: {
              id: '67',
              name: 'Lorem Ipsum',
          },
      };
      return graphql(jsSchema, testQuery).then((res) => {
          expect(Object.getPrototypeOf(res.data.thread)).to.deep.equal(
                  Object.getPrototypeOf(expected)
                  );
      });
  });

this is continuation of ardatan/graphql-tools#127

DxCx added a commit to DxCx/graphql-js that referenced this issue Sep 24, 2016
@leebyron
Copy link
Contributor

leebyron commented Sep 29, 2016

Can you explain more about what code is producing the error: "Uncaught TypeError: Cannot convert object to primitive value"?

That on its own seems suspect. Object's prototype method for converting to a primitive returns [object Object] which is probably not what you're after in the first place.

The missing prototype isn't a bug, it's an intentional tradeoff. Imagine what would be produced by the query:

query {
  toString: me { name }
  valueOf: someOtherRealField
}

Those aliases would cause those properties to be set on the resulting object. If the resulting object was expected to have ObjectPrototype with toString and valueOf prototype methods, then setting those properties would render them inaccessible - if you rely on them then your code would break. A clever malicious person might try to do this to break your server.

@DxCx
Copy link
Author

DxCx commented Sep 29, 2016

yeah @leebyron, i waited to hear your thoughts about this before going and fixing it,
went to fix it only after more then 2 weeks passed with no answer..
anyhow, for that to happen, the server itself needs to define "toString" and "valueOf" as valid properties,
is it?

the error itself were coming as an issue report to graphql-tools,
i'm helping there as well.. :)

@patotoma can you please elaborate on this?

DxCx added a commit to DxCx/graphql-js that referenced this issue Sep 29, 2016
@leebyron
Copy link
Contributor

Sorry for the delayed response. I'm glad you jumped in.

The server doesn't need to define them as fields, a query can just use aliases to create that scenario

@DxCx
Copy link
Author

DxCx commented Sep 29, 2016

Aliases? Cool! Do you have an example for alias? Didnt know this feature..
Anyhow, lets wait for the original author to respond, i gave him the a workaround to use json convert/parse to rebuild the properties but i agree with you regarding being dangerous now that i understand why it was chosen to be done this way..

@leebyron
Copy link
Contributor

You can read more about them here: http://graphql.org/learn/queries/#aliases

@DxCx
Copy link
Author

DxCx commented Sep 30, 2016

Thanks!

@DxCx
Copy link
Author

DxCx commented Nov 10, 2016

another Month without patotoma response.. so im gonna close it. Thanks lee!

@DxCx DxCx closed this as completed Nov 10, 2016
@shtanton
Copy link

shtanton commented Jul 2, 2018

Is it possible to have obj instanceof Object === true but still have obj.toString === undefined etc.

Some libraries use instanceof to check the type of arguments and this prevents them from functioning correctly.

If a query had an object with a type 'toString' surely the toString method is simply overwritten and so there is no problem, this seems like a better solution though I haven't thought about it that much.

PiDelport added a commit to registreerocks/registree-core that referenced this issue Feb 3, 2021
This primarily intended to work around the problem of graphql-js using
null prototype objects to represent maps.

See: graphql/graphql-js#484 and related

(This code is a bit ridiculous in implementation, but it lets callers
deal with a simpler interface.)
PiDelport added a commit to registreerocks/registree-core that referenced this issue Feb 3, 2021
This primarily intended to work around the problem of graphql-js using
null prototype objects to represent maps.

See: graphql/graphql-js#484 and related

(This code is a bit ridiculous in implementation, but it lets callers
deal with a simpler interface.)
PiDelport added a commit to registreerocks/registree-core that referenced this issue Feb 4, 2021
This primarily intended to work around the problem of graphql-js using
null prototype objects to represent maps.

See: graphql/graphql-js#484 and related

(This code is a bit ridiculous in implementation, but it lets callers
deal with a simpler interface.)
PiDelport added a commit to registreerocks/registree-core that referenced this issue Feb 4, 2021
This primarily intended to work around the problem of graphql-js using
null prototype objects to represent maps.

See: graphql/graphql-js#484 and related

(This code is a bit ridiculous in implementation, but it lets callers
deal with a simpler interface.)
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.

3 participants