sqlmancer icon indicating copy to clipboard operation
sqlmancer copied to clipboard

Add support for multiple clients

Open danielrearden opened this issue 4 years ago • 5 comments

The @sqlmancer directive could accept a list configuration objects instead of just one, allowing the generation of multiple database clients. This would be useful for projects that utilize more than one database. A name option would be necessary to distinguish between clients and identify which client (or clients) a particular model belongs to.

danielrearden avatar Apr 23 '20 23:04 danielrearden

The function createSqlmancerClient currently only receives a single knex instance and return it augmented with the models. This should be the other way around, having the models returned augmented with their respective knex instances. This would allow passing multiple, scoped, knex instances. For example :

const pgKnex = Knex({
  client: 'pg',
  connection: process.env.PG_CONNECTION_STRING,
});
const myKnex = Knex({
  client: 'mysql',
  connection: process.env.MY_CONNECTION_STRING,
});
const models = createSqlmancerClient('./src/**/*.graphql', {
  pg: pgKnex
  mysql: myKnex
});

In order to specify each model's connection, the @model directive should allow specifying the named knex instance. For example :

type Actor @model(db: "pg", table: "actor", pk: "id") {
  id: ID!
  firstName: String!
  lastName: String!
}

type Movie @model(db: "mysql", table: "movies", pk: "id") {
  id: ID!
  name: String!
  year: Number!
  genre: MovieGenreEnum!
}

Therefore :

models.Actor.knex === pgKnex;  // true
models.Movie.knex === myKnex;  // true

yanickrochon avatar Jun 18 '20 14:06 yanickrochon

@yanickrochon I think the above suggested changes to the API make perfect sense. However, I don't think consumers of the API should have to go through a particular model to access the Knex instance. In other words, I should be able to do:

client.mysql.transaction(trx => { ... })

instead of

models.Actor.knex.transaction(trx => { ... })

So the typing would look something like:

type SqlmancerClient = {
  [key: string]: Knex & { models: Models }
}

If a single Knex instance is passed in instead of a map, we can default to using knex or db as the key.

I'm not opposed to exposing the underlying Knex instance as a property on each individual model -- but I don't think the relationship between the connection and its models should be completely inverted.

danielrearden avatar Jun 20 '20 23:06 danielrearden

Sure! The point was to expose each database connections. So, what you're suggesting is fine with me also. But I fail to see why createSqlmancerClient should return the connections if they are already provided as second argument. That is, this does not make much sense :

const db = {
  pg: pgKnex
  mysql: myKnex
};
const client = createSqlmancerClient('./src/**/*.graphql', db);
// client.db === db

What is actually relevant are the models. Something like :

const db = {
  pg: pgKnex
  mysql: myKnex
};
const models = createSqlmancerClient('./src/**/*.graphql', db);

export { db, models };

In short, whatever db is, if it's a Knex instance, then all models will make use of it, otherwise it will be an object defining named connections. No need to define a map with some default properties, the whole will be user-specific.

yanickrochon avatar Jun 20 '20 23:06 yanickrochon

@yanickrochon Just to provide some context: the "client" returned by createSqlmancerClient is meant to be an object that includes the defined models and also exposes the methods provided by the underlying Knex instance. At the moment, we're just assigning the models as a property of the Knex instance, but the "client" object is really intended to function as a proxy for those methods. This was an intentional design choice stemming from potentially needing to expose other properties or methods in the future that are not model-specific.

We could just have a createSqlmancerModels function, which is effectively what you're proposing. But I think I'd like to maintain the concept of a "client" that we could tack additional properties onto if needed. In that context, each connection/Knex instance would have its own named client, with its own set of models and whatever other methods.

danielrearden avatar Jun 20 '20 23:06 danielrearden

However, the current design, along with issue #122, introduces a chicken and the egg paradox, namely that creating the models from an already defined schema, including the typeDefs and resolvers, and returning the models, does create a problem of module interdependence.

  • the models are used in the resolvers
  • the models are created from the resolvers

This is why I was suggesting keeping the models inside an object exposed by SQL Mancer, something like

// resolver.js
import { models } from 'sqlmancer';

export default {
  Query: {
    films: (root, args, ctx, info) => models.Film.findMany().resolveInfo(info).execute()
  }
};
// setup.js
const typeDefs = loadTypeDefs();
const resolvers = loadResolvers();
const schema = makeSqlmancerSchema({ typeDefs, resolvers });
const db = { ... };
const client = createSqlmancerClient(schema, db);

However, one way of simplifying this would be to change the way this function is called, and provide a way to pass optional arguments, for example

interface SqlmancerClientOptions {
  glob: String,
  db: Knex | Object<String,Knex>,
  typeDefs: Object,
  resolvers: Object,
  schema: Object
}
function createSqlmancerClient<T extends GenericSqlmancerClient = GenericSqlmancerClient>(options: SqlmancerClientOptions);

yanickrochon avatar Jun 21 '20 00:06 yanickrochon