[DE-584] Refactor Deprecated /_api/simple methods
#275
Conversation
| @@ -1364,7 +1379,7 @@ def test_document_find_in_box(col, bad_col, geo, cluster): | |||
| result = col.find_in_box( | |||
| latitude1=0, longitude1=0, latitude2=6, longitude2=3, limit=0, index=geo["id"] | |||
| ) | |||
| assert clean_doc(result) == [doc1, doc3] | |||
| assert clean_doc(result) == [] | |||
There was a problem hiding this comment.
Based on limit = 0, this case should have never returned any data. strange 🤔
regardless, limit = 0 is behaving as expected now
arango/collection.py
Outdated
| if index is not None: | ||
| data["geo"] = self._name + "/" + index | ||
| geo_index = self.get_index(f"{self.name}/{index}") |
There was a problem hiding this comment.
In need of a second opinion on the new implementation of collection.find_in_box
Given the deprecation of the WITHIN_RECTANGLE AQL function, we are now opting to use the GEO_CONTAINS method in combination with the GEO_POLYGON method: https://www.arangodb.com/docs/stable/aql/functions-geo.html#within_rectangle
The GEO_CONTAINS method takes two parameters: the polygon, and the coordinates of the document.
Given that the attribute names representing the coordinates of the document can vary on a user-basis, we cannot assume that doc.longitude and doc.latitude will always exist.
In order to prevent any breaking changes to collection.find_in_box, we rely on the existing index parameter to fetch the Geo Index's fields values to use as the document's coordinate attributes.
Therefore, we assume that the Geo Index fields property is an array containing 2 values at most. Otherwise, a ValueError is raised.
Happy to move to another solution
There was a problem hiding this comment.
I agree!
Trying to create a GEO index with more than 2 attributes will throw an exception. IMO, double checking it here (along with the fact that the index is truly "geo") is a good idea.
arango/collection.py
Outdated
| # TODO: REVISIT `geo_str` logic | ||
| # https://www.arangodb.com/docs/stable/aql/functions-geo.html#within_rectangle | ||
| # Initial assumption that attribute names are "latitude" and "longitude" |
There was a problem hiding this comment.
The default behavior of whithin_rectangle, if an index is not specified, is to use the first geo index that is found. Assuming that the user wants to use whatever geo index is there has its pros and cons, and while I'm not really a fan of that, for now, I think we should keep the current behavior unchanged.
This would be the equivalent of retrieving self.indexes() and searching for the first one with type: geo. If no such index exists, we should throw.
The error that is returned from the arangodb server is:
"ERROR_QUERY_GEO_INDEX_MISSING" : { "code" : 1570, "message" : "no suitable geo index found for geo restriction on '%s'" }
We could create a new exception for this case (e.g. IndexMissingError).
There was a problem hiding this comment.
Sounds good, let me know what you think: 55d559d
arango/collection.py
Outdated
| if index is not None: | ||
| data["geo"] = self._name + "/" + index | ||
| geo_index = self.get_index(f"{self.name}/{index}") |
There was a problem hiding this comment.
I agree!
Trying to create a GEO index with more than 2 attributes will throw an exception. IMO, double checking it here (along with the fact that the index is truly "geo") is a good idea.
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #275 +/- ##
==========================================
- Coverage 99.01% 98.77% -0.24%
==========================================
Files 26 26
Lines 3958 4016 +58
==========================================
+ Hits 3919 3967 +48
- Misses 39 49 +10
|
adding a geo index only allows 1 or 2 fields, so this case will not happen (unless this changes in the future)
Main changes
Re-implements the following methods via AQL:
collection.idscollection.keyscollection.allcollection.findcollection.find_in_box(requesting feedback / second opinion on implementation)collection.randomcollection.update_matchcollection.replace_matchcollection.delete_matchImplements the
collection.get_indexmethod (GET /_api/index/{id}) as it was previously missingMiscellaneous changes
build_filter_conditions(now used in the filter-based simple methods)allow_dirty_readparameter to the reworked simple methods given that they are now implemented using the_api/cursorendpoint