ERROR: Can't extend QueryBuilder with existing method ('cache') - when initating with knex connection

[onasman]

When initing a new SQLDataSource in the datasources function with an existing knex connection it throws the following error:

Can't extend QueryBuilder with existing method ('cache')

Option 1: This does not work:

const dbConnection = Knex(knexConfig)

const server = new ApolloServer({
         schema,
         dataSources: () => ({ db: new SQLDataSource(dbConnection) })
})

The below both works: Option 2: when you insert a dataSource that's called outside of the of the datasource function

const dbConnection = Knex(knexConfig)

const db = new SQLDataSource(dbConnection)

const server = new ApolloServer({
         schema,
         dataSources: () => ({ db })
})

Option 3: Or when you call the SQLDataSource function with the config instead of an existing connection

const server = new ApolloServer({
         schema,
         dataSources: () => ({ db: new SQLDataSource(knexConfig) })
})

The first example which doesn't work is what's most appealing to me. I have an external rest api that also uses the db connection so would be good to pass that one along. And according to the docs the datasources function should create a new instance of each data source for each operation which is not the case for option 2.

[taybin]

I'm seeing this too.

[cvburgess]

Hmm this was reported and fixed before.

What version are you using? https://github.com/cvburgess/SQLDataSource/issues/32

[taybin]

I'm using version 2.0.1. With typescript 4.7, which might be relevant.

[taybin]

I think the issue is that the apollo documentation suggests creating a new DataSource on every call, like:

const server = new ApolloServer({
  typeDefs,
  resolvers,
  cache,
  context,
  dataSources: () => ( { db: new MyDatabase(knexConfig) })
});

This would cause the cache method to be added multiple times which wouldn't happen with the example in your README.

[ddziara]

Hi, this is code from the constructor of SQLDataLoader. Method "extend" checks if given property exists and throws Error if it does. Couldn't you just do the same but skip calling extend in such case?

if (!this.db.cache) {
  Knex.QueryBuilder.extend("cache", function(ttl) {
    return _this.cacheQuery(ttl, this);
  });
}

[marvin-kolja]

I think the issue is that the apollo documentation suggests creating a new DataSource on every call, like:

const server = new ApolloServer({
  typeDefs,
  resolvers,
  cache,
  context,
  dataSources: () => ( { db: new MyDatabase(knexConfig) })
});

This would cause the cache method to be added multiple times which wouldn't happen with the example in your README.

I would not do this, since this will also create a new knex instance on every call. 200 calls mean 200 new database connections. In production this is very bad. The suggestion by @ddziara seems very good since it allows to only have one knex instance and just create a new data source on every call.

[daniel-keller]

Extending Knex will not retroactively add the custom method to existing instances of knex. As @ddziara mentions the issue lies in the SQLDataSource constructor. If we instantiate the database connection before passing it into SQLDataSource the cache method is added to the QueryBuilder prototype and will apply to all future instances but not the one we passed in.

constructor(knexConfig) {
    super();

    this.context;
    this.cache;

    if (typeof knexConfig === "function") {
      this.db = knexConfig;    // <=== If knexConfig is instantiated in advance
    } else {
      this.db = Knex(knexConfig); // <==== side note: this first instance of this.db will also not include the cache method but this bug never occurs b/c this SQLDataSource is instantiated once when the server starts (without cache) and then again on every request (with cache).
    }

    this.knex = this.db;

    const _this = this;
    if (!this.db.cache) {      // <==== this.db.cache will never be set  if we passed in an existing connection (i.e. typeof knexConfig === "function")
      Knex.QueryBuilder.extend("cache", function(ttl) { // <==== this will only apply custom cache method to future instances of knex not the existing instance (this.db)
        return _this.cacheQuery(ttl, this);
      });
    }
  }

This creates a catch-22. Reinstantiating the connection to add the cache method defeats the purpose of passing the connection into the SQLDataSource but we can't extend the querybuilder prior to instantiating the SQLDataSource b/c the cache method is dependent on non static properties of the SQLDataSource (cacheQuery). The error is thrown when extend tries to re-add the cache method to the QueryBuilder prototype b/c it wasn't found on the current instants of this.db.

ERROR: Can't extend QueryBuilder with existing method ('cache')

The work around I've come up with is to manually extend this.db with the cache method. This is a little hacky and we still need to call QueryBuilder.extend which adds the cache method to the builder prototype preventing knex from complaining that cache doesn't exist but now we can use the same database connection for all of our request.

New SQLDataSource constructor

constructor(knexConfig) {
    super();

    this.context;
    this.cache;

    if (typeof knexConfig === "function") {
      this.db = knexConfig;
    } else {
      this.db = Knex(knexConfig);
    }

    this.knex = this.db;
    const _this = this;
    
    const cachefn = function(ttl) {
      return _this.cacheQuery(ttl, this);
    };

    if (!this.db.cache) {
      // Extend modifies Knex prototype to include cache method.
      // Does not retroactively add cache to existing connection.
      Knex.QueryBuilder.extend("cache", cachefn);
      // Must be manually added to existing knex connection.
      this.db.cache = cachefn;
    }
  }

PR #84