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

Only call super on validations class if it exists via shouldCallSuper #170

Merged
merged 3 commits into from
Apr 16, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions addon/utils/should-call-super.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* Copyright 2016, Yahoo! Inc.
* Copyrights licensed under the New BSD License. See the accompanying LICENSE file for terms.
*/

/**
* Checks if the give key exists on the object's super.
* If so, we can successfuly call the obj[key] _super
*
* Created by @rwjblue
*/
export default function shouldCallSuper(obj, key) {
let current = Object.getPrototypeOf(obj);
current = Object.getPrototypeOf(current);

while (current) {
let descriptor = Object.getOwnPropertyDescriptor(current, key);

if (descriptor) {
return true;
}

current = Object.getPrototypeOf(current);
}

return false;
}
5 changes: 5 additions & 0 deletions addon/utils/utils.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* Copyright 2016, Yahoo! Inc.
* Copyrights licensed under the New BSD License. See the accompanying LICENSE file for terms.
*/

export function hasEmberData() {
return typeof self.DS !== 'undefined';
}
30 changes: 25 additions & 5 deletions addon/validations/factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import ValidationResult from './result';
import ValidationResultCollection from './result-collection';
import BaseValidator from '../validators/base';
import cycleBreaker from '../utils/cycle-breaker';
import shouldCallSuper from '../utils/should-call-super';

const {
get,
Expand Down Expand Up @@ -98,14 +99,27 @@ function buildValidations(validations = {}, globalOptions = {}) {
let Validations;

return Ember.Mixin.create({
_validationsClass: computed(function () {
init() {
this._super(...arguments);

// Count number of mixins to bypass super check if there is more than 1
this.__validationsMixinCount__ = this.__validationsMixinCount__|| 0;
this.__validationsMixinCount__++;
Copy link
Member

@rwjblue rwjblue Apr 16, 2016

Choose a reason for hiding this comment

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

Small tweak here, we should store the resulting integer in a closure scoped variable:

let Validations, validationMixinIndex;
// ...snip...

validationMixinIndex = this.__validationsMixinCount__++;

Then below, the guard for calling super in __validationsClass__ would look like:

if(shouldCallSuper(this, '__validationsClass__') || validationMixinIndex > 1) {
  inheritedClass = this._super();
}

This ensures that the first mixin with a __validationsClass__ only calls _super if the superclass passes the shouldCallSuper check, and the second (and any more) validation mixins always call _super.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 Added the fix

},
__validationsClass__: computed(function () {
if (!Validations) {
Validations = createValidationsClass(this._super(), validations);
let inheritedClass;

if(shouldCallSuper(this, '__validationsClass__') || this.__validationsMixinCount__ > 1) {
inheritedClass = this._super();
}

Validations = createValidationsClass(inheritedClass, validations);
}
return Validations;
}).readOnly(),
validations: computed(function () {
return this.get('_validationsClass').create({
return this.get('__validationsClass__').create({
model: this
});
}).readOnly(),
Expand Down Expand Up @@ -181,7 +195,7 @@ function createValidationsClass(inheritedValidationsClass, validations = {}) {
let validatableAttributes = Object.keys(validations);

// Setup validation inheritance
if (inheritedValidationsClass) {
if (inheritedValidationsClass && inheritedValidationsClass.__isCPValidationsClass__) {
const inheritedValidations = inheritedValidationsClass.create();

validationRules = merge(validationRules, inheritedValidations.get('_validationRules'));
Expand Down Expand Up @@ -235,7 +249,7 @@ function createValidationsClass(inheritedValidationsClass, validations = {}) {
});

// Create `validations` class
return Ember.Object.extend(TopLevelProps, {
const ValidationsClass = Ember.Object.extend(TopLevelProps, {
model: null,
attrs: null,
isValidations: true,
Expand Down Expand Up @@ -286,6 +300,12 @@ function createValidationsClass(inheritedValidationsClass, validations = {}) {
});
}
});

ValidationsClass.reopenClass({
__isCPValidationsClass__: true
});

return ValidationsClass;
}

/**
Expand Down
67 changes: 67 additions & 0 deletions tests/integration/validations/factory-general-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -650,3 +650,70 @@ test("nested keys - inheritance", function(assert) {
});


test("call super in validations class with no super property", function(assert) {
// see https://github.com/offirgolan/ember-cp-validations/issues/149
assert.expect(1);

var Validations = buildValidations({
'firstName': validator(Validators.presence)
});

var mixin = Ember.Mixin.create({
actions: {
foo() {
// The issue is that __validationsClass__ is calling super but since
// this is no __validationsClass__ method on the super, it is wrapped
// with the closest context which is the 'foo' action
assert.ok(false); // should not reach here
}
}
});

var component = setupObject(this, Ember.Component.extend(Validations, mixin, {
actions: {
foo() {
assert.ok(true);
const validations = this.get('validations');
}
}
}));

component.send('foo');
});


test("multiple mixins", function(assert) {
var Validations1 = buildValidations({
'firstName': validator(Validators.presence)
});

var Validations2 = buildValidations({
'middleName': validator(Validators.presence)
});

var Validations3 = buildValidations({
'lastName': validator(Validators.presence)
});

var object = setupObject(this, Ember.Object.extend(Validations1, Validations2, Validations3));

assert.deepEqual(object.get('validations.validatableAttributes').sort(), ['firstName', 'middleName', 'lastName'].sort());
assert.equal(object.get('validations.attrs.firstName.isValid'), false);
assert.equal(object.get('validations.attrs.lastName.isValid'), false);
assert.equal(object.get('validations.isValid'), false);

object.set('firstName', 'Offir');

assert.equal(object.get('validations.attrs.firstName.isValid'), true);
assert.equal(object.get('validations.isValid'), false);

object.set('middleName', 'David');

assert.equal(object.get('validations.attrs.middleName.isValid'), true);
assert.equal(object.get('validations.isValid'), false);

object.set('lastName', 'Golan');

assert.equal(object.get('validations.attrs.lastName.isValid'), true);
assert.equal(object.get('validations.isValid'), true);
});
37 changes: 37 additions & 0 deletions tests/unit/utils/should-call-super-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import Ember from 'ember';
import { module, test } from 'qunit';
import shouldCallSuper from 'ember-cp-validations/utils/should-call-super';

const {
computed
} = Ember;

module('Unit | Utils | shouldCallSuper');

test('shouldCallSuper - true', function(assert) {
const Parent = Ember.Object.extend({
foo: computed(function() {})
});

const Child = Parent.extend({
foo: computed(function() {
assert.ok(shouldCallSuper(this, 'foo'));
})
});

const child = Child.create();
child.get('foo');
});

test('shouldCallSuper - false', function(assert) {
const Parent = Ember.Object.extend();

const Child = Parent.extend({
foo: computed(function() {
assert.ok(!shouldCallSuper(this, 'foo'));
})
});

const child = Child.create();
child.get('foo');
});