Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for multiple clients #15

Open
danielrearden opened this issue Apr 23, 2020 · 5 comments
Open

Add support for multiple clients #15

danielrearden opened this issue Apr 23, 2020 · 5 comments
Labels
enhancement New feature or request

Comments

@danielrearden
Copy link
Owner

danielrearden commented Apr 23, 2020

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 danielrearden added the enhancement New feature or request label Apr 23, 2020
@danielrearden danielrearden added this to the Possible features milestone Apr 23, 2020
@yanickrochon
Copy link

yanickrochon commented Jun 18, 2020

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

@danielrearden
Copy link
Owner Author

@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.

@yanickrochon
Copy link

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.

@danielrearden
Copy link
Owner Author

@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.

@yanickrochon
Copy link

yanickrochon commented Jun 21, 2020

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);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants