-
-
Notifications
You must be signed in to change notification settings - Fork 12
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 double prefixes #81
Conversation
In two situations: - When reopening a nested sublevel, because on open we add the parent's prefix to the child's prefix. Open twice, and you get a double prefix. Fixed by keeping around the original prefix. - When having more than 2 nested sublevels. This resulted in subdown wrapping subdown, effectively applying a double prefix to keys. Fixed by tweaking the unwrapping voodoo.
leveldown.js
Outdated
self.prefix = subdb.prefix + self.prefix | ||
self.leveldown = reachdown(subdb.db, matchdown, false) | ||
self.prefix = subdb.prefix + self.ownPrefix | ||
self.leveldown = subdb.leveldown || reachdown(subdb.db, matchdown, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only need 1 one of these solutions (either subdb.leveldown
or reachdown(..)
), not sure which I like best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna go with subdb.leveldown
because reachdown(sub.db)
is like taking a detour (it goes: own levelup -> encoding-down -> subleveldown -> external levelup -> encoding-down -> leveldown).
I've approved solely on the basis you've added a unit test for the case - I'm afraid I don't know the ramifications of the choice you outlined above; the best I can suggest/ask is whether one approach is better at handling "exotic" subdowns. i.e. |
@MeirionHughes Roger, thanks. We support those exotic situations since 4.1.3 and have a test for it :) |
In two situations:
Closes #78. This PR is an alternative to #79, which fixes more issues but is WIP, more invasive and semver-major, while this is semver-patch (with a caveat).