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 default validation based on attribute type #3472

Merged

Conversation

@fixe
Copy link
Contributor

@fixe fixe commented Apr 6, 2015

We were thinking about adding a validation error to prevent database errors from being thrown since we already know the type before performing the query.

This is immensely useful with PostgreSQL to stop malformed queries from reaching the database.

@janmeier is this something that you guys are willing to merge? If so I can add some test cases.

@mickhansen
Copy link
Contributor

@mickhansen mickhansen commented Apr 6, 2015

Definitely! Schema inferred validations is something we've talked about previously that we'd like to do.
We just need to make sure they are skipped if allowNull: true and val === null

@mickhansen
Copy link
Contributor

@mickhansen mickhansen commented Apr 6, 2015

However like with the allowNull schema validator we might get some complaints that the error messages aren't customizable.

@@ -61,6 +61,10 @@ SqlString.escape = function(val, stringifyObjects, timeZone, dialect, field) {
return val + '';
}

if (field && field.type && field.type.validate) {
field.type.validate(val);
Copy link
Contributor

@mickhansen mickhansen Apr 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this should be part of the validator codebase instead?

Copy link
Member

@janmeier janmeier Apr 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I feel this would fit better in built in validators

Copy link
Contributor Author

@fixe fixe Apr 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the best place to put this without losing the type validation when performing queries?

Copy link
Contributor

@mickhansen mickhansen Apr 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fixe You mean all non instance-create/update queries? Not sure, instance-validator wouldn't be enough certainly.

Copy link
Contributor Author

@fixe fixe Apr 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm talking about every type of query that can be generated by Sequelize. Since we already know the data type of the columns that we are querying we can prevent an error from being thrown by the database.

Copy link
Contributor Author

@fixe fixe Apr 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@mickhansen mickhansen Apr 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fixe I'm still giving it some thought, i like the idea of catching all types before they hit the database but escape seems like an odd place to have it (but yes it's likely the only place being called by all types of values before going into sql).

@janmeier ?

Copy link
Member

@janmeier janmeier Apr 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps in query-generator.escape?

Not much better, but at least the qg is our own code, whereas sql-string is adapted from https://www.npmjs.com/package/sql-string so we might want to keep that as free of external modifications as possible

Copy link
Contributor Author

@fixe fixe Apr 15, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original take at this was exactly in that method but it's being overridden in postgres dialect. Suggestions?

Copy link
Member

@janmeier janmeier Apr 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, no, not really sure I can come up with better suggestions then :)

@BridgeAR
Copy link
Contributor

@BridgeAR BridgeAR commented Apr 7, 2015

Nice +1

@fixe fixe force-pushed the feature/add-default-validation branch from 885bb8d to a83ecd2 Apr 15, 2015
@BridgeAR
Copy link
Contributor

@BridgeAR BridgeAR commented Apr 22, 2015

@fixe Are you still going to fix the existing issues? I really like your proposal and if you do not have the time to finish it, I'd volunteer, even though I'd prefer you finishing the PR.

@fixe
Copy link
Contributor Author

@fixe fixe commented Apr 22, 2015

@BridgeAR: still unsure what's missing.

@mickhansen if I fix the outstanding issues will you guys merge this?

@mickhansen
Copy link
Contributor

@mickhansen mickhansen commented Apr 23, 2015

@fixe I need to confer with @janmeier, but yeah i think that it's extremely likely.
I like this as general protection but i also think it would be nice to apply to the schema validations so that it happens during the .validate() process aswell.

@fixe
Copy link
Contributor Author

@fixe fixe commented Apr 23, 2015

Let me know :-)

@janmeier
Copy link
Member

@janmeier janmeier commented Apr 24, 2015

I think we should move the validate call to escape in the query generator. It would introduce some duplication since we need ti both in abstract and in PG, but I think it would still be nicer than in sql string.

And as mick said, I think type validation should also be called in the validation step https://github.com/sequelize/sequelize/blob/master/lib/instance-validator.js#L189

So: move validate to escape, add validate to instance-validator, and add some tests that calling .validate throws the appropriate errors.

If you can come up with an awesome way to let users specify custom messages for the validations that'd be awesome, but if not thats fine and we can defer that to later :)

@fixe fixe force-pushed the feature/add-default-validation branch from a83ecd2 to a3b11be Apr 28, 2015
@BridgeAR
Copy link
Contributor

@BridgeAR BridgeAR commented May 19, 2015

Ping @fixe: Do you think you can finish this this week? I need this soon, since I had some issues today because of the missing validation.

@fixe
Copy link
Contributor Author

@fixe fixe commented May 19, 2015

@BridgeAR impossible -- drowned this week, sorry.

@BridgeAR
Copy link
Contributor

@BridgeAR BridgeAR commented May 19, 2015

Ok thx

@BridgeAR
Copy link
Contributor

@BridgeAR BridgeAR commented Jun 1, 2015

This fixes #1431

@fixe fixe force-pushed the feature/add-default-validation branch from a3b11be to c62bafa Jun 8, 2015
@fixe
Copy link
Contributor Author

@fixe fixe commented Jun 8, 2015

@janmeier: I have updated the PR by moving the code from sql string to the query generator. I need feedback in some validators (returning true for now).

@@ -376,6 +440,9 @@ var HSTORE = function() {
util.inherits(HSTORE, ABSTRACT);

HSTORE.prototype.key = HSTORE.key = 'HSTORE';
HSTORE.prototype.validate = function(value) {
return true;
Copy link
Member

@janmeier janmeier Jun 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_.isPlainObject and maybe check that it is not more than one level deep

Copy link
Contributor Author

@fixe fixe Jun 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you suggest to check for more than one level deep here?

@janmeier
Copy link
Member

@janmeier janmeier commented Jun 21, 2015

Ping @fixe

@fixe fixe force-pushed the feature/add-default-validation branch from c62bafa to 73c0b70 Jun 21, 2015
@fixe
Copy link
Contributor Author

@fixe fixe commented Jun 21, 2015

@janmeier implemented your suggestions and added some tests. Take a look.

test('should throw an error if `value` is invalid', function() {
var type = DataTypes.STRING();

try {
Copy link
Member

@janmeier janmeier Jun 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect(function () {
     type.validate(12345);
}).to.throw(Sequelize.ValidationError, '12345 is not a valid string');

Copy link
Member

@janmeier janmeier Jun 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same applies for the other tests

@janmeier
Copy link
Member

@janmeier janmeier commented Jun 22, 2015

Hmm, for some reason this is failing miserably for postgres - Most of the errors seem to come from .values being empty in enums...

Do you need help on this @fixe or can you debug yourself?

@fixe fixe force-pushed the feature/add-default-validation branch from 73c0b70 to 49d4a7c Jun 22, 2015
@fixe
Copy link
Contributor Author

@fixe fixe commented Jun 22, 2015

@janmeier the thing is that enums are being created in a way that the data type don't have access to values:

owner_type: { type: DataTypes.ENUM, values: ['user', 'org'], defaultValue: 'user', unique: 'combiIndex' }

In order for the validation to work the following must be done:

owner_type: { type: DataTypes.ENUM({ values: ['user', 'org'] }), defaultValue: 'user', unique: 'combiIndex' }

WDYT?

@fixe fixe force-pushed the feature/add-default-validation branch from b969f47 to a243a82 Jul 14, 2015
@fixe
Copy link
Contributor Author

@fixe fixe commented Jul 14, 2015

@janmeier PR updated, can you guys port this to other dialects?

@janmeier
Copy link
Member

@janmeier janmeier commented Jul 15, 2015

@fixe I can sure give it a try :)

@@ -406,7 +406,7 @@ QueryInterface.prototype.renameColumn = function(tableName, attrNameBefore, attr

_options[attrNameAfter] = {
attribute: attrNameAfter,
type: data.type,
type: DataTypes[data.type],
Copy link
Member

@janmeier janmeier Jul 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fixe This was breaking a mysql test so I removed it. Tests seem to pass fine without this change

@janmeier janmeier merged commit a243a82 into sequelize:master Jul 17, 2015
1 check failed
janmeier added a commit that referenced this issue Jul 17, 2015
…tion

Add default validation based on attribute type
@fixe fixe deleted the feature/add-default-validation branch Jul 18, 2015
@fixe
Copy link
Contributor Author

@fixe fixe commented Jul 18, 2015

👍

@janmeier
Copy link
Member

@janmeier janmeier commented Jul 19, 2015

Good work @fixe !

defunctzombie
Copy link
Contributor

defunctzombie commented on a243a82 Aug 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mickhansen this check breaks code because isNumber doesn't allow for string values which is what you usually want with Decimal since they may be outside the range of js numbers.

What was the desire behind this PR? Why not let the db handle the validation?

defunctzombie
Copy link
Contributor

defunctzombie commented on a243a82 Aug 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the checks help prevent other types of errors which is certainly good. Guess just this use case needs to be handled better.

janmeier
Copy link
Member

janmeier commented on a243a82 Aug 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4314

The motivation behind this PR is that failing fast is good - no need to bother the db

defunctzombie
Copy link
Contributor

defunctzombie commented on a243a82 Aug 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just let the db do it since it can also do it quickly. If the user wants more specific validation they can enable it. I think it is tricky in js because the type system is flexible and thus some element of fuzziness comes to be expected.

Anyhow, seems the validation was a bit rushed and didn't take into account enough cases so at least disabling it for now by default (since it does break behavior) could be a nice move to give users time to actively test the validation. Looking forward to the fixes :) If there is something that needs to be reviewed here or a PR made let me know.

@@ -317,6 +368,13 @@ BOOLEAN.prototype.key = BOOLEAN.key = 'BOOLEAN';
BOOLEAN.prototype.toSql = function() {
return 'TINYINT(1)';
};
BOOLEAN.prototype.validate = function(value) {
if (!_.isBoolean(value)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for using Validator.isBoolean(), _.isBoolean() is too strict even the value is number 1 or 0

@franciscotfmc
Copy link

@franciscotfmc franciscotfmc commented Oct 2, 2015

Hello guys,

Maybe I'm missing something here, but suppose we have one model with an integer attribute, with values from 1000 to 2000 in the database. My need is to search for all the rows that begin with '10'. In order for this to work, I would need to disable the type validation for queries. Am I wrong?

Thanks in advance!

@dannielhugo
Copy link

@dannielhugo dannielhugo commented Oct 2, 2015

+1 for @franciscotfmc comment

3 similar comments
@talitacampos
Copy link

@talitacampos talitacampos commented Oct 2, 2015

+1 for @franciscotfmc comment

@ceciliadeveza
Copy link

@ceciliadeveza ceciliadeveza commented Oct 2, 2015

+1 for @franciscotfmc comment

@shbmira
Copy link

@shbmira shbmira commented Oct 2, 2015

+1 for @franciscotfmc comment

@mickhansen
Copy link
Contributor

@mickhansen mickhansen commented Oct 2, 2015

@franciscocardoso gte 1000 lt 1100?
In any case, see the current issue about the problems with type validators.

@franciscocardoso
Copy link

@franciscocardoso franciscocardoso commented Oct 3, 2015

Hi,

@mickhansen, I believe you were trying to ping @franciscotfmc.

@mickhansen
Copy link
Contributor

@mickhansen mickhansen commented Oct 3, 2015

@franciscocardoso I was, sorry about that, the github auto complete is not entirely context aware apparently :)

@franciscotfmc
Copy link

@franciscotfmc franciscotfmc commented Oct 5, 2015

@mickhansen the case is that the query I'm about to execute comes from an input in the browser. The user can type pretty much anything and I need to search in a bunch of fields, including the integer type, checking if its value starts with '10'. The user can even type something like '10 abcd'.

@mickhansen
Copy link
Contributor

@mickhansen mickhansen commented Oct 5, 2015

@franciscocardoso So you just throw whatever input and try to LIKE match it against all fields?

@franciscotfmc
Copy link

@franciscotfmc franciscotfmc commented Oct 5, 2015

Basically that, @mickhansen. This is one of those screens with only one search input and a list of results. I don't think that using multiple inputs would be very friendly in my case, so that I could use gte and lt as you said. Picture the facebook search as an example. There you can type a profile name or a phone number. If something contains my typed query, it'll show up.

@franciscocardoso
Copy link

@franciscocardoso franciscocardoso commented Oct 5, 2015

Wrong handle again @mickhansen.

@mickhansen
Copy link
Contributor

@mickhansen mickhansen commented Oct 5, 2015

@franciscocardoso Oh god you must hate me by now.

@franciscocardoso
Copy link

@franciscocardoso franciscocardoso commented Oct 5, 2015

That's ok @mickhansen , no worries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet