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

added onlyIfChanged opt #98

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ Make a new Hyperbee instance. `core` should be a [Hypercore](https://github.com/
{
keyEncoding: 'binary', // "binary" (default), "utf-8", "ascii", "json", or an abstract-encoding
valueEncoding: 'binary' // Same options as keyEncoding like "json", etc
onlyIfChanged: true // put a value only if it means a change in the db
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative name: changeOnly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or casChanges

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the boolean option, but just maybe a different API could be a cas option in the constructor, so user can pass its global comparator

Copy link
Contributor

@LuKks LuKks May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should say false because that's the actual default

}
```

Expand Down
4 changes: 3 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
const sameData = require('same-data')
const codecs = require('codecs')
const { Readable } = require('streamx')
const mutexify = require('mutexify/promise')
Expand Down Expand Up @@ -287,6 +288,7 @@ class Hyperbee extends ReadyResource {
this.sep = opts.sep || SEP
this.readonly = !!opts.readonly
this.prefix = opts.prefix || null
this.onlyIfChanged = !!opts.onlyIfChanged

this._unprefixedKeyEncoding = this.keyEncoding
this._sub = !!this.prefix
Expand Down Expand Up @@ -669,7 +671,7 @@ class Batch {
async put (key, value, opts) {
const release = this.batchLock ? await this.batchLock() : null

const cas = (opts && opts.cas) || null
const cas = (opts && opts.cas) || (this.tree.onlyIfChanged ? sameData : null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a tricky one in terms of API. What's the intended behaviour if a user specified onlyIfChanged: true, but also passes an explicit cas?

ATM it will ignore onlyIfChanged if the cas is passed. The alternative would be to run both the cas and the onlyIfChanged, and to proceed only if both pass. Not saying that is better, but it's important to explicitly decide what the behaviour should be.

Also in terms of API: I would also allow passing onlyIfChanged as an option to put and del

Copy link
Contributor Author

@rafapaezbas rafapaezbas May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the intended behavior is that cas overwrites onlyIfChanged.

I think OnlyIfChanged option for put sounds like a good idea, for del probably it doesnt make sense since the default behavior is the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API is ok because method option overrides constructor option, just like with Hypercore timeout options, etc

const encoding = this._getEncoding(opts)

if (!this.locked) await this.lock()
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"protocol-buffers-encodings": "^1.2.0",
"ready-resource": "^1.0.0",
"safety-catch": "^1.0.2",
"same-data": "^1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case would same-data not be enough? There is also same-object (what brittle uses)

Just asking because people might use the option to later realize that there are duplicated inserts due same-data not supporting the case?

"streamx": "^2.12.4"
},
"devDependencies": {
Expand Down
37 changes: 36 additions & 1 deletion test/basic.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const test = require('brittle')
const b4a = require('b4a')
const { create, collect, createCore } = require('./helpers')

const Hyperbee = require('..')

test('out of bounds iterator', async function (t) {
Expand Down Expand Up @@ -486,3 +485,39 @@ test('supports encodings in snapshot', async function (t) {
t.alike(await snap1.get('hi'), { seq: 1, key: b4a.from('hi'), value: 'there' })
t.alike(await snap2.get('hi'), { seq: 1, key: 'hi', value: b4a.from('there') })
})

test('onlyIfChanged put', async function (t) {
{
const db = create({ onlyIfChanged: true })
await db.ready()
await db.put('key', 'value')
t.is(db.feed.length, 2)
await db.put('key', 'value')
t.is(db.feed.length, 2)
}
{
const db = create({ onlyIfChanged: true, valueEncoding: 'binary' })
await db.ready()
await db.put('key', Buffer.from('buffer'))
t.is(db.feed.length, 2)
await db.put('key', Buffer.from('buffer'))
t.is(db.feed.length, 2)
}
{
const db = create({ onlyIfChanged: true, valueEncoding: 'json' })
await db.ready()
await db.put('key', { a: 1 })
t.is(db.feed.length, 2)
await db.put('key', { a: 1 })
t.is(db.feed.length, 2)
}
})

test('onlyIfChanged put after del', async function (t) {
const db = create({ onlyIfChanged: true })
await db.ready()
await db.put('key', 'value')
await db.del('key')
await db.put('key', 'value')
t.is(db.feed.length, 4)
})