Null fields in mongo return sub fields

[yoadsn]

Hey,
I am not sure where this behavior is coming from.. have been debugging both graphql-compose* and graphql-js for the past 5 hours. Any help in the right direction would be great!

Assume this mongodb document:

{
 _id: objectId("5816f273f3bb751f5021d849"),
 someField: null
}

And this mongoose schema:

let inner = {
 otherField: String
}

let MyType = Schema({
  someField: inner
})

The following gql query (assume the getOne field returns the above document):

query {
  getOne(_id: "5816f273f3bb751f5021d849") {
    someField {
      otherField
    }
  }
}

I expect to get:

{
   "data" : {
    "getOne" : {
      someField: null
    }
  }
}

This is for example what graffiti-mongoose returns..
But from graphql-compose* I get:

{
   "data" : {
    "getOne" : {
      someField: {
        otherField: null
      }
    }
  }
}

Am I expecting the wrong behavior? I found this issue graphql/graphql-js#424 that support what I expect (I think). Where is the difference?
So far I reached the conclusion that graphql-js does not recognize the value for "someField" to be null here:
https://github.com/graphql/graphql-js/blob/73513d35747cdea255156edbe30d85d9bd0c1b81/src/execution/execute.js#L774
I'm using graphql 0.7.2 although this specific code seems to remain the same in 0.8*.

Thanks!

[yoadsn]

Some more info.

Assume in the above code in graphql-js I add some hackish deubgging code like this:

  console.log('-----------')
  console.log(path)
  console.log(result)
  console.log(isNullish(result));
  console.log('***********')

  // If result value is null-ish (null, undefined, or NaN) then return null.
  if (isNullish(result)) {
    return null;
  }

Then with graffiti-mongoose the output looks like this:

-----------
[ 'getOne' ]
{ _id: 5816f273f3bb751f5021d849, someField: null, _type: 'MyType' }
false
***********
-----------
[ 'getOne', 'someField' ]
null
true

And with graphql-compose* it's:

-----------                          
[ 'getOne' ]
{ _id: 5816f273f3bb751f5021d849, someField: null }
false                                
***********                          
-----------                          
[ 'getOne', 'someField' ]
null <--- this is the strange part! it prints "null" but actually it's a mongoose object!
false                                
***********                          
-----------                          
[ 'getOne', 'someField', 'otherField' ]
undefined
true                                 
***********                          

So again, for some reason the isNullish method cannot recognize the result as "null" since it's actually an empty object.

[nodkz]

You expect correct behavior. It seems that graphql-compose-mongose for nested schemas does not has correct resolve method. I'll try to catch this tomorrow.

Right now just add such thing to MyTC.

MyTC.extendField('someField', {
resolve: (source) => source.someField ? source.someField : null,
});

Will be nice if you provide result of console.log(source).

[nodkz]

Suppose that source.someField will be {}. And this is problem somewhere between mongoose and graphql-compose-mongoose.

[yoadsn]

I'm still debugging this - the above test case cannot be trusted anymore since some previous debug lines changed stuff. Will post new debugging data ASAP.
Thank you for the directions.

[yoadsn]

Ok, now the printout is correct. But it would not provide the full story since I need to figure out why "null" is printed when doing toString() of a mongoose object (which is not null).

[nodkz]

Point in code where we should add resolve method with null check to fields with sub schema type: https://github.com/nodkz/graphql-compose-mongoose/blob/master/src/fieldsConverter.js#L152

[yoadsn]

Ok, @nodkz I believe I have now more information about what is going on. (Some is obvious to you probably, but still).

When we execute a mongoose query - we get back a document and not a normal javascript object. The mongoose document we get for this mongodb document:

{
 _id: objectId("5816f273f3bb751f5021d849"),
 someField: null
}

Would have a non-null value on the someField property, it's actually the document itself with a changed path. (confusingly, doing a toString() on it would print "null" - which is also not an actual null)

So when the graphql-js default field resolver gets that mongoose document, and checks isNullish on the field it gets false, which leads it to dive deeper continuing to resolve sub fields.

Changing the code at:
https://github.com/nodkz/graphql-compose-mongoose/blob/1bf65f7ba0c97ac189ac21fca4e3cf9fa31a8c18/src/fieldsConverter.js#L152

To something like this:

graphqlFields[fieldName] = {
  type: convertFieldToGraphQL(mongooseField, typeName),
  description: _getFieldDescription(mongooseField),
  resolve: (src) => {
    if (fieldName === 'someField') {
      src.someField === null ? 'NULL' : 'NOT NULL';
    }
    return 'doesn't matter';
  }
};

confirms this and prints 'NOT NULL'.

Your suggestion to check for null here is possible, we need to use the (undocumented?) getValue(path) method of the mongoose document to actually get the value we want. Like this:

graphqlFields[fieldName] = {
  type: convertFieldToGraphQL(mongooseField, typeName),
  description: _getFieldDescription(mongooseField),
  resolve: (src) => {
    return src.getValue(fieldName);
  }
};

effectively overriding the default field resolver for mongoose documents.

But we have also another alternative which is also documented in mongoose API ref and used in graffiti-mongoose. The are converting the mongoose document to a javascript plain object immediately after it returns from the query execution. See here:
https://github.com/RisingStack/graffiti-mongoose/blob/1d1f6d1d3d18c52da969ef9694c8b6a619679170/src/query/query.js#L36
This is also where they append the _type meta field.

For us, we might go with the same approach by changing the different resolvers generated by graphql-compose-mongoose - for example change here:
https://github.com/nodkz/graphql-compose-mongoose/blob/1bf65f7ba0c97ac189ac21fca4e3cf9fa31a8c18/src/resolvers/findById.js#L45
To something like this:

return resolveParams.query.exec().then(result => result.toObject());

We can also centrally wrap those resolvers here:
https://github.com/nodkz/graphql-compose-mongoose/blob/1bf65f7ba0c97ac189ac21fca4e3cf9fa31a8c18/src/composeWithMongoose.js#L121

const resolver = createResolverFn(
  model,
  typeComposer,
  opts[resolverName] || {}
).wrapResolve(
    next => resolveParams =>
        next(resolveParams).then(result => {
            if (Array.isArray(result) {
                return result.map(doc => doc.toObject());
            } else {
                return result.toObject();
            }
        })
)

Of course this code is not robust but the gist should be clear..

What do you think?

[yoadsn]

PS this broke my "relations" since I relied on the virtual getter id to exist. So maybe this would break other code which relies on virtual getters (oh my).
I still think that in terms of layered design, handing over a plain object is better than passing around a mongoose model - since higher level plugins/mw might depend on mongoose doc properties which is a bad thing. Ideally, you should be able to replace mongoose based GQL types with ES based GQL types and all should continue to work properly..

[nodkz]

Awesome analysis.
Yep, I know about that resolvers return mongoose documents. And this is desired behavior - I definitely want to allow to developers have access to the model's methods in wrap resolver. It's better than to convert completely document to plain object and loose an ability to call model methods.

But I didn't know that it produce null problem for nested fields. So I try to fix this. Stay tuned.

[yoadsn]

Thank you! too bad spent the last hour writing wrappers around all resolvers ;)
Do you think it makes sense to use {virtuals : true} on the toObject() call of the sub document?
I only know of the id virtual which is relevant to top level document but maybe if the developer added some on the subdoc we need to support it?

[nodkz]

I think that fix will be fast and easy. But got tons of problems with nested docs in nested doc. So only one correct way was to call toObject cause it makes a nested conversion.

I think that {virtuals : true} should be defined in options for nested schema.

NestedSchema = mongoose.Schema(
  {
   ...someFields
  }, {
    toObject: { virtuals: true },
  }
);

I think this is more control behavior.