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

adds optional meta field to manifest #681

Merged
merged 3 commits into from Sep 27, 2019
Merged

adds optional meta field to manifest #681

merged 3 commits into from Sep 27, 2019

Conversation

@tabcat
Copy link
Member

@tabcat tabcat commented Sep 10, 2019

This PR would give users the ability to add additional data to the db manifest. This could be useful when wanting to package information with the db that should not change during the lifetime of the db. An example could be adding a public ECDH key that the owner of the db controls.

The meta field would only exist in the manifest if options.meta !== undefined is true when creating the database. This keeps db addresses the same as before when creating a db without a meta field. When creating a db with a meta field, make options.meta equal to a JSON serialize-able value so it can safely traverse to/from ipfs. This value with be readable in the db instance property db.options.meta (if the manifest does not contain a meta field db.options.meta is undefined).

The tests added will check when creating a db that 1) the meta field is only added to the manifest if options.meta is defined 2) if there is no meta field in the manifest db.options.meta is undefined 3) if there is a meta field when options.meta is defined and options.meta is equal to db.options.meta

src/db-manifest.js Show resolved Hide resolved
@haadcode
Copy link
Member

@haadcode haadcode commented Sep 11, 2019

Thank you @tabcat for the PR! 🙏 This would indeed be a good addition and has been requested in the past by some users.

To understand the approach a little better, can you elaborate on the rationale for separating defaults and meta (as per tabcat@da3ec81#diff-f1c4721cce5f33ff9a226802788fe12aR384)? Are those not the same? Could they be the same, as in just have the meta field?

@tabcat
Copy link
Member Author

@tabcat tabcat commented Sep 11, 2019

@haadcode They could indeed yes. I designed the meta field to be totally controlled by the user but it could encapsulate defaults and some field for the user by changing the code a bit. Options.defaults and options.meta could stay the same but be set up in the manifest differently (eg. meta: { defaults, meta }). both should still probably both be added optionally.

@aphelionz
Copy link
Member

@aphelionz aphelionz commented Sep 11, 2019

I can see the distinction also being at the "OrbitDB layer" vs the "application layer". If you're just issuing a naked orbitdb.open call (like on a pinning service or something) you don't need to care about what's in meta. People are gonna do whatever they want anyway, I imagine, but it's still a worthy consideraton imo

@tabcat
Copy link
Member Author

@tabcat tabcat commented Sep 11, 2019

Yeah I think I like separating them more but not sure there is really a difference anywhere but the manifest.

@RichardLitt
Copy link
Member

@RichardLitt RichardLitt commented Sep 11, 2019

I like the idea. Stupid question: What's to stop me from adding a meta field to the data, without adding all of these checks? Can't I just add my own fields to any JSON object without having an issue?

@tabcat
Copy link
Member Author

@tabcat tabcat commented Sep 11, 2019

if by checks you are referring to the checking if meta is defined in options before adding the meta field to the manifest, the goal of the check is to enable backwards compatible and adding a new field everywhere even if its not used would break that property. It would work just fine but anyone using orbitdb without this feature would have different addresses with the same config because they would not have the meta field in their manifest.

@tabcat
Copy link
Member Author

@tabcat tabcat commented Sep 11, 2019

if orbitdb ever needs to add something that breaks the manifest address anyway these checks can be removed, and the meta field could be added by default with it, but i didnt see a reason to make a breaking change here.

db = await orbitdb.create('no-meta', 'feed')
const manifest = await io.read(ipfs, db.address.root)
assert.strictEqual(db.options.meta, undefined)
assert.deepStrictEqual(Object.keys(manifest).filter(k => k === 'meta'), [])
Copy link
Member

@shamb0t shamb0t Sep 16, 2019

Choose a reason for hiding this comment

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

Could we use assert.strictEqual(manifest.meta, undefined) here to match the convention in the rest of the codebase? What do you think?

Copy link
Member Author

@tabcat tabcat Sep 16, 2019

Choose a reason for hiding this comment

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

yes that's a good idea

Copy link
Member Author

@tabcat tabcat Sep 16, 2019

Choose a reason for hiding this comment

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

i guess the lines 228 and 229 are also redundant?

Copy link
Member

@shamb0t shamb0t Sep 17, 2019

Choose a reason for hiding this comment

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

Hmm could be but I don't see them as redundant since you're checking whether meta is set in the db options, while 229 checks that meta has been written to ipfs.

Copy link
Member Author

@tabcat tabcat Sep 19, 2019

Choose a reason for hiding this comment

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

made a commit to try and solve the code convention issue. it makes lines 228 and 229 sort of redundant so we could remove line 228, but personally it doesnt seem like a big deal either way and also keeps both tests more symmetrical. lmk what you think

db = await orbitdb.create('meta', 'feed', { meta })
const manifest = await io.read(ipfs, db.address.root)
assert.deepStrictEqual(db.options.meta, meta)
assert.deepStrictEqual(Object.keys(manifest).filter(k => k === 'meta'), ['meta'])
Copy link
Member

@shamb0t shamb0t Sep 16, 2019

Choose a reason for hiding this comment

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

For similar reasons perhaps here we could use assert.notEqual(manifest.meta, undefined)? Or assert.deepStrictEqual(manifest.meta, meta)? Would that achieve the same thing?

Copy link
Member Author

@tabcat tabcat Sep 16, 2019

Choose a reason for hiding this comment

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

i guess either one works although using not notEqual here the next line, line 237, seems close to redundant. line 237's type of check is really only needed in the no-meta test, if you dont see a need for using deepStrictEqual here you could get rid of one of those lines.

Copy link
Member Author

@tabcat tabcat Sep 19, 2019

Choose a reason for hiding this comment

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

made a commit below that changes db.options.meta to manifest.meta in both tests. it seemed like it would solve the code convention issue here.

@shamb0t
Copy link
Member

@shamb0t shamb0t commented Sep 16, 2019

Agrees this is a great addition, thank you @tabcat! Just commented a few stylistic suggestions

@aphelionz
Copy link
Member

@aphelionz aphelionz commented Sep 17, 2019

Happy to merge this once the feedback has been addressed

@aphelionz
Copy link
Member

@aphelionz aphelionz commented Sep 24, 2019

@shamb0t satisfied with the code changes above?

@shamb0t
Copy link
Member

@shamb0t shamb0t commented Sep 25, 2019

Thanks @tabcat @aphelionz lgtm 👍

@aphelionz aphelionz merged commit c04be71 into orbitdb:master Sep 27, 2019
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants