Skip to content

Implement a bundle reader for Web #3097

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jun 5, 2020
Merged

Implement a bundle reader for Web #3097

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9602712
Renaming interfaces without leading I
wu-hui May 15, 2020
c5e783e
Initial commit of bundle reading - for web only.
wu-hui May 21, 2020
5e7fb89
Tests only run when it is not Node.
wu-hui May 21, 2020
1ee1615
Fix redundant imports
wu-hui May 21, 2020
18f0be1
Fix missing textencoder
wu-hui May 21, 2020
aa455bf
Remove generator.
wu-hui May 29, 2020
782e76f
Address comments
wu-hui Jun 3, 2020
f5e4474
Address more feedback.
wu-hui Jun 4, 2020
c60f8dc
Race condition.
wu-hui Jun 4, 2020
e005485
Wait for metadata to be read before move on to rest elements.
wu-hui Jun 4, 2020
6d9d5cd
Remove only.
wu-hui Jun 4, 2020
a869466
Fix payload printing.
wu-hui Jun 5, 2020
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
37 changes: 20 additions & 17 deletions packages/firestore/src/util/bundle_reader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ export class BundleReader {
}
this.reader = (this.bundleStream as ReadableStream).getReader();

// Read the metadata (which is the first element).
this.nextElement().then(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a small inline comment before this line:

// Read the metadata (which is the first element)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

element => {
if (element && element.isBundleMetadata()) {
Expand All @@ -109,9 +110,7 @@ export class BundleReader {
);
}
},
error => {
this.metadata.reject(error);
}
error => this.metadata.reject(error)
);
}

Expand Down Expand Up @@ -175,25 +174,29 @@ export class BundleReader {
* If reached end of the stream, returns a null.
*/
private async readLength(): Promise<Uint8Array | null> {
Copy link
Contributor

Choose a reason for hiding this comment

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

JSDoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

let position = this.indexOfOpenBracket();
while (position < 0) {
let position: number;
while ((position = this.indexOfOpenBracket()) < 0) {
const done = await this.pullMoreDataToBuffer();
if (done) {
if (this.buffer.length === 0) {
return null;
}
position = this.indexOfOpenBracket();
// Underlying stream is closed, and we still cannot find a '{'.
if (position < 0) {
this.raiseError(
'Reached the end of bundle when a length string is expected.'
);
}
} else {
position = this.indexOfOpenBracket();
break;
}
}

// Broke out of the loop because underlying stream is closed, and there
// happens to be no more data to process.
if (this.buffer.length === 0) {
return null;
}

position = this.indexOfOpenBracket();
Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop position from the loop above and assign it here as a const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// Broke out of the loop because underlying stream is closed, but still
// cannot find an open bracket.
if (position < 0) {
this.raiseError(
'Reached the end of bundle when a length string is expected.'
);
}

const result = this.buffer.slice(0, position);
// Update the internal buffer to drop the read length.
this.buffer = this.buffer.slice(position);
Expand Down
30 changes: 17 additions & 13 deletions packages/firestore/test/unit/util/bundle.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ function genericBundleReadingTests(bytesPerRead: number): void {
};
const limitToLastQueryString = lengthPrefixedString(limitToLastQuery);

async function getAllElement(
async function getAllElements(
bundle: BundleReader
): Promise<SizedBundleElement[]> {
const result: SizedBundleElement[] = [];
Expand Down Expand Up @@ -194,7 +194,7 @@ function genericBundleReadingTests(bytesPerRead: number): void {
);
}

async function parseThroughBundle(
async function generateBundleAndParse(
bundleString: string,
bytesPerRead: number,
validMeta = false
Expand All @@ -208,7 +208,7 @@ function genericBundleReadingTests(bytesPerRead: number): void {
expect(await bundle.getMetadata()).to.deep.equal(meta.metadata);
}

await getAllElement(bundle);
await getAllElements(bundle);
}

it('reads with query and doc with bytesPerRead ' + bytesPerRead, async () => {
Expand All @@ -224,7 +224,7 @@ function genericBundleReadingTests(bytesPerRead: number): void {

expect(await bundle.getMetadata()).to.deep.equal(meta.metadata);

const actual = await getAllElement(bundle);
const actual = await getAllElements(bundle);
expect(actual.length).to.equal(4);
verifySizedElement(actual[0], limitQuery, limitQueryString);
verifySizedElement(actual[1], limitToLastQuery, limitToLastQueryString);
Expand All @@ -246,7 +246,7 @@ function genericBundleReadingTests(bytesPerRead: number): void {
);
const bundle = new BundleReader(bundleStream);

const actual = await getAllElement(bundle);
const actual = await getAllElements(bundle);
expect(actual.length).to.equal(5);
verifySizedElement(actual[0], doc1Meta, doc1MetaString);
verifySizedElement(actual[1], doc1, doc1String);
Expand All @@ -270,7 +270,7 @@ function genericBundleReadingTests(bytesPerRead: number): void {

expect(await bundle.getMetadata()).to.deep.equal(meta.metadata);

const actual = await getAllElement(bundle);
const actual = await getAllElements(bundle);
expect(actual.length).to.equal(2);
verifySizedElement(actual[0], doc1Meta, doc1MetaString);
verifySizedElement(actual[1], doc1, doc1String);
Expand All @@ -286,7 +286,7 @@ function genericBundleReadingTests(bytesPerRead: number): void {

expect(await bundle.getMetadata()).to.deep.equal(meta.metadata);

const actual = await getAllElement(bundle);
const actual = await getAllElements(bundle);
expect(actual.length).to.equal(3);
verifySizedElement(actual[0], noDocMeta, noDocMetaString);
verifySizedElement(actual[1], doc1Meta, doc1MetaString);
Expand All @@ -301,7 +301,7 @@ function genericBundleReadingTests(bytesPerRead: number): void {

expect(await bundle.getMetadata()).to.deep.equal(meta.metadata);

const actual = await getAllElement(bundle);
const actual = await getAllElements(bundle);
expect(actual.length).to.equal(0);
}
);
Expand All @@ -310,28 +310,32 @@ function genericBundleReadingTests(bytesPerRead: number): void {
'throws with ill-formatted bundle with bytesPerRead ' + bytesPerRead,
async () => {
await expect(
parseThroughBundle('metadata: "no length prefix"', bytesPerRead)
generateBundleAndParse('metadata: "no length prefix"', bytesPerRead)
).to.be.rejectedWith(
'Reached the end of bundle when a length string is expected.'
);

await expect(
parseThroughBundle('{metadata: "no length prefix"}', bytesPerRead)
generateBundleAndParse('{metadata: "no length prefix"}', bytesPerRead)
).to.be.rejectedWith('Unexpected end of JSON input');

await expect(
parseThroughBundle(metaString + 'invalid-string', bytesPerRead, true)
generateBundleAndParse(
metaString + 'invalid-string',
bytesPerRead,
true
)
).to.be.rejectedWith(
'Reached the end of bundle when a length string is expected.'
);

await expect(
parseThroughBundle('1' + metaString, bytesPerRead)
generateBundleAndParse('1' + metaString, bytesPerRead)
).to.be.rejectedWith('Reached the end of bundle when more is expected.');

// First element is not BundleMetadata.
await expect(
parseThroughBundle(doc1MetaString + doc1String, bytesPerRead)
generateBundleAndParse(doc1MetaString + doc1String, bytesPerRead)
).to.be.rejectedWith('The first element of the bundle is not a metadata');
}
);
Expand Down