feat: Use HEAD HTTP method to check if a document exists

[Darkheir]

The current has method requests the full document to only check if it exists or not. It may be overkill for big documents.

This PR replaces it with a simple HEAD call allowing to know if the document exists or not.

:warning: This PR will change the error_code returned. Since we don't have a body in the response, the error_code value will default to status_code. If the only codes we should handle are the ones in the test, I could update the response object to set the appropriate error_code based on the status code.

[joowani]

Hi @Darkheir, thanks for the PR. Could you please address the failing test? Thanks.

[Darkheir]

After some investigation it seems that ArangoDB server hangs on result retrieval for HEAD requests. I created an issue here: https://github.com/arangodb/arangodb/issues/15639

To solve this issue I created a specific condition in the AsyncExecutor for HEAD requests where we directly get the result and return a dummy async job that imitate the async job signature.

IMO this is not a big issue since HEAD requests are never going to be costly and the only one I know is for the document's headers.

[codecov-commenter]

Codecov Report

Merging #192 (d88243e) into main (ff990fd) will decrease coverage by 0.13%. The diff coverage is 94.11%.

@@            Coverage Diff             @@
##             main     #192      +/-   ##
==========================================
- Coverage   99.97%   99.83%   -0.14%     
==========================================
  Files          26       26              
  Lines        3694     3734      +40     
==========================================
+ Hits         3693     3728      +35     
- Misses          1        6       +5     

Impacted Files	Coverage Δ
arango/aql.py	99.10% <50.00%> (-0.45%)	:arrow_down:
arango/job.py	97.91% <85.71%> (-2.09%)	:arrow_down:
arango/connection.py	99.26% <96.15%> (-0.74%)	:arrow_down:
arango/client.py	100.00% <100.00%> (ø)
arango/collection.py	99.87% <100.00%> (-0.13%)	:arrow_down:
arango/executor.py	100.00% <100.00%> (ø)
arango/resolver.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 77cbc68...d88243e. Read the comment docs.

[joowani]

Hi @Darkheir,

I'm not in favor of introducing the dummy async job class. It seems too "hacky" for a small performance gain, and breaks the users' expectation of error codes from async API calls. I recommend that we undo the dummy class addition, and merge just the "head" bit once arangodb/arangodb#15639 is resolved. Thanks again for the pull request and for creating the issue with ArangoDB.

[Darkheir]

Hi @Darkheir,

I'm not in favor of introducing the dummy async job class. It seems too "hacky" for a small performance gain, and breaks the users' expectation of error codes from async API calls. I recommend that we undo the dummy class addition, and merge just the "head" bit once arangodb/arangodb#15639 is resolved. Thanks again for the pull request and for creating the issue with ArangoDB.

Fair enough, let's wait until ArangoDB fixed the issue :-)

The bad thing is that even if ArangoDB fixes it, all the people using an old ArangoDB version will get some errors when using python-arango...

[joowani]

That's a good point. You can leave this PR as is for now, and I'll think about it a bit more (maybe we'll merge this after all).

[apetenchea]

Hi @Darkheir,

Thanks again for your work. Are you still interested in continuing this PR? Otherwise, I can take over.

The task is even easier now, as DummyAsyncJob is no longer needed.

[Darkheir]

Hi @Darkheir,

Thanks again for your work. Are you still interested in continuing this PR? Otherwise, I can take over.

The task is even easier now, as DummyAsyncJob is no longer needed.

Did ArangoDB fix the issue ? The ticket is still open on their side: #15639

[apetenchea]

Did ArangoDB fix the issue ? The ticket is still open on their side: #15639

3.8 is not supported since long ago. I tried the head method on 3.11 and it works fine. Anyway, the driver has a CI now, which will tell whether there's any problem, though I doubt there will be any. Since we're planning the next release to be a major one, this would be the time to make such a change.

Please let me know if you still want to take this through (no pressure).

[Darkheir]

Just updated the PR, let's see how the CI goes :-)

[Darkheir]

It seems that the CI doesn't run all the tests (no --complete flag).

The complete test suite is failing locally and I still reproduce #15639 on the latest version of ArangoDB...

[apetenchea]

It seems that the CI doesn't run all the tests (no --complete flag).

The complete test suite is failing locally and I still reproduce #15639 on the latest version of ArangoDB...

Ok, I'll take a look. Could you please let me know the pytest command you used? (to make sure we're talking about the same thing)

[Darkheir]

I ran the test with pytest --complete

[apetenchea]

My bad, I ran it without the x-arango-async header. Indeed, the issue is still there 🙄

I will look into the ArangoDB C++ code when I get some time, perhaps I can fix this issue. But I really want this to get through one way or another (worst case we can add an "use_get" parameter to make it work).

I cannot make any promise regarding the timing, but I am keeping this on my list and will get back to you at some point 🙏