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.

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

Merged
merged 2 commits into from
Oct 8, 2019
Merged

Fix double prefixes #81

merged 2 commits into from
Oct 8, 2019

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Oct 8, 2019

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.

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).

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.
@vweevers vweevers added the semver-patch Bug fixes that are backward compatible label Oct 8, 2019
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)
Copy link
Member Author

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.

Copy link
Member Author

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).

@MeirionHughes
Copy link
Member

MeirionHughes commented Oct 8, 2019

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. sub(encryptdown(sub(db, "foo")), "bar") etc...

@vweevers
Copy link
Member Author

vweevers commented Oct 8, 2019

@MeirionHughes Roger, thanks. We support those exotic situations since 4.1.3 and have a test for it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Bug fixes that are backward compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Applies prefix twice on nested sublevel
3 participants