arangodb / arangojs

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

maybe missing `isArangoError` check in catch? #757

Closed meadowsys closed 2 years ago

meadowsys commented 2 years ago

I was looking through the source code, and i found this snippet:

https://github.com/arangodb/arangojs/blob/44c5e54fd2cb246771cdc83100e4a5af7b60ab8f/src/collection.ts#L3533-L3534

looking at other catch blocks in the same file, i see that in the if statement it usually checks the err using isArangoError first before doing anything with it, but the first one doesn't do it:

https://github.com/arangodb/arangojs/blob/44c5e54fd2cb246771cdc83100e4a5af7b60ab8f/src/collection.ts#L3379-L3380 https://github.com/arangodb/arangojs/blob/44c5e54fd2cb246771cdc83100e4a5af7b60ab8f/src/collection.ts#L3576-L3577

nt sure if this is intentional or anything, but I feel like its worth bringing up?

pluma commented 2 years ago

I thought you had found a potential bug but after implementing the change the tests broke, which reminded me why these methods are different:

The exists() methods use HEAD which does not include the response body and thus can not return an ArangoError. This is a performance optimization to avoid fetching unnecessary data. There are some caveats (e.g. if you point it at a server that is not actually an ArangoDB instance you might still get a 404) but they're worth the tradeoff.

meadowsys commented 2 years ago

ahh alrighty, thanks!