arangodb / arangojs

The official ArangoDB JavaScript driver.
https://arangodb.github.io/arangojs
Apache License 2.0
601 stars 107 forks source link

The cursor.forEach callback is called with "undefined" even if there are no results #683

Closed rensdewolf closed 4 years ago

rensdewolf commented 4 years ago

arangojs 7.0.1 arangodb 3.6.5 typescript 4.0.2

When running the following using a db instance:

const query = aql`FOR i IN [] RETURN i`; const cursor = await db.query(query, { count: true }); console.log(cursor.count); await cursor.forEach((value: any) => { console.log(value); });

The console output is:

0 undefined

It seems the forEach callback is called once although the count is 0. IMHO this should not happen and the console output should read 0 only. Note that in the previous version (6.x.x), the each callback was not called.

The forEach method of the ArrayCursor looks at this.hasMore || Boolean(this._batches.length) to execute the callback. The use of the non-null assertion operator on the value seems somewhat misleading as value returned is undefined. Not sure why the _batches.length equals 1...

rensdewolf commented 4 years ago

@pluma Is someone able to confirm this issue? Many thanks.

pluma commented 4 years ago

This looks like a bug. The problem is that an empty result set is still logically a batch (though an empty one) and the logic only checks if there are any batches left. I think the correct behavior would be not to create empty batches: https://github.com/arangodb/arangojs/blob/v7.0.1/src/cursor.ts#L101-L102

 const initialBatch = new LinkedList(body.result);
-const batches = new LinkedList([initialBatch]);
+const batches = new LinkedList(body.result.length ? [initialBatch] : []);

We currently clean up batches as they are drained, so allowing the initial batch to be empty rather than just having no initial batch when the result set is empty doesn't seem like a good idea.

However it seems we don't have a test case for an empty batch, so we should add one before fixing this.