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

Issue 87 problem with unvalidated nested data #117

Closed thekie closed 5 years ago

thekie commented 5 years ago

The problem for me was that pyArango implicitly expects nested fields to be validated as well. In the DocumentStore you find the following lines in the set() and __setitem__ methods:

if isinstance(value, dict) :
    if field in self.validators:
        vals = self.validators[field]
    else :
        vals = {}
    self.store[field] = DocumentStore(self.collection, validators = vals, initDct = value, patch =         
    self.patching, subStore=True, validateInit=self.validateInit)      

These lines check if the current value of the field is a dictionary and if that is the case, the validators of this fields are redirected into the sup DataStore. This is problematic if your field definition looks like this:

_fields = {
    "nested_something": Field()
}

If you now set nested_something to a some nested data, the value of the Field nested_somethingis a dict and the validators vals (from the code snippet above) has the value of a Field object. Unfortunately, the DataStoreexpects to get a dictionary mapping field name to Fieldobject, which results in the error message.

TypeError: argument of type 'Field' is not iterable

This code makes perfect sense if your field definition looks like this:

_fields = {
    "nested_string": {
        "some_string": Field([VAL.String()])
    }
}

In this case, the value of the validatorsvals, which get into the DataStore is a dictionary mapping field name to Fieldobject and the nested fields are getting validated as expected. But in our definition 1, we don't want to validate the subfields.

So the fix is to check if the field definition is actually a dict to and only then redirect the validators to the sub DataStore.

if field in self.validators and isinstance(self.validators[field], dict):

This way the validation runs as expected and if you allow allow_foreign_fields for your Collection you can use arbitrary nested data.

codecov-io commented 5 years ago

Codecov Report

Merging #117 into master will increase coverage by 0.27%. The diff coverage is 97.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
+ Coverage   80.01%   80.28%   +0.27%     
==========================================
  Files          12       12              
  Lines        2117     2141      +24     
==========================================
+ Hits         1694     1719      +25     
+ Misses        423      422       -1
Impacted Files Coverage Δ
pyArango/document.py 74.37% <100%> (+0.71%) :arrow_up:
pyArango/tests/tests.py 97.67% <97.29%> (-0.09%) :arrow_down:

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 3d91c08...b09fa4a. Read the comment docs.

tariqdaouda commented 5 years ago

Thank you for this great PR. Very well explained and tested. Amazing work.

thekie commented 5 years ago

Thank you! My pleasure @tariqdaouda!