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

Cannot read property 'sqlTable' of undefined #352

Closed
zhorzhz opened this issue Sep 21, 2018 · 20 comments
Closed

Cannot read property 'sqlTable' of undefined #352

zhorzhz opened this issue Sep 21, 2018 · 20 comments
Assignees
Labels

Comments

@zhorzhz
Copy link

zhorzhz commented Sep 21, 2018

Join-monster fails with

(node:3555) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 11): TypeError: Cannot read property 'sqlTable' of undefined

OR

{
	"errors": [
		{
			"message": "Cannot read property 'sqlTable' of undefined",
			"locations": [
				{
					"line": 2,
					"column": 2
				}
			],
			"path": [
				"orders"
			]
		}
	],
	"data": {
		"orders": null
	}
}

Here is my code

const Order =  new graphql.GraphQLObjectType({
    name    : "Order",
    sqlTable: `order`,
    uniqueKey: "id",
    fields  : () => ({
        order_id    : {
            type: graphql.GraphQLString,
        },
    }),
});

module.exports = new graphql.GraphQLSchema({
    query : new graphql.GraphQLObjectType({
        name    : "Query",
        fields  : () => ({
            orders: {
                type    : Order,
                resolve :(obj, args, context, resolveInfo) => {
                    return join_monster(resolveInfo, context, sql => {
                        return pg.query(sql);
                    });
                },
            },
        }),
    }),
});

@zhorzhz zhorzhz changed the title can not find sqlTable Cannot read property 'sqlTable' of undefined Sep 21, 2018
@naturalethic
Copy link

As mentioned here:

#351

This is due incompatibility with graphql 14.

Just install the older version:

yarn add graphql@0.13.2

@zhorzhz
Copy link
Author

zhorzhz commented Sep 24, 2018

Thanks, @naturalethic. Is there any chance that new versions of join-monster will be compatible with the latest graphql versions.

@zhorzhz zhorzhz closed this as completed Sep 24, 2018
@naturalethic
Copy link

@zhorzhz Can you please re-open this.

I looked into the graphql code for the new version and they no longer expose type._typeConfig after the type is constructed. Here's a temporary work-around until the join-monster developers can deal with this in a better way.

Originally your code would have looked something like this:

const User = new gql.GraphQLObjectType({
  name: 'User',
  sqlTable: 'user',
  uniqueKey: 'id'
  fields: () => ({
    id: {
      type: gql.GraphQLInt
    }
  })
})

The fix is to now only define the graphql specific properties in the type constructor, then provide the join-monster properties afterward like so:

const User = new gql.GraphQLObjectType({
  name: 'User',
  fields: () => ({
    id: {
      type: gql.GraphQLInt
    }
  })
})

User._typeConfig = {
  sqlTable: 'user',
  uniqueKey: 'id'
}

@zhorzhz
Copy link
Author

zhorzhz commented Sep 24, 2018

@naturalethic Thanks a lot. I will try it. Seems it can solve our issue, but of course it would be great if join-monster developers will find a permanent solution to this problem.

@zhorzhz zhorzhz reopened this Sep 24, 2018
@maksis
Copy link

maksis commented Sep 24, 2018

This feature might be useful also for join-monster: graphql/graphql-js#1527

@naturalethic
Copy link

That seems appropriate.

@I3uckwheat
Copy link

I can also confirm this issue. @naturalethic 's work-around resolved this for me.

@switz
Copy link

switz commented Sep 25, 2018

This is quality info, thanks for the leg work and concise summary @naturalethic

@GlennMatthys
Copy link
Collaborator

GlennMatthys commented Nov 2, 2018

Reading the issue over at graphql-js I see no other way forward than proposed: adding an extensions property to each GraphQL object. Although it will incur API breakage when implemented, it would be safer for graphql-js that foreign properties can be added in a controlled way. So basically the only thing we can do, aside from @naturalethic's workaround, is to bug the graphql-js team to implement the extensions property.

@wtho
Copy link
Contributor

wtho commented Nov 14, 2018

Here's a function which encapsulates the fix proposed by @naturalethic, so less changes have to be made:

function newJoinMonsterGraphQLObjectType(objectTypeConfig) {
  const joinMonsterConfig = {}
  const joinMonsterKeys = ['sqlTable', 'uniqueKey', 'alwaysFetch']
  joinMonsterKeys.forEach(key => {
    if (key in objectTypeConfig) {
      joinMonsterConfig[key] = objectTypeConfig[key]
    }
  })

  const GraphQLObject = new GraphQLObjectType(objectTypeConfig)
  GraphQLObject._typeConfig = joinMonsterConfig

  return GraphQLObject
}

You can use it like this:

Before (graphql version < 14)

const User = new gql.GraphQLObjectType({
  name: 'User',
  sqlTable: 'users',
  uniqueKey: 'id',
  fields: () => ({
    ...
  })
})

After (graphql v14)

const User = newJoinMonsterGraphQLObjectType({
  name: 'User',
  sqlTable: 'users',
  uniqueKey: 'id',
  fields: () => ({
    ...
  })
})

Check out the Typescript version if you prefer that.

I also had to patch objects not directly tied to a sql table, as join monster would check them anyway and run in a ReadOfUndefined-Error, assuming the gqlType._typeConfig is always defined.

@flippidippi
Copy link

To what @wtho said, if you want to get this working without any changes with graphql-tools, I submitted a work around here join-monster/join-monster-graphql-tools-adapter#15.

@wtho
Copy link
Contributor

wtho commented Nov 14, 2018

@flipxfx Awesome!
Unfortunately I do not use Apollo-Server

@s97712
Copy link

s97712 commented Nov 23, 2018

// TableType                                                                                                
interface TableConfig {                                                                                     
  sqlTable: string,                                                                                         
  uniqueKey: string,                                                                                        
  // ...other                                                                                               
}                                                                                                           
export type GraphQLTableTypeConfig<TSource, TContext> = GraphQLObjectTypeConfig<TSource, TContext> & TableCo
nfig;                                                                                                       
export class GraphQLTableType extends GraphQLObjectType {                                                   
  _typeConfig: TableConfig;                                                                                 
                                                                                                            
  constructor(config: GraphQLTableTypeConfig<any, any>){                                                    
    super(config);                                                                                          
    this._typeConfig = {                                                                                    
      sqlTable: config.sqlTable,                                                                            
      uniqueKey: config.uniqueKey                                                                           
    }
  }
}

// usage
const User = new GraphQLTableType({
  name: 'User',
  sqlTable: 'user',
  uniqueKey: 'id'
  fields: () => ({
    id: {
      type: gql.GraphQLInt
    }
  })
});

May be we can through inherit GraphQLObjectType to solve , but this repo has too much reference of constructor.name, I can't let it work

@wtho
Copy link
Contributor

wtho commented Nov 29, 2018

@s97712 yeah, that is why I used the original object.

Another way how your solution should work would be to name the class the same (untested):

import { GraphQLObjectType as OriginalGraphQLObjectType } from 'graphql';
export class GraphQLObjectType extends OriginalGraphQLObjectType {
  // ...
}

I personally prefer duck typing over constructor name comparison, but it is clearly more efficient to just check the constructor name.
Maybe the maintainers are open for a PR to change this (for more extensibility). Let us know in case you read this :bowtie:

@GlennMatthys
Copy link
Collaborator

Doesn't seem like a bad plan, but personally I prefer waiting what the upstream graphql-js answer is to this problem. If we change our API now, and then upstream changes its mind, it will be work for nothing.

@wtho
Copy link
Contributor

wtho commented Nov 30, 2018

Yeah, totally agree! Let's wait and react once the graphql-js API is updated.

@flippidippi
Copy link

@GlennMatthys GraphQL added the ability to pass extra metadata via extensions!

graphql/graphql-js#2097

@airhorns
Copy link
Collaborator

Is anyone working on a PR to upgrade join-monster to use the new extensions property and get us all the new GraphQL features? I am not super familiar with the codebase but I can take a crack at it if no one else has started! I think it would have to result in a major version bump too because almost the whole public facing API of join-monster would have to change right?

@alexbbt
Copy link
Contributor

alexbbt commented May 29, 2020

@airhorns a POC PR would certainly be welcome. No one on the core team has started work on this yet.

@lorensr
Copy link
Member

lorensr commented Aug 11, 2020

We just released support for graphql@15 in 3.0.0-alpha.1. Let us know if it works for you! Note the new object format:

https://join-monster.readthedocs.io/en/latest/map-to-table/

const User = new GraphQLObjectType({
  name: 'User',
  extensions: {
    joinMonster: {
      sqlTable: 'accounts', // the SQL table for this object type is called "accounts"
      uniqueKey: 'id' // id is different for every row
    }
  },
  fields: () => ({
    /*...*/
  })
})

@lorensr lorensr self-assigned this Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests