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

Gracefully handle bad mergeCollection #272

Merged
merged 2 commits into from
Jul 12, 2023
Merged

Gracefully handle bad mergeCollection #272

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
69ec6e9
gracefully handle bad mergeCollection
luacmartins Jul 12, 2023
ad1a66d
fix tests
luacmartins Jul 12, 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
10 changes: 8 additions & 2 deletions lib/Onyx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1129,14 +1129,20 @@ function mergeCollection(collectionKey, collection) {
}

// Confirm all the collection keys belong to the same parent
let hasCollectionKeyCheckFailed = false;
_.each(collection, (_data, dataKey) => {
if (isKeyMatch(collectionKey, dataKey)) {
return;
}

throw new Error(`Provided collection doesn't have all its data belonging to the same parent. CollectionKey: ${collectionKey}, DataKey: ${dataKey}`);
hasCollectionKeyCheckFailed = true;
Logger.logAlert(`Provided collection doesn't have all its data belonging to the same parent. CollectionKey: ${collectionKey}, DataKey: ${dataKey}`);
Copy link
Contributor

@marcaaron marcaaron Jul 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We could keep throwing if you are on dev? In my opinion, this should throw. It means someone is not testing their API changes against the app. The fact that they are getting to staging or production is a problem. We are kind of making this more likely with this PR since an alert is easier to ignore than a throw.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we should be fine with this for now, the typescript migration should start pretty soon and that should catch any of these malformed data updates.

Also just couple lines above when the data is "more" malformed, we just resolve the promise and log info which is same as here so thats what makes me think it should be fine

Logger.logInfo('mergeCollection() called with invalid or empty value. Skipping this update.');

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's not a huge deal. But I am not following how the TS migration would prevent this. Updates from Web-Expensify get passed to Onyx.update() more or less blindly. It's gonna be a run time error that can't be prevented during compile time because the API could send the wrong type or even just a 🥔 emoji and Onyx would try to do something with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

To reiterate the main concern here: we are making these errors harder to catch on dev. We will find out about this as a staging deploy blocker, but it would be better to create no deploy blocker in the first place and catch it on dev.

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 agree that we can throw in dev. Created a PR #277

});

// Gracefully handle bad mergeCollection updates so it doesn't block the merge queue
if (hasCollectionKeyCheckFailed) {
return Promise.resolve();
}

return getAllKeys()
.then((persistedKeys) => {
// Split to keys that exist in storage and keys that don't
Expand Down
18 changes: 12 additions & 6 deletions tests/unit/onyxTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,12 +306,18 @@ describe('Onyx', () => {
});
});

it('should throw error when a key not belonging to collection key is present in mergeCollection', () => {
try {
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_1: {ID: 123}, notMyTest: {beep: 'boop'}});
} catch (error) {
expect(error.message).toEqual(`Provided collection doesn't have all its data belonging to the same parent. CollectionKey: ${ONYX_KEYS.COLLECTION.TEST_KEY}, DataKey: notMyTest`);
}
it('should skip the update when a key not belonging to collection key is present in mergeCollection', () => {
const valuesReceived = {};
connectionID = Onyx.connect({
key: ONYX_KEYS.COLLECTION.TEST_KEY,
initWithStoredValues: false,
callback: (data, key) => valuesReceived[key] = data,
});

return Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_1: {ID: 123}, notMyTest: {beep: 'boop'}})
.then(() => {
expect(valuesReceived).toEqual({});
});
});

it('should return full object to callback when calling mergeCollection()', () => {
Expand Down