feat: add support for sync car reading #121

hugomrdias · 2023-01-12T16:10:15Z

This PR adds a new class CarReaderSync to support syncronous car reading.

Changes:

Extracted some utils from the async decoder
- decodeV2Header, decodeVarint and getMultihashLength
Created a new sync decoder
BytesReader interface has a new arg in the exactly method to seek after reading, defaults to false (previous behavior)
Async decoder was tweaked to used the extracted utils and the new types.
New types were added for buffer (sync) methods

I was hoping to be able to extract more from the async decoder than i did, but i think this is useful and does not introduce any breaking change.

The new CarReaderSync is exported in package.json.

src/api.ts

src/coding.ts

src/decoder-sync.js

Gozala

Other than interface for the sync reader everything looks good to me, although I did provide some additional feedback which you can address or disregard as you find fit.

Please add a sync interface though, or if you really disagree let's find a compromise.

src/api.ts

src/coding.ts

src/decoder-sync.js

src/reader-browser.js

Gozala · 2023-01-18T19:37:43Z

src/reader-browser.js

+   * @param {Uint8Array} bytes
+   * @returns {CarReader} blip blop
+   */
+  static fromBytesSync (bytes) {


I think it might be a good idea to align naming convention with https://github.com/ipld/js-car/blob/master/src/buffer-writer.js which is same idea but a writer interface.

what do you means ?

Things like CarBufferReader as opposed to CarReaderSync, same thing for method names and stuff.

Gozala · 2023-01-18T19:39:26Z

src/reader-browser.js

@@ -139,6 +140,22 @@ export class CarReader {
    }
  }

+  /**
+   *


I think it should say something about the fact that no verification occurs with this reader, meaning that block that follows CID may not in fact have that CID.

Gozala · 2023-01-18T19:44:37Z

src/reader-sync-browser.js

+    this._header = header
+    this._blocks = blocks
+    this._keys = blocks.map((b) => b.cid.toString())
+  }


Can we have a method to perform verification that will verify that blocks match claimed CIDs ?

@Gozala this would be worthwhile but needs to be an option and should be tackled in a separate PR. Opened an issue here and it'd be great if someone could pick this off: #123

As noted there, this requires having the hashers available to do the validation, which is always a problem in JS—what do you do if someone shows up with blake2b CIDs?

Also worth having notes in the README, although pretty much all of the JS stack doesn't perform this kind of validation, so there's a bit of a caveat emptor all round on that front unfortunately.

rvagg · 2023-01-19T07:13:20Z

I'm not sure jsdoc2readme is working properly on this anymore, npm run docs is supposed to take the jsdoc updates and insert it into the readme; if that doesn't work, you might have to do a bit of manual sync, but getting something on the README would be good for this.

As long as the 3 of you agree on the details don't let me be a blocker, other than the desire for decent docs.

hugomrdias · 2023-01-19T19:05:24Z

I'm not sure jsdoc2readme is working properly on this anymore, npm run docs is supposed to take the jsdoc updates and insert it into the readme; if that doesn't work, you might have to do a bit of manual sync, but getting something on the README would be good for this.

As long as the 3 of you agree on the details don't let me be a blocker, other than the desire for decent docs.

i was able to make jsdoc2readme work readme is updated and i also added a reference to the sync reader in the usage section.

alanshaw

There's so many small things here I'm not even sure I got all of them.

src/api.ts

src/buffer-reader-browser.js

README.md

src/api.ts

src/buffer-reader-browser.js

alanshaw

Just a few minor problems but I think nothing blocking...🚀

alanshaw · 2023-01-23T15:50:57Z

README.md

+    - [`CarBufferReader.fromBytes(bytes)`](#carbufferreaderfrombytesbytes)
+    - [`CarBufferReader.readRaw(fd, blockIndex)`](#carbufferreaderreadrawfd-blockindex)
+  - [License](#license)
+    - [Contribution](#contribution)


This is just the API listing not the ToC for the whole README...

alanshaw · 2023-01-23T17:56:05Z

src/buffer-reader-browser.js

+   * @returns {boolean}
+   */
+  has (key) {
+    return Boolean(this._cids.find((cid) => cid.equals(key)))


Suggested change

return Boolean(this._cids.find((cid) => cid.equals(key)))

return this._blocks.some(b => b.cid.equals(key))

alanshaw · 2023-01-23T17:58:35Z

src/buffer-reader-browser.js

+    const index = this._keys.indexOf(key.toString())
+    return index > -1 ? this._blocks[index] : undefined


Why not just find the block?

Suggested change

const index = this._keys.indexOf(key.toString())

return index > -1 ? this._blocks[index] : undefined

return this_blocks.find(b -> b.cid.equals(key))

alanshaw · 2023-01-23T18:02:04Z

src/buffer-reader-browser.js

+    this._header = header
+    this._blocks = blocks
+    this._keys = blocks.map((b) => b.cid.toString())
+    this._cids = this._blocks.map(b => b.cid)


I'm not sure I understand the benefit of _keys and _cids, or why in has we do .equals() and in get we do a string comparison?

I guess this._cids is so we don't have to recompute the array every time .cids() it's called - but personally I'd just cache the result after the first call.

hugomrdias · 2023-01-24T14:27:24Z

@rvagg seems ready to merge.

rvagg

lgtm, very nice work all!

## [5.1.0](v5.0.3...v5.1.0) (2023-01-27) ### Features * add support for sync car reading ([#121](#121)) ([c3917aa](c3917aa))

github-actions · 2023-01-27T07:24:48Z

🎉 This PR is included in version 5.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Remove sync car reader code that has moved upstream into [@ipld/car](ipld/js-car#121 (comment)) Fixes #401 License: MIT Signed-off-by: Oli Evans <oli@protocol.ai>

Remove sync car reader code in favour of CarBufferReader that landed upstream in [@ipld/car](ipld/js-car#121 (comment)) Fixes #401 ❤️ License: MIT Signed-off-by: Oli Evans <oli@protocol.ai> --------- Signed-off-by: Oli Evans <oli@protocol.ai> Co-authored-by: Benjamin Goering <171782+gobengo@users.noreply.github.com> Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>

feat: add support for sync car reading

604b12c

hugomrdias requested review from alanshaw and rvagg January 12, 2023 16:11

alanshaw requested changes Jan 13, 2023

View reviewed changes

src/api.ts Outdated Show resolved Hide resolved

src/api.ts Outdated Show resolved Hide resolved

src/api.ts Outdated Show resolved Hide resolved

src/coding.ts Outdated Show resolved Hide resolved

src/decoder-sync.js Outdated Show resolved Hide resolved

chore: feedback

f682759

hugomrdias requested review from alanshaw and Gozala January 18, 2023 15:34

Gozala approved these changes Jan 18, 2023

View reviewed changes

hugomrdias added 2 commits January 19, 2023 18:43

fix: add node CarReaderSync and fix types

93a2951

chore: change name to car buffer reader

2374d1e

hugomrdias requested a review from Gozala January 19, 2023 19:05

alanshaw requested changes Jan 19, 2023

View reviewed changes

fix: remove asyncs from jsdocs and normalize naming to Buffer*

3714112

hugomrdias requested a review from alanshaw January 20, 2023 15:50

hugomrdias added 2 commits January 20, 2023 15:54

chore: remove one last sync

f2b2287

chore: tweak fromBytes methods docs

0342fb1

alanshaw approved these changes Jan 23, 2023

View reviewed changes

chore: feedback

a1e26da

Gozala approved these changes Jan 24, 2023

View reviewed changes

rvagg approved these changes Jan 27, 2023

View reviewed changes

hugomrdias merged commit c3917aa into master Jan 27, 2023

hugomrdias deleted the feat/car-sync branch January 27, 2023 07:19

github-actions bot pushed a commit that referenced this pull request Jan 27, 2023

chore(release): 5.1.0 [skip ci]

653d52c

## [5.1.0](v5.0.3...v5.1.0) (2023-01-27) ### Features * add support for sync car reading ([#121](#121)) ([c3917aa](c3917aa))

github-actions bot added the released label Jan 27, 2023

rvagg mentioned this pull request Jan 27, 2023

Option to validate CID matches block #123

Open

hugomrdias mentioned this pull request Jan 27, 2023

Remove car sync reader storacha/w3up#401

Closed

olizilla mentioned this pull request Feb 8, 2023

chore: use CarBufferReader from @ipld/car storacha/w3up#428

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for sync car reading #121

feat: add support for sync car reading #121

hugomrdias commented Jan 12, 2023 •

edited

Loading

Gozala left a comment

Gozala Jan 18, 2023

hugomrdias Jan 19, 2023

Gozala Jan 19, 2023

Gozala Jan 18, 2023

Gozala Jan 18, 2023

rvagg Jan 27, 2023

rvagg commented Jan 19, 2023

hugomrdias commented Jan 19, 2023

alanshaw left a comment

alanshaw left a comment

alanshaw Jan 23, 2023

alanshaw Jan 23, 2023

alanshaw Jan 23, 2023

alanshaw Jan 23, 2023

hugomrdias commented Jan 24, 2023

rvagg left a comment

github-actions bot commented Jan 27, 2023

@@ @@ -139,6 +140,22 @@ export class CarReader { @@
                   }
                 }
+                /**
+                 *

	return Boolean(this._cids.find((cid) => cid.equals(key)))
	return this._blocks.some(b => b.cid.equals(key))

		const index = this._keys.indexOf(key.toString())
		return index > -1 ? this._blocks[index] : undefined

	const index = this._keys.indexOf(key.toString())
	return index > -1 ? this._blocks[index] : undefined
	return this_blocks.find(b -> b.cid.equals(key))

feat: add support for sync car reading #121

feat: add support for sync car reading #121

Conversation

hugomrdias commented Jan 12, 2023 • edited Loading

Gozala 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Jan 19, 2023

hugomrdias commented Jan 19, 2023

alanshaw left a comment

Choose a reason for hiding this comment

alanshaw 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

hugomrdias commented Jan 24, 2023

rvagg left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 27, 2023

hugomrdias commented Jan 12, 2023 •

edited

Loading