graphql-compose-mongoose icon indicating copy to clipboard operation
graphql-compose-mongoose copied to clipboard

[update] update functions should only update fields

Open st0ffern opened this issue 8 years ago • 7 comments

Right now the update function updates the entire document supplied the mutation data. I think that the update should only update the fields sendt in the mutation.

st0ffern avatar Mar 31 '17 07:03 st0ffern

How I know it updates only that fields which was passed via mutation. If you pass fieldname with null, it must save null to DB. If field not present in mutation it should keep it in DB unchanged.

@rlogachyov can you confirm this?

nodkz avatar Mar 31 '17 08:03 nodkz

I have this function on Users scema:

UserSchema.pre('save', function (next) {
  if (this.password || this.isNew){
    this.password = bcryptjs.hashSync(this.password, 10)
  }
  next()
})

https://github.com/nodkz/graphql-compose-mongoose/blob/master/src/resolvers/updateById.js#L109-L116

Will get full document, update fields that is changed and save complete document back to database with all fields. This triggers crypt of a password that is not sendt in mutation.

I find http://mongoosejs.com/docs/api.html#document_Document-update to be more in the right place as it just sends a update to model containing just the fields to update.

😉

st0ffern avatar Mar 31 '17 10:03 st0ffern

I can confirm when updating attributes on a subdocument i.e.,

{
  _id: 'someId',
  profile: { 
      ...
   }
}

the update wipes out fields that are not passed along with the query.

smooJitter avatar Jul 30 '17 23:07 smooJitter

@smooJitter From MongoDB docs:

To update an embedded document or an array as a whole, specify the replacement value for the field. To update particular fields in an embedded document or in an array, use dot notation to specify the field.

I think, that it also will be true for using with mongoose. And it will work with graphql-compose-mongoose if we wrap updateById resolver where make record fields flat via dot-notation keys.

So partial updating of embedded docs is quite tricky. Maybe it will be nice to write additional standard resolver for graphql-compose-mongoose and call it like updatePartialById.

nodkz avatar Jul 31 '17 08:07 nodkz

So we ran into this issue as well. Before writing a resolver, we found a work around just using updateMany(filter: {_id: ID}, record: { field: value}}). It works in most cases when you don't need the record back (since updateMany only returns numAffected).

jrmeier avatar Nov 09 '18 20:11 jrmeier

I have this same issue is there a solution?

SabrinaDanielle avatar Jan 08 '19 04:01 SabrinaDanielle

I know this issue is quite old now, but this was something that I wanted to achieve in a universal manner, and I found a way that seems to work nicely.

I created this before-save hook which can be applied to all my updateById. This goes in a file called "mergeFields.js"

function isObject(item) {
  return item && typeof item === "object" && !Array.isArray(item);
}

export function merge(target, source) {
  // sanity check - probably overkill
  if (!isObject(source)) {
    return source !== null ? source : undefined;
  }
  const output = target;
  const entries = Object.entries(source);
  for (let i = 0; i < entries.length; i += 1) {
    const [key, value] = entries[i];
    if (isObject(source[key])) {
      if (target[key] === undefined) {
        // add new embedded documents
        output[key] = value;
      } else {
        // recursively merge existing embedded documents
        output[key] = merge(target[key], value);
      }
    } else {
      // remove existing fields with a `null` input
      output[key] = value !== null ? value : undefined;
    }
  }
  return output;
}

export default function mergeFields(resolvers) {
  Object.keys(resolvers).forEach((k) => {
    resolvers[k] = resolvers[k].wrapResolve(
      (next: any) => async (rp) => {
        rp.beforeRecordMutate = async (doc, rp) => {
          const entries = Object.entries(rp.args.record);
          for (let i = 0; i < entries.length; i += 1) {
            const [key, value] = entries[i];
            if (isObject(value) && !(value instanceof Date)) {
              if (doc[key] === undefined) {
                // add new embedded documents that don't exist yet
                doc[key] = value;
              } else {
                // merge new embedded documents into existing ones
                rp.args.record[key] = merge(doc[key], rp.args.record[key]);
              }
            } else if (value === null) {
              // remove existing fields with a `null` input
              rp.args.record[key] = undefined;
            }
          }
          return doc;
        };
        return next(rp);
      }
    );
  });
  return resolvers;
}

Then when I add my resolvers:

import mergeFields from "./mergeFields";

export const NestedExampleMutation = {
  ...mergeFields({ updateNestedExample: NestedExampleTC.getResolver("updateById") }),
  addNestedExample: NestedExampleTC.getResolver("createOne"),
};

If this approach actually works as well as it seems to in my testing, it might be nice to have it available as an option in UpdateByIdResolverOpts.

BeauGieskens avatar Dec 01 '20 02:12 BeauGieskens