fix: Fix an issue where invalid VerifierOptions keys with Array values would cause a TypeError #432
Conversation
…s would cause a TypeError
|
I don't know how the ubuntu build ever worked, but I'll fix that in this PR too. |
|
|
||
| context('when given an unknown array argument', () => { | ||
| it('should return a Verifier object', () => { | ||
| expectSuccessWith({ |
There was a problem hiding this comment.
Should the user be aware that an invalid option was provided, and bail because of it?
otherwise they might feel that the argument is correctly being set, but it isn't
There was a problem hiding this comment.
I thought about adding a warning - but the existing behaviour (for unknown options that don't have an array value) was to silently ignore them.
I'm not sure if there are some unvalidated options - if we want to warn, we'll have to double check the list.
There was a problem hiding this comment.
hmmm, whilst that wouldn't be my preference, it is the way it is currently so I wouldn't want to break userspace too much.
looking at the users provided file, the only invalid option I think was tags which is now providerVersionTags and log option is no longer in v10 of pact-js post moving to the rust core.
There was a problem hiding this comment.
that is probably one for another PR tho!
| context('when given an unknown array argument', () => { | ||
| it('should return a Verifier object', () => { | ||
| expectSuccessWith({ | ||
| madeupArg: [''], |
There was a problem hiding this comment.
would js treat [''] as different to [] (because js)
and would that make a diff for this logic (rules || []).forEach((rule) => {
There was a problem hiding this comment.
Nope 👍
rules is from the rules object above - which is only accessed using the key. The value is only relevant for entering in the Array.isArray branch, which was missing the undefined check.
You can see the same (rules || []) pattern on the other branch - I'm not wild about having inline defaults, but I didn't want to change the code too much just to match personal preferences.
|
Thanks team! 🙏 We chose not to fail when incorrect arguments were passed because the previous implementation didn't either, and who knows what people would do. Now, in theory, the implementation did fail - just not with a warning, so we could potentially introduce that. I'm still inclined to leave it as a warn or silently ignore, because there are still plenty of those that would still upgrade from 9.x.x and then run into this behaviour. |
PR Template
This PR fixes an issue raised by Gustavo Souza on slack here, where an unknown key with an array value would cause the Verifier validation to fail with
cannot read property forEach of undefined.It also adds a test for this case.
The test also passes.