INT-3404 - Add tests to validate behavior of jobState.hasKey
#694
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
austinkelleher
force-pushed
the
INT-3404-test
branch
from
a976e12
to
06338cf
Compare
The Qualys integration was using `await`ing the result of `jobState.hasKey` and firing off multiple promises with [`pQueue`](https://github.com/sindresorhus/p-queue). The Qualys integration was occasionally seeing `DUPLICATE_KEY_DETECTED` errors being thrown despite checking whether the graph object `_key` exists before adding the new entity to the `jobState`. These tests were added as part of the investigation into the issue. The Qualys integration fix can be found here: JupiterOne-Archives/graph-qualys#190
austinkelleher
force-pushed
the
INT-3404-test
branch
from
06338cf
to
ec315eb
Compare
The Qualys integration was using `await`ing the result of `jobState.hasKey` and firing off multiple promises with [`pQueue`](https://github.com/sindresorhus/p-queue). The Qualys integration was occasionally seeing `DUPLICATE_KEY_DETECTED` errors being thrown despite checking whether the graph object `_key` exists before adding the new entity to the `jobState`. These tests were added as part of the investigation into the issue. The Qualys integration fix can be found here: JupiterOne-Archives/graph-qualys#190
austinkelleher
force-pushed
the
INT-3404-test
branch
from
ec315eb
to
2fea84f
Compare
ceelias
reviewed
Apr 21, 2022
packages/integration-sdk-runtime/src/execution/__tests__/jobState.test.ts
Outdated
Show resolved
Hide resolved
ceelias
previously approved these changes
Apr 21, 2022
aiwilliams
reviewed
Apr 21, 2022
| // the `hasKey` promise while handling multiple entities concurrently, | ||
| // could result in a `hasKey` returning `false` because neither of the | ||
| // duplicate entities have been fully added to the job state yet. | ||
| if (jobState.hasKey(e._key)) return; |
There was a problem hiding this comment.
It seems to me that the documentation here
should change to provide reasons for why both sync and async forms are present, and when to use which.There was a problem hiding this comment.
Agreed. Can tackle separately. Will open ticket. Thanks for suggestion.
ceelias
approved these changes
Apr 21, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Qualys integration was using
awaiting the result ofjobState.hasKeyand firing off multiple promises with
pQueue.The Qualys integration was occasionally seeing
DUPLICATE_KEY_DETECTEDerrors being thrown despite checking whether the graph object
_keyexists before adding the new entity to the
jobState. These tests wereadded as part of the investigation into the issue.
The Qualys integration fix can be found here: JupiterOne-Archives/graph-qualys#190