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

Jump to bottom

fix: Fix an issue where invalid VerifierOptions keys with Array values would cause a TypeError #432

Merged
merged 2 commits into from
Mar 8, 2023
Merged

fix: Fix an issue where invalid VerifierOptions keys with Array values would cause a TypeError #432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ecc29f
fix: Fix an issue where invalid VerifierOptions keys with Array value…
TimothyJones Mar 8, 2023
e40b985
ci: Move the setting of msvs version to after the node setup
TimothyJones Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/build-and-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ jobs:

steps:
- uses: actions/checkout@v2
- name: Set MSVS version
run: npm config set msvs_version 2017
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v1
with:
node-version: ${{ matrix.node-version }}
- name: Set MSVS version
run: npm config set msvs_version 2017
- run: script/ci/build-and-test.sh
env:
NODE_VERSION: ${{ matrix.node-version }}
Expand Down
12 changes: 12 additions & 0 deletions src/verifier/validateOptions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,18 @@ describe('Verifier argument validator', () => {
});
});
});

context('when given an unknown array argument', () => {
it('should return a Verifier object', () => {
expectSuccessWith({
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

that is probably one for another PR tho!

madeupArg: [''],
Copy link
Member

Choose a reason for hiding this comment

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

would js treat [''] as different to [] (because js)

and would that make a diff for this logic (rules || []).forEach((rule) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

providerBaseUrl: 'http://localhost',
pactBrokerUrl: 'http://foo.com',
pactUrls: ['http://idontexist'],
provider: 'someprovider',
} as VerifierOptions);
});
});
});

context('when not given --pact-urls or --provider-base-url', () => {
Expand Down
2 changes: 1 addition & 1 deletion src/verifier/validateOptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ export const validateOptions = (options: VerifierOptions): VerifierOptions => {
// get type of parameter (if an array, we apply the rule to each item of the array instead)
if (Array.isArray(options[k])) {
options[k].forEach((item: unknown) => {
rules.forEach((rule) => {
(rules || []).forEach((rule) => {
// rule(item) // If the messages aren't clear, we can do this
rule(options)(item, k);
});
Expand Down