Cache false positives when new properties added to options

[RichAyotte]

I'm adding a meta property to options to provide Sequelize with context but it causes the cache to return false positives.

The context is saved in my the graphql-sequelize before resolve hook

const beforeResolve = (options = {}, args, context, info) => {
  options.meta = {
    context
  }
  return options
}

meta isn't considered in getCacheKey so I end up with false positives.

dataloader-sequelize/src/index.js

Line 68 in 38eb295

options = stringifyObject(options, ['association', 'attributes', 'groupedLimit', 'limit', 'offset', 'order', 'where', 'through', 'raw']);

Instead of using a list of harcoded values, would Object.keys() work?

Also, adding meta to the list causes a GraphQLError: Maximum call stack size exceeded in the custom stringifier

dataloader-sequelize/src/index.js

Line 63 in 38eb295

function stringifyObject(object, keys = Object.keys(object)) {

The reason for the custom stringifier isn't clear to me, I'm not sure why JSON.stringify isn't liked.

I'm also not sure if Object.keys() would be very robust. Perhaps a hashing function would be better, something like https://github.com/puleos/object-hash?

In the meantime, what's the best workaround? Can the cache be disabled or reset before resolve?

[RichAyotte]

I had another quick peak at the code and I think you could replace:

function stringifyValue(value, key) {
  if (value && value.associationType) {
    return `${value.associationType},${value.target.name},${value.as}`;
  } else if (Array.isArray(value)) {
    if (key !== 'order') {
      // attribute order doesn't matter - order order definitely does
      value = clone(value).sort();
    }
    return value.map(stringifyValue).join(',');
  } else if (typeof value === 'object' && value !== null) {
    if (value instanceof Date) return value.toJSON();
    return stringifyObject(value);
  }
  return value;
}

// This is basically a home-grown JSON.stringifier. However, JSON.stringify on objects
// depends on the order in which the properties were defined - which we don't like!
// Additionally, JSON.stringify escapes strings, which we don't need here
function stringifyObject(object, keys = Object.keys(object)) {
  return keys.sort().map(key => `${key}:${stringifyValue(object[key], key)}`).join('|');
}

export function getCacheKey(model, attribute, options) {
  options = stringifyObject(options, ['association', 'attributes', 'groupedLimit', 'limit', 'offset', 'order', 'where', 'through', 'raw']);

  let name = `${model.name}|${attribute}|${options}`;
  const schema = model.options && model.options.schema;
  if (schema) {
    name = `${schema}|${name}`;
  }
  return name;
}

With something like:

import hash from 'object-hash';

export function getCacheKey(model, attribute, options) {
  return hash({
    model,
    attribute,
    options
  })
}

[RichAyotte]

Commit 1 actually had something similar, not a hash but a single line that built a string, simply:

let cacheKey = model.name + attribute + JSON.stringify(options);

The complexity grew out of the need to support associations, where, limit, offset and other properties. I'm assuming that these changes increased cache hits but can someone provide insight on how this works?

[RichAyotte]

@mickhansen This and a few other issues currently open are cache related. Have you done benchmarks recently with and without caching? It doesn't seem like it would help very much, especially with all the complexity added since the first commit.

[RichAyotte]

Batching stops working with the cache disabled.

I'm having a hard time wrapping my head around this code and I'm not sure if it's because I'm not the author or if it's just a complex problem.

[mickhansen]

@RichAyotte The library really needs to be refactored so that it works off request caching rather than global caching.

I'm thinking something like:

context.dataloaderSequelize = dataloaderSequelize(sequelize);
SomeModel.findOne({
  graphqlContext: context
});

[idris]

Sounds about right. graphqlContext is a bit too graphql-specific. Maybe it should be:

req.dataloaderContext = dataloaderSequelize(sequelize);
SomeModel.findOne({
  dataloaderContext: req.dataloaderContext
});

[mickhansen]

@idris yeah that works, could just keep the option name to find dataloaderSequelize

[janmeier]

In connection with #43 (review), does object-hash support symbol keys?

[RichAyotte]

I had to use stringify in my implementation. It looks something like this. where is the original where object without the primary key.

const loaderId = hash(
    JSON.stringify({
        attributes: options.attributes
        , include: options.include
        , limit: options.limit
        , lock: options.lock
        , offset: options.offset
        , order: options.order
        , paranoid: options.paranoid
        , raw: options.raw
        , table: target.options.tableName
        , transaction: options.transaction
        , where
    })
    , {
        encoding: 'binary'
    }
)

[RichAyotte]

Symbol keys aren't supported in stringify nor object-hash.
puleos/object-hash#53
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol#Symbols_and_JSON.stringify()

[RichAyotte]

I've implemented a simpler dataloader here that seems pretty robust but it's much simpler and I feel like I might be missing something regarding the joins.

https://gist.github.com/RichAyotte/cb4cae6147f2f8ca1c31371b640793b5

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.