Map raw field names to attributes.

[sankethkatta]

Add feature #3714.

This also allows the returning: true option for update in postgres to work properly. Currently it generates out RETURNING *, which faces the same problem as described with raw queries in #3714.

[mickhansen]

I think it would be better if it happened at a lower level than build so it could be reused for bulkCreate etc.

[sankethkatta]

Any suggestions where a better place for it would be? Maybe in the Instance constructor or initValues?

Also, not sure if you have any insight into the error the build system is throwing, I am not able to reproduce the error locally (and it seems unrelated to my changes):

 Error: Expected 0 running queries. 1 queries still running in [POSTGRES-NATIVE] Continuation local storage bluebird shims then rejected

[mickhansen]

Probably in post query result parsing, let the caller pass a field to attribute map that the query result parser can use perhaps. It's something we need a generalized approach to.

[sankethkatta]

I attempted to generalize this a bit more and moved the aliasing into handleSelectQuery. It uses an option called aliasMap which is passed into Sequelize.prototype.query.
Let me know what you think.

[mickhansen]

result[field] = undefined is a bit quicker, can you check if it produces the same result?

[sankethkatta]

It still passes the tests, however, it leaves remnants in the dataValues object.
For example, this is from one of the tests, email_address was aliased to emailAddress.

dataValues: { 
  id: 1,
  username: 'john',
  email_address: undefined,
  createdAt: Sun Jan 01 2012 02:10:10 GMT-0800 (PST),
  updatedAt: Sun Jan 01 2012 02:10:10 GMT-0800 (PST),
  emailAddress: 'john@gmail.com' 
}

[mickhansen]

Ah yeah of course, we don't want that.

[mickhansen]

Looking good. Although i'm not sure about the aliasMap name.

[sankethkatta]

Looks like the other parameter names for .query() are single words like replacements. How about aliases or mappings?

[mickhansen]

That's even less descriptive though :)

[sankethkatta]

I see, more descriptive! How about rawFieldMap?

[mickhansen]

Maybe just fieldMap? I think that would be okay.

[sankethkatta]

That sounds good to me, updating it now.

[BridgeAR]

This should be written as

result.field !== undefined

[mickhansen]

@BridgeAR in causes de-opt? And it would be result[field]

[BridgeAR]

This depends on if any field name in result is somehow special or not. If it is e.g. Strange / field it'll deopt (there was an issue with someone using slashing in his field names :D ). Otherwise it should not but checking for undefined will be faster.

[BridgeAR]

LGTM (besides one minor comment) even tough I think it would be nice to improve this at some point to only include the fields needed from the beginning on. But this is only internal stuff.

[mickhansen]

@BridgeAR From the beggining? I'm not quite sure i understand? You mean parse and rewrite a raw query to alias the fields?

[BridgeAR]

Yes

[sankethkatta]

Just pushed the minor change, checking if the field existed in the result using undefined as suggested.

[mickhansen]

@BridgeAR That does not sound feasible at all, don't think we'll ever do that ;p Rewriting raw queries would be a bit too magic i think. (Why is the input not my actual input?)

[mickhansen]

We should probably put this under the raw if so it isn't run on all selects, no need to encur the extra loop when it's not needed.

[sankethkatta]

Currently the fix also works for postgres returning: true on update. We'd have to put it behind this.options.raw || this.options.type === QueryTypes.BULKUPDATE.

[mickhansen]

That sounds like a bug, update should not result in a handleSelectQuery call.

[sankethkatta]

I assumed it did this by design as handling a UPDATE ... RETURNING * is similar to a select. Here is the call to handleSelectQuery in postgres/query.js.

[mickhansen]

Huh, yeah maybe you're right. I'm afraid we can't incur that perf overhead for all select queries since they are already perf sensitive and wouldnt take nicely to another nested loop, please do the large if :)

[sankethkatta]

Hmm, it looks like this.options.raw isn't set if a model is passed in query.js#L671. Is there a way to know if the query originated from user input on sequelize.query directly without using options.raw?

[mickhansen]

Hmm no, no way to know, that is a bit of a problem since actually in this case you don't want a raw result.

[sankethkatta]

We could introduce a new option to toggle on or off the auto-mapping to model attributes behavior. Right now we assume we should map fields anytime the model option is set.
But it would require users writing raw queries to set on the new option in addition to setting the model.

[sankethkatta]

@mickhansen Trying another approach to this problem. I replaced fieldMap with a boolean mapToModel option. The mapping will no longer be on the code path for select queries, but will require users of the raw query api to explicitly set mapToModel: true to enable the behavior.

Let me know your thoughts!

[janmeier]

@sankethkatta I like this approach, it looks pretty clean to me. I'll let @mickhansen comment on this too before merging but LGTM :)

[sankethkatta]

@janmeier @mickhansen any update on this?

[janmeier]

@sankethkatta Mick is on vacation, I'll bug him about this when he's back ;)

[sankethkatta]

Thanks! 😃

[sankethkatta]

ping! 🔔

[mattprivman]

Merge please?

[mickhansen]

Hmm, nevermind the last comment i just deleted.

[sankethkatta]

I don't necessarily like that the current approach requires the user to set the mapToModel option. However, given the performance limitations of running unnecessary loops and the fact it is not possible to know whether sequelize.query was called directly by the user (which we discussed pull/3995#discussion_r34085726). This seems like the best compromise, and I'd rather err on the side of explicitness anyways.

Let me know if you can think of a better way!

[mickhansen]

Sorry for being so late to comment.
Ideally i'd like a generic mapper passed to query, or one generated in query and then used here.

The less the query generator and query handler needs to know about Model the better.

Refactor this specific code to use this.options.fieldMap and then in sequelize.query you can use options.fieldMap = mapToModel && options.model && etc.

[sankethkatta]

Refactored in latest commit.

[mickhansen]

[FEATURE] > [ADDED]
perhaps: "Map raw fields back to attribute names when using returning ..."

[mickhansen]

Please add API docs to sequelize.query describing fieldMap and mapToModel options.
Excellent work!

(please squash commits to a single descriptive commit when ready aswell)

[sankethkatta]

I addressed the comments and squashed the commits.
As for the documentation, do I need to write somewhere else in addition to the JSDoc? (which I did on lib/sequelize#L643.
Thanks!

[mickhansen]

Nope jsdoc works :)

[mickhansen]

Thank you for your work!

[sankethkatta]

Awesome! Thank you for working through this with me! 😃