ArangoDB-Community / pyArango

Python Driver for ArangoDB with built-in validation
https://pyarango.readthedocs.io/en/latest/
Apache License 2.0
238 stars 90 forks source link

Fix infinite loop when iterating Query with rawResults is set #174

Closed DonQueso89 closed 4 years ago

DonQueso89 commented 4 years ago

When retrieving Query results with rawResults set to True, the iterator never terminates because the IndexError signaling that there are no more items in self.result is caught in __getitem__ and an empty list is returned.

In stead of this, the error should propagate to __next__ so that nextBatch is called and StopIteration is raised.

codecov-io commented 4 years ago

Codecov Report

Merging #174 into master will increase coverage by 0.04%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
+ Coverage   78.13%   78.18%   +0.04%     
==========================================
  Files          17       17              
  Lines        2946     2943       -3     
==========================================
- Hits         2302     2301       -1     
+ Misses        644      642       -2
Impacted Files Coverage Δ
pyArango/query.py 82.85% <100%> (+1.03%) :arrow_up:

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 c68899b...e91e034. Read the comment docs.

tariqdaouda commented 4 years ago

Thank you @DonQueso89, I think this has already been solved in an other PR. Let me know if you still encounter the same issue.

DonQueso89 commented 4 years ago

Hey @tariqdaouda, yes I saw that pr but still submitted this one hoping that you would accept this one for being a better solution. In stead of handling the IndexError in two different places it would be better to have ‘getitem’ always let the caller handle the exception. This makes ‘getitem’ more transparent, easier to test and its behavior would be consistent regardless of the setting of rawResult

Now that i mention it i might even add some tests

tariqdaouda commented 4 years ago

@DonQueso89 I agree, it would be great if you could a small test so we do not face the same problem again. I'll merge your PR them. Is that ok with you?

DonQueso89 commented 4 years ago

Waiting for a test.

@tariqdaouda added a basic test that covers the usage of SimpleQuery as a regular iterator

DonQueso89 commented 4 years ago

@tariqdaouda will this still be merged?

tariqdaouda commented 4 years ago

Would you mind re sending the PR to the dev branch? That's where everything is integrated.