search

feathersjs-ecosystem / feathers-sequelize

A Feathers service adapter for the Sequelize ORM. Supporting MySQL, MariaDB, Postgres, SQLite, and SQL Server
MIT License
208 stars 75 forks source link

Support withSchema [with PR] #433

Open Artaud opened 2 months ago

Artaud commented 2 months ago

I'd like to dynamically change schema based on request params, which can be done in Sequelize by calling the model with withSchema, similarly to withScope usage which is already supported by feathers-sequelize.

Is that something you would accept in feathers-sequelize? I can propose a PR.

Artaud commented 2 months ago

At the currently supported version of sequelize, it would be just schema which is supposed to be deprecated by withSchema in sequelize 7

Artaud commented 2 months ago

I also see that you are currently getting to a new major release. I'd love to slip this addition in :)

Artaud commented 2 months ago

I'm not sure how to structure the addition though. I see that applyScope is deprecated in favor of ModelWithScope, which is being used as a model in all DB methods. If I wanted to add schema support, I either can:

  1. add a new function similar to applyScope, like applySchema
  2. alter the ModelWithScope to be loosened to ModelWithScopeAndSchema which I really don't like for its smell, or
  3. assume that support for more sequelize props can be added in the future, and make the Model function more generic such as: 
    ModelWithSequelizeParams(params: ServiceParams) {
const Model = this.getModel(params)
# apply scope if present in params
# apply schema if present in params
# here be space for future additions
return Model
}

    and then use this in all DB methods.

What do you think?

DaddyWarbucks commented 2 weeks ago

I am sorry for the slow response on this! I will investigate Sequelize's withSchema options and see if we can get that added.

Artaud commented 2 weeks ago

Thanks! I can see that you've got a lot on your hands (don't we all :D) so I completely understand, no rush :) I've been already using the change i proposed in the PR in production (using patch-package) and it's been working good so far.