Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

Option to disable iterator tests #347

Closed
wants to merge 3 commits into from
Closed

Conversation

piranna
Copy link

@piranna piranna commented Aug 16, 2019

See #345

@vweevers
Copy link
Member

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 (Level/community#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
Copy link
Member

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

I agree.

@piranna
Copy link
Author

piranna commented Aug 17, 2019

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
Copy link
Member

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
Copy link
Author

piranna commented Aug 17, 2019

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
Copy link
Member

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
Copy link
Author

piranna commented Aug 17, 2019

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
Copy link
Member

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
Copy link
Member

I hope you have sufficient info to move forward now.

@vweevers vweevers closed this Aug 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants