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

Passing parameters to model getters #7404

Closed
jharting opened this Issue Mar 21, 2017 · 4 comments

Comments

3 participants
@jharting
Contributor

jharting commented Mar 21, 2017

This is a feature request. There may already be a way to achieve what I am trying to do in which case I'll be grateful for pointers.

What you are doing?

I am implementing a versioned API. Assume the following simple example:

GET /v1/places

200 OK
[{
    name: "Luigi's Pizza Place",
    rating: 84
}, {
    name: "Greek Taverna",
    rating: 78
}]

The rating field is implemented as an INTEGER in the db and expected to have values 0-99

Now assume that for v2 of the API we need to change how we present rating so that it instead of 0-99 it returns 0-4 floating point number rating.

We can implement that easily with model a getter:

getterMethods: {
    rating: function () {
        return this.getDataValue('rating') / 20;
    }
}

So far so good.

The problem now is how to preserve backwards compatibility in v1 but at the same time support new representation in v2. IMHO the cleanest would be to do that in the getter, e.g:

getterMethods: {
    rating: function () {
        const value = this.getDataValue('rating');

        if (apiVersion === 1) {
            return value;
        }

        return value / 20;
    }
}

Now what I am struggling with is how to actually pass the apiVersion down to model getters. I can obviously get that value from some global constant but that can get tricky as execution of different requests may overlap.

The cleanest approach IMO would be for Sequelize to support passing in query context. Here's an example:

sequelize.places.findAll({
    limit: 5
},
{ // this is a context object
    apiVersion: req.param.version
})

now the getter could be written as:

getterMethods: {
    rating: function (param, context) {
        const value = this.getDataValue('rating');

        if (context.apiVersion === 1) {
            return value;
        }

        return value / 20;
    }
}

WDYT? Is there already a nice way to implement this that I am missing? If not would the aforementioned proposal make sense in Sequelize?

@janmeier

This comment has been minimized.

Member

janmeier commented Mar 23, 2017

I think the options object passed to get will actually be passed down to each getter function http://docs.sequelizejs.com/en/v3/api/instance/#getkey-options-objectany (i.e. you would do

places.findAll().then(places => res.json(places.map(places.get({ apiVersion: 42 })))

If not, you could look into using cls or domains to maintain a context

@jharting

This comment has been minimized.

Contributor

jharting commented Mar 27, 2017

@janmeier thanks for the suggestion. It unfortunately does not seem to be the case. When I call

place.get({apiVersion: 3});

the rating getter is called with "rating" as a param.

Mind if I change the getter logic for this to work?

cls will work I assume but I don't love that solution as it seems to be an overkill for this usecase.

I was thinking about another approach to address this. What I am trying to achieve would be trivial if the following feature was available in Sequelize: When I issue a Sequelize query then the original query object (options object passed to findAll, findOne, etc...) is made available to the model instance

Example:

const options = {
    where: {
        ...
    }
    limit: 5,
    request: {
        apiVersion: 3
    }
});

sequelize.places.findAll(options);

Sequelize would make the options object passed in available in the instance under an attribute of this, e.g. this.options or this.query

The getter would then look like:

getterMethods: {
    rating: function (param, context) {
        if (this.query.request.apiVersion === 1) {
            return value;
        }

        return value / 20;
    }
}

@janmeier WDYT?

jharting added a commit to jharting/sequelize that referenced this issue Mar 27, 2017

@jharting jharting changed the title from Passing request context to model getters to Passing parameters to model getters Mar 27, 2017

jharting added a commit to jharting/sequelize that referenced this issue Mar 27, 2017

@jharting

This comment has been minimized.

Contributor

jharting commented Mar 27, 2017

@janmeier actually you can disregard the second part of my previous comment. Fixing the options object passing to getters is actually all I need. I sent a PR here: #7435

@sushantdhiman

This comment has been minimized.

Member

sushantdhiman commented Apr 1, 2017

Fixed in #7435

sushantdhiman added a commit that referenced this issue Apr 1, 2017

V3 #7404 make it possible to pass parameters to getter functions (#7435
…) (#7441)

* #7404 make it possible to pass parameters to getter functions (#7435)

#7404

* [ci-skip] Changelog

holm added a commit to holm/sequelize that referenced this issue Nov 13, 2017

V3 sequelize#7404 make it possible to pass parameters to getter funct…
…ions (sequelize#7435) (sequelize#7441)

* sequelize#7404 make it possible to pass parameters to getter functions (sequelize#7435)

sequelize#7404

* [ci-skip] Changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment