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

Fix withRelated not always grouping properly for binary IDs #1918

Merged
merged 6 commits into from Dec 9, 2018

Conversation

Projects
2 participants
@6utt3rfly
Copy link
Contributor

6utt3rfly commented Nov 18, 2018

Introduction

When I was working with a larger dataset that uses binary IDs, I found that sometimes the 'withRelated' was returning incorrect results. The database queries were correct with retrieving the relationships, but they were being incorrectly assigned to the primary collection. In the end, I found that if I changed the grouping to convert Buffer types to strings, then all of my relationships were set correctly.

Motivation

This should be a minor, non-breaking change that only changes local behavior to the relationships eagerPair method.

Proposed solution

When grouping relationships and building the collection map, convert Buffer IDs to string keys

Current PR Issues

no known issues with making this change...

Alternatives considered

I debated using a local parse method in my model, but thought that this would fix it more at the source rather than requiring me to add a parse method to all of my models.

Fix withRelated not always grouping properly for binary IDs
- When grouped relations, convert Buffers to hex strings to use as the mapping key
@ricardograca
Copy link
Member

ricardograca left a comment

This looks simple enough but it needs tests to verify the correct behavior of the fix, and to ensure that this bug doesn't get introduced again in the future.

@6utt3rfly

This comment has been minimized.

Copy link
Contributor

6utt3rfly commented Nov 19, 2018

I actually wrote a unit test, but found that the problem was bigger than I initially thought... it basically boils down to code something like this:

a[new Buffer('93', 'hex')] = ['123'];
a[new Buffer('90', 'hex')] = ['456'];
console.log(a, Object.keys(a).length);

There are several places in the code where the ID is assumed to be used as a unique object key. However, with Buffers, this doesn't always work because they get converted to a string representation which may have collisions.

I now found that the collection _byId property also does this, so when I query and get back 2 distinct rows, it's treating it as a single result.

I could go further and update this PR, but I thought I'd ask if you have any other thoughts first @ricardograca ?

@ricardograca

This comment has been minimized.

Copy link
Member

ricardograca commented Nov 19, 2018

I don't even understand how/why Buffers can be used as keys, so the fact that it more or less works at all is new to me.

You can do whatever you need to make your use case work as long as that doesn't break any of the existing functionality and tests are included. If the extra fixes you mention are not immediately being required to you it's OK if you don't fix them.

6utt3rfly added some commits Nov 19, 2018

- Fixed collections not being created properly with binary IDs due to…
… collection keying again

- Added a collection test with binary IDs
Merge remote-tracking branch 'origin/fix/related-binary-ids' into fix…
…/related-binary-ids

# Conflicts:
#	test/integration/relations.js
@6utt3rfly

This comment has been minimized.

Copy link
Contributor

6utt3rfly commented Nov 19, 2018

OK - I've added tests and fixes to collections. If you need more details or changes I'm happy to do so @ricardograca . Thanks for your quick responses!!

@ricardograca
Copy link
Member

ricardograca left a comment

Just needs to address the two small issues I mentioned, and then I'll merge. I'm still waiting for another big PR to be merged, so this one will probably only go after that one: #1848.

Show resolved Hide resolved test/integration/relations.js Outdated
- Remove 'Issue #xx' from unit test comment
- Remove 'debug:true' from unit test
@6utt3rfly

This comment has been minimized.

Copy link
Contributor

6utt3rfly commented Nov 22, 2018

Changes made - thanks for catching the debug:true which I forgot about! @ricardograca

@ricardograca
Copy link
Member

ricardograca left a comment

Will merge after #1848 is merged, since I already spent a lot of time fixing merge conflicts in that one and if there are any in this afterwards they will certainly be easier to fix.

@ricardograca ricardograca added this to To Do in Version 0.14.0 via automation Nov 22, 2018

@ricardograca ricardograca moved this from To Do to In progress in Version 0.14.0 Nov 22, 2018

@6utt3rfly 6utt3rfly referenced this pull request Nov 26, 2018

Merged

Fix/related binary ids #2

@ricardograca ricardograca merged commit e4c51ee into bookshelf:master Dec 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Version 0.14.0 automation moved this from In progress to Done Dec 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment