Level / abstract-leveldown

An abstract prototype matching the leveldown API.
MIT License
146 stars 53 forks source link

Option to disable `iterator` tests #347

Closed piranna closed 5 years ago

piranna commented 5 years ago

See #345

vweevers commented 5 years ago

I'm -1 on this, because IMO iterators and read streams are a fundamental part of Level. Supporting other use cases can be interesting and the Level org has always been open to experimentation. However, straying too far from the core API means going against user expectations.

Although runtime manifests (https://github.com/Level/community/issues/42) could in the future describe the capabilities of an abstract-leveldown implementation, I think they should only describe additional features, and not allow core features to be disabled.

@ralphtheninja thoughts?

ralphtheninja commented 5 years ago

I'm -1 on this, because IMO iterators and read streams are a fundamental part of Level.

I agree.

piranna commented 5 years ago

As explained in the other issue thread, my AbstractLevel hide keys by using hashes and values by using per-entry encryption keys, so it doesn't make sense to have iterators of entries that you can't know what's their key nor access to its value... Other than this, it's fully compliant. I know this is a corner case usage of the AbstractLevel standar API, but don't know of any other way to make it compliant beyond making iterators optional or decrease the security level of my module... :-/ Any idea?

I understand the reasons why iterators could be considered a basic feature of level stores, but from an usage perspective, it's a level higher of what a basic key-value store can do...

vweevers commented 5 years ago

don't know of any other way to make it compliant beyond making iterators optional or decrease the security level of my module... :-/ Any idea?

I'm afraid the conclusion is that it can't be compliant. I am wondering though why you picked abstract-leveldown as your interface? If not to integrate with the Level ecosystem.

piranna commented 5 years ago

am wondering though why you picked abstract-leveldown as your interface? If not to integrate with the Level ecosystem.

Yeah! That's exactly the reason. At first I was thinking about using directly level module, but seeing I was able to do it with minor changes in a more standard, compatible and reusable way, I decided to move that way and make it integrable. This way I could focus on the encryption part and later be able to change both the backend database or access it from a REST API.

vweevers commented 5 years ago

I'm not sure how much value there is to integrating with the Level ecosystem without iterators. I suggest to clearly document that limitation of your API.

As for the test suite, you could just copy https://github.com/Level/abstract-leveldown/blob/master/test/index.js and take out the iterator tests. Do note that we assume that only abstract-leveldown/test is directly required by dependents; we may move internal files around without (semver) notice.

piranna commented 5 years ago

I suggest to clearly document that limitation of your API.

That's was part of my plan, at least for short-mid term.

As for the test suite, you could just copy /test/index.js@master and take out the iterator tests. Do note that we assume that only abstract-leveldown/test is directly required by dependents; we may move internal files around without (semver) notice.

I'm inheriting from the AbstractLevel class, too. Only missing part from the API... the iterators :-P

vweevers commented 5 years ago

I'm inheriting from the AbstractLevel class, too

Right. To clarify, I was only talking about the test/ directory. You can require('abstract-leveldown/test') for the whole suite or require('abstract-leveldown/test/batch-test') etc for individual tests, but the latter may be moved.

vweevers commented 5 years ago

I hope you have sufficient info to move forward now.