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.

Sign up for GitHub

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

objectSpec sub-spec on arrays fix; themeSpec fix #11133

Merged
merged 5 commits into from
Aug 25, 2023
Merged

objectSpec sub-spec on arrays fix; themeSpec fix #11133

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e051578
objectSpecs handle validating all children of an Array w/ defined spec
nucleogenesis Aug 8, 2023
92d4bff
make themeSpec pass objectSpecs
nucleogenesis Aug 22, 2023
840daae
fix tests in objectSpecs
nucleogenesis Aug 14, 2023
3bbc59e
clarify logic w/ comment
nucleogenesis Aug 24, 2023
684ca27
use isPlainObject instead of isObject <3 @bjester
nucleogenesis Aug 24, 2023
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
14 changes: 11 additions & 3 deletions kolibri/core/assets/src/objectSpecs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,18 @@ function _validateObjectData(data, options, dataKey) {

// object sub-spec
if (hasData && options.spec) {
if (!isObject(data)) {
return _fail('Only objects can have sub-specs', dataKey, data);
if (!isObject(data) && !isArray(data)) {
return _fail('Only objects or arrays can have sub-specs', dataKey, data);
}
if (!validateObject(data, options.spec)) {
// If it is an array, we will validate each item in the array
if (isArray(data)) {
for (const item of data) {
if (!validateObject(item, options.spec.spec)) {
return _fail('Object in Array sub-spec failed', dataKey, item);
}
}
}
if (isObject(data) && !isArray(data) && !validateObject(data, options.spec)) {
Copy link
Member

Choose a reason for hiding this comment

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

There are some unreachable conditions here, considering the first if ensuring data is only an object or an array. Making this an else if would remove !isArray(data), but also, would isObject and isArray ever be true at the same time?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because isObject is super general and comes back true for an Array (and functions) - so isObject && !isArray needs the !isArray check there because above that bit I'm checking for isArray and handling it when true but don't return so the last step is saying "this is an object that isn't an array" -- which probably should also check "is also not a function".

Although - now that I look at it again, it'd probably be simpler to just return after succeeding within the block where isArray is true and passes validation. Will push an update

Copy link
Member

Choose a reason for hiding this comment

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

Since this is just for the object spec, perhaps isPlainObject is better? https://lodash.com/docs/4.17.15#isPlainObject

Copy link
Member Author

Choose a reason for hiding this comment

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

Bah - nevermind I misread the GH diff - it can't return early.

I added a comment to clarify a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome thanks for that - all uses of isObject were really trying to ask isPlainObject so I replaced them throughout.

return _fail('Validator sub-spec failed', dataKey, data);
}
}
Expand Down
2 changes: 1 addition & 1 deletion kolibri/core/assets/src/styles/themeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ export default {
},
logos: {
type: Array,
default: [],
default: () => [],
spec: _imageSpec,
},
};
40 changes: 39 additions & 1 deletion kolibri/core/assets/test/objectSpecs.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,26 @@ const simpleSpec = {
};
},
},
arrayOfNum: {
type: Array,
default: () => [],
spec: {
type: Number,
required: true,
},
},
arrayOfObj: {
type: Array,
default: () => [],
spec: {
type: Object,
default: null,
spec: {
prop_C: { type: String, default: 'val_C' },
prop_D: { type: String, default: 'val_D' },
},
},
},
};

describe('validateObject basic operation', () => {
Expand Down Expand Up @@ -99,6 +119,14 @@ describe('validateObject basic operation', () => {
};
expect(validateObject(obj, simpleSpec)).toBe(true);
});
test('validateObject should test all children of an Array that has a spec', () => {
const obj = {
str1: 'A',
arrayOfNum: [1, 2, 3, 4, 5],
arrayOfObj: [{ prop_C: 'val_C', prop_D: 'val_D' }],
};
expect(validateObject(obj, simpleSpec)).toBe(true);
});
});

describe('validateObject rejects bad specs', () => {
Expand Down Expand Up @@ -126,7 +154,7 @@ describe('validateObject rejects bad specs', () => {
};
expect(validateObject(obj, badSpec)).toBe(false);
});
test('non-object cannot have sub-spec', () => {
test('non-object/array cannot have sub-spec', () => {
const badSpec = {
str1: {
type: String,
Expand Down Expand Up @@ -156,6 +184,16 @@ describe('validateObject rejects bad specs', () => {
};
expect(validateObject(obj, badSpec)).toBe(false);
});
test('A specced array rejects an array even if any value is bad', () => {
const badNumArrayObj = {
arrayOfNum: [1, 2, 3, 4, 5, 'A'],
};
expect(validateObject(badNumArrayObj, simpleSpec)).toBe(false);
const badObjArrayObj = {
arrayOfObj: [{ foo: 'bar', prop_c: 'val_c' }],
};
expect(validateObject(badObjArrayObj, simpleSpec)).toBe(false);
});
});

describe('objectWithDefaults fills defaults', () => {
Expand Down