Feature/set multiple fields #409

Schwartz10 · 2019-04-29T20:16:41Z

Hello! A couple small things to note:

thoughts on what to return for setMultiple? I noticed the tests seem to return true if set completes without errors? We could do a similar thing for setMultiple, but it also might be nice to return the full, updated profile. https://github.com/3box/3box-js/compare/develop...openworklabs:feature/set-multiple-fields?expand=1#diff-efe243c83b3a509aaca8757b8cc08d35R32
I couldn't get all the tests to run successfully (something on my machine seemed to throw off one of the tests in), but all tests in the files I changed are passing.

oed

Thanks @Schwartz10, this is super cool!

1 - 2 are to your questions, 3 - 4 are additional thoughts.

I don't have a strong opinion on what the function should return. What do you mean by "the full update profile"? All entries in the store or just all entries that you added?
Yeah, we've had some issues with the tests recently. It should be fixed if you rebase on develop!
We would also need to add the setMultiple method to public and private of spaces. The spaces class is implemented a bit differently.
Kind of feel like the function should live in the KeyValueStore class with minimal wrappers in private and public store. This should be similar with spaces.

See also additional inline comments 🙂

src/__tests__/privateStore.test.js

src/publicStore.js

Schwartz10 · 2019-04-30T21:21:34Z

@oed I'm struggling to get the privateStore tests to pass at the moment. It's odd because the part it's failing at is here. I've verified that it's not a race condition.

Wondering if there is something else I have to adjust in the mocks? Could you provide a second pair of eyes on this one?

With respect to the setMultiple for public and private spaces, is here (public) and here (private) ok?

Schwartz10 · 2019-05-01T18:58:02Z

@oed i fixed the tests. But do let me know about where to write the functionality for setMultiple for public and private spaces (see comment above for my best guess)

oed

Looks great now 🙌

Yep that's exactly the place for setMultiple in spaces :)

Just a small nitpick below 👇

oed · 2019-05-02T07:45:45Z

src/privateStore.js

+  async setMultiple (keys, values) {
+    utils.throwIfNotEqualLenArrays(keys, values)
+    const dbKeys = keys.map(this._genDbKey, this)
+    const encryptedValues = values.map(this._encryptEntry, this)


Cool, didn't know you could pass this like that. I've always just used .bind

src/keyValueStore.js

Schwartz10 · 2019-05-02T18:09:02Z

Hey @oed ! Everything seems to be working well. Tests are passing, and running all these methods for both public/private spaces and public/private profile stores in a real application is working too. How should i proceed?

oed · 2019-05-02T18:18:45Z

Awesome @Schwartz10 I'll have another look at it first thing tomorrow. I assume it should be good, then It'll get merged to develop. Can probably make another release tomorrow as well, if it's urgent 🙂

oed

Looks good, just a few questions before I merge. Maybe you'd want to rebase on develop? Isn't strictly needed though :)

oed · 2019-05-03T08:31:32Z

src/keyValueStore.js

+        this._requireLoad()
+        this._ensureConnected()
+        const timeStamp = new Date().getTime()
+        return this._db.put(nextKey, { value: values[index], timeStamp })


Why not call this.set instead? Am I missing something?

Weird! thought I left a comment about this.

For some reason running the private store, calling this.set from within setMultiple did not save any database entries. It passes in the tests but does not work in my actual application. As far as i could tell, it didn't have to do with race conditions, the this context, or anything else I investigated. I was (and still am) super confused, especially because it worked fine for the public store. This commit solved the problem. Do you have any idea why that would be the case?

I just verified this again in my actual application - returning this.set from within setMultiple causes private fields to return null. SO weird. Would love to know why this is happening.

returning this._db.put from inside this.set doesn't solve it.

And old-school promisifying this.set doesn't do it either:

set (key, value) { return new Promise(async (resolve, reject) => { throwIfUndefined(key, 'key') this._requireLoad() this._ensureConnected() const timeStamp = new Date().getTime() await this._db.put(key, { value, timeStamp }) resolve(true) }) }

So this is only an issue with the privateStore and not the publicStore? That is indeed very strange!

Oh, I think it's because this in that context might be the subclass and not the superclass! I.e. it uses privateStore set which tries to encrypt the entry again. Then when you get the entry it tries to decrypt it once, when it's encrypted two times.

So yes, we should definitely not call this.set then. I think that's better though. We can move this._requireLoad() and this._ensureConnected() to be called only once in setMultiple 😃

oed · 2019-05-03T08:32:53Z

src/space.js

+          throwIfUndefined(nextKey, 'key')
+          return store.set(PREFIX + nextKey, values[index])
+        }, Promise.resolve())
+        return true


Why not do keys = keys.map(e => PREFIX + e) then call store.setMultiple?

Made this change. Also, just realizing, what happens if key is a number? Is it converted to a string somewhere?

👍 Here it will be converted to a string since we + with a string. However it's a good question if a number is passed into orbitdb as a key. I'm not completely sure about that.

src/space.js

Schwartz10 · 2019-05-03T17:38:12Z

Happy to rebase this, but want to wait just before we're ready to pull it in

oed

Yup, makes sense! And thanks for clarifying the this.set issue! Just one small change then we should be good to go!

oed · 2019-05-04T06:56:28Z

src/keyValueStore.js

+        this._requireLoad()
+        this._ensureConnected()
+        const timeStamp = new Date().getTime()
+        return this._db.put(nextKey, { value: values[index], timeStamp })


So this is only an issue with the privateStore and not the publicStore? That is indeed very strange!

oed · 2019-05-04T07:00:14Z

src/keyValueStore.js

+        this._requireLoad()
+        this._ensureConnected()
+        const timeStamp = new Date().getTime()
+        return this._db.put(nextKey, { value: values[index], timeStamp })


Oh, I think it's because this in that context might be the subclass and not the superclass! I.e. it uses privateStore set which tries to encrypt the entry again. Then when you get the entry it tries to decrypt it once, when it's encrypted two times.

oed · 2019-05-04T07:04:04Z

src/keyValueStore.js

+        this._requireLoad()
+        this._ensureConnected()
+        const timeStamp = new Date().getTime()
+        return this._db.put(nextKey, { value: values[index], timeStamp })


So yes, we should definitely not call this.set then. I think that's better though. We can move this._requireLoad() and this._ensureConnected() to be called only once in setMultiple 😃

oed · 2019-05-04T07:05:34Z

src/space.js

+          throwIfUndefined(nextKey, 'key')
+          return store.set(PREFIX + nextKey, values[index])
+        }, Promise.resolve())
+        return true


👍 Here it will be converted to a string since we + with a string. However it's a good question if a number is passed into orbitdb as a key. I'm not completely sure about that.

Schwartz10 · 2019-05-06T16:07:08Z

@oed all set! do you want me to squash this into 1 commit before I rebase?

oed · 2019-05-06T16:11:39Z

Sweet @Schwartz10 and yes that would be nice!

Schwartz10 · 2019-05-06T16:28:48Z

@oed all set!

oed reviewed Apr 30, 2019

View reviewed changes

src/__tests__/privateStore.test.js Show resolved Hide resolved

src/publicStore.js Show resolved Hide resolved

oed reviewed May 2, 2019

View reviewed changes

oed reviewed May 3, 2019

View reviewed changes

oed reviewed May 4, 2019

View reviewed changes

feat(api): add setMultiple

593b57b

Schwartz10 force-pushed the feature/set-multiple-fields branch from b1b2629 to 593b57b Compare May 6, 2019 16:28

oed approved these changes May 7, 2019

View reviewed changes

oed added this to the Sprint 19 milestone May 7, 2019

oed merged commit 837d633 into 3box:develop May 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/set multiple fields #409

Feature/set multiple fields #409

Schwartz10 commented Apr 29, 2019

oed left a comment

Schwartz10 commented Apr 30, 2019

Schwartz10 commented May 1, 2019

oed left a comment

oed May 2, 2019

Schwartz10 commented May 2, 2019

oed commented May 2, 2019

oed left a comment

oed May 3, 2019

Schwartz10 May 3, 2019 •

edited

Loading

Schwartz10 May 3, 2019

oed May 4, 2019

oed May 4, 2019

oed May 4, 2019

oed May 3, 2019

Schwartz10 May 3, 2019 •

edited

Loading

oed May 4, 2019

Schwartz10 commented May 3, 2019

oed left a comment

oed May 4, 2019

oed May 4, 2019

oed May 4, 2019

oed May 4, 2019

Schwartz10 commented May 6, 2019

oed commented May 6, 2019

Schwartz10 commented May 6, 2019

Feature/set multiple fields #409

Feature/set multiple fields #409

Conversation

Schwartz10 commented Apr 29, 2019

oed left a comment

Choose a reason for hiding this comment

Schwartz10 commented Apr 30, 2019

Schwartz10 commented May 1, 2019

oed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Schwartz10 commented May 2, 2019

oed commented May 2, 2019

oed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Schwartz10 May 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Schwartz10 May 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Schwartz10 commented May 3, 2019

oed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Schwartz10 commented May 6, 2019

oed commented May 6, 2019

Schwartz10 commented May 6, 2019

Schwartz10 May 3, 2019 •

edited

Loading

Schwartz10 May 3, 2019 •

edited

Loading