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

Changes to how sequelize.literal and cousins is escaped #1916

Merged
merged 1 commit into from Jun 17, 2014

Conversation

2 participants
@janmeier
Member

janmeier commented Jun 17, 2014

This uses a modified version of the test from #1910, and closes #1875, #1915 and #1556

return attr.toString(self); // We trust the user to rename the field correctly
} else if (attr instanceof Utils.cast ||
attr instanceof Utils.fn
) {

This comment has been minimized.

@janmeier

janmeier Jun 17, 2014

Member

Not sure if throwing an error is the best thing to do here?

However, if you use fn or cast, you must specify an AS for that to actually be returned to a model

This comment has been minimized.

@mickhansen

mickhansen Jun 17, 2014

Contributor

throwing is fine imo. Being explicit about where users go wrong is never bad.

This comment has been minimized.

@janmeier

janmeier Jun 17, 2014

Member

Should we do the same with regular finds then?

This comment has been minimized.

@mickhansen

mickhansen Jun 17, 2014

Contributor

What do we do currently? I mean throw should be the same as reject inside a promise right?

This comment has been minimized.

@janmeier

janmeier Jun 17, 2014

Member

Nothing :D

This comment has been minimized.

@mickhansen

mickhansen Jun 17, 2014

Contributor

Well explicit is better than nothing :D

@@ -639,7 +639,7 @@ module.exports = (function() {
*
* @param {Object} [options] A hash of options to describe the scope of the search
* @param {Object} [options.where] A hash of attributes to describe your search. See above for examples.
* @param {Array<String>} [options.attributes] A list of the attributes that you want to select

This comment has been minimized.

@janmeier

janmeier Jun 17, 2014

Member

Mentioned here that you can use [] to rename an attribute

@@ -1,9 +1,11 @@
"use strict";

This comment has been minimized.

@janmeier

janmeier Jun 17, 2014

Member

Added strict mode, and changed clearDB to use promises

@@ -110,43 +110,6 @@ describe(Support.getTestDialectTeaser("Utils"), function() {
})
})

describe('isHash', function() {

This comment has been minimized.

@janmeier

janmeier Jun 17, 2014

Member

All places there used isHash changed to use isPlainObject / isObject as appropriate

{
model: PostComment,
attributes: [
Sequelize.literal('EXISTS(SELECT 1) AS "PostComments.someProperty"'),

This comment has been minimized.

@mickhansen

mickhansen Jun 17, 2014

Contributor

Won't the quotes fail on non-pg?

attributes: [
Sequelize.literal('EXISTS(SELECT 1) AS "PostComments.someProperty"'),
[Sequelize.literal('EXISTS(SELECT 1)'), 'someProperty2'],
['comment_title', 'commentTitle']

This comment has been minimized.

@mickhansen

mickhansen Jun 17, 2014

Contributor

You are aliasing to a field that doesn't exist?

}).then(function () {
return User.findAll({
attributes: [
Sequelize.literal('EXISTS(SELECT 1) AS "someProperty"'),

This comment has been minimized.

@mickhansen

mickhansen Jun 17, 2014

Contributor

Quotes might not work outside of PG

mickhansen added a commit that referenced this pull request Jun 17, 2014

Merge pull request #1916 from sequelize/theBigEscape
Changes to how sequelize.literal and cousins is escaped

@mickhansen mickhansen merged commit e93b229 into master Jun 17, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@janmeier janmeier deleted the theBigEscape branch Jun 18, 2014

janmeier added a commit that referenced this pull request Jun 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment