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

Invalidate array of objects with only a partial object value #77

Merged
merged 1 commit into from Jun 21, 2016

Conversation

@iamdoron
Copy link
Contributor

iamdoron commented Jun 20, 2016

Before this PR

Code.expect([{ a: 1, b: 1 }]).to.include({ a: 1 });

would not throw. After this PR, it throws.

It might be an issue with Hoek, though it is possible that the default value for part flag is true in Hoek.contain

@iamdoron iamdoron changed the title invalidate array with only a partial object value Invalidate array with only a partial object value Jun 20, 2016
@iamdoron iamdoron changed the title Invalidate array with only a partial object value Invalidate array of objects with only a partial object value Jun 20, 2016
@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Jun 20, 2016

This threw in code < 3.0.0. However, in 3.0.0, we added a breaking change to default to deep comparison. I think if Hoek.contain([{ a: 1, b: 1 }], { a: 1 }, { deep: true }) is working as intended with the deep flag, then we should leave this as is. cc: @nlf

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Jun 20, 2016

i agree, i think this works appropriately as it is

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Jun 20, 2016

OK. Closing then.

@cjihrig cjihrig closed this Jun 20, 2016
@cjihrig cjihrig added the non issue label Jun 20, 2016
@cjihrig cjihrig self-assigned this Jun 20, 2016
@iamdoron

This comment has been minimized.

Copy link
Contributor Author

iamdoron commented Jun 20, 2016

Maybe I don't understand. Why do you say it works as intended? I don't see how it is related to the deep flag.

There is the part flag. You either use it or don't - if you don't you expect it to be false.

what is the difference between

Code.expect([{ a: 1, b: 1 }]).to.include({ a: 1 });

and

Code.expect([{ a: 1, b: 1 }]).to.part.include({ a: 1 });

How would you say an array of objects 'fully' (not partially) includes an object?

what I would expect

Code.expect([{ a: 1, b: 1 }]).to.include({ a: 1 }); // throws
Code.expect([{ a: 1, b: 1 }, { c: 1, d: 1}]).to.include({ a: 1, b: 1}); // passes

Code.expect([{ a: 1, b: 1 }]).to.part.include({ b: 1}); // passes
Code.expect([{ a: 1, b: 1 }]).to.part.include({ d: 1}); // throws
@nlf

This comment has been minimized.

Copy link
Member

nlf commented Jun 20, 2016

Hoek.contains is recursive. What you're effectively asking is: does array [{ a: 1, b: 1 }] contain an object that contains { a: 1 }

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

iamdoron commented Jun 20, 2016

OK, but you cannot use shallow, and effectively

Code.expect([{a:1, b: 2}]).to.part.include({a: 1})
Code.expect([{a:1, b: 2}]).to.include({a: 1})

means the same thing

And even if that's the meaning in Hoek, it doesn't mean it should be the default in here - especially that you cannot (at least I haven't found a way) assert that an object exists in an array

Hoek.contain has part & deep flags. When part is false the behavior is as I expected. Don't you agree?

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Jun 20, 2016

if you want to validate that the whole object exists in the array, pass the whole object as the include() parameter.

Code.expect([{ a: 1, b: 2 }]).to.include({ a: 1, b: 2 });

works just fine. isn't that what you're trying to do?

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

iamdoron commented Jun 20, 2016

Hoek.contain([{ a: 1, b: 2 }, { d: 1 }], { b: 2, a: 1 }, { deep: true }) === true
Hoek.contain([{ a: 1, b: 2 }, { d: 1 }], { b: 2 }, { deep: true }) === true

Hoek.contain([{ a: 1, b: 2 }, { d: 1 }], { b:2 }, { deep: true, part: false }) === false
Hoek.contain([{ a: 1, b: 2 }, { d: 1 }], { b: 2, a: 1 }, { deep: true, part: false }) === true
@iamdoron

This comment has been minimized.

Copy link
Contributor Author

iamdoron commented Jun 20, 2016

it doesn't work as I expected because

Code.expect([{ a: 1, b: 2 }]).to.include({ a: 1}); // doesn't throw

(which is exactly the test I added in the PR)

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

iamdoron commented Jun 20, 2016

another issue that I think stems from this one:

Code.expect([{ a: 1, b: 2 }]).to.include({}) // doesn't throw
@nlf

This comment has been minimized.

Copy link
Member

nlf commented Jun 20, 2016

you're right in that part defaults to true in hoek, which we won't change there.

we could potentially add a flag to allow disabling partial matches, at the very least more clear documentation would be helpful.

what exactly are you trying to test?

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Jun 20, 2016

@nlf with it being recursive, shouldn't this pass:

Code.expect([{ a: 1, b: 2 }]).to.part.include({ a: 1, d: 4 });

Since this does:

Code.expect({ a: 1, b: 2 }).to.part.include({ a: 1, d: 4 });
@nlf

This comment has been minimized.

Copy link
Member

nlf commented Jun 20, 2016

"recursive" was me misspeaking, it's not actually recursive, the behavior is because part defaults to true when deep is true

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

iamdoron commented Jun 20, 2016

but here part=true is not the default, it is set to true only if you use the part in the expectation chain, so you cannot set it to false directly

I'm trying to test an object is within an array, specifically I'm testing mongodb indices, and I want to check an index is present in an array of indices.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Jun 20, 2016

It might make sense to do this then for includes():

this._flags.part = !!this._flags.part;
@cjihrig cjihrig reopened this Jun 20, 2016
@iamdoron

This comment has been minimized.

Copy link
Contributor Author

iamdoron commented Jun 20, 2016

@cjihrig that's what I initially did, but I thought it is a little bit more expressive this way

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Jun 20, 2016

Marking this as semver-major.

@cjihrig cjihrig added this to the 4.0.0 milestone Jun 21, 2016
@cjihrig cjihrig merged commit 88a5d24 into hapijs:master Jun 21, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Dec 1, 2016

Was there release note for this?

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Dec 5, 2016

I just did a normal release. Added the breaking changes label and added the 4.0.0 milestone.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Dec 5, 2016

Next time is would be better to at least give the full details in the top of the issue. I had to actually read this entire thread to figure out what changed. That wasn't great.

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

iamdoron commented Dec 5, 2016

You are right - I thought the test was good enough to describe it. I should have at least put an abbreviation of it in the description of the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.