Implement $arrayToObject operator #624

hugo19941994 · 2020-05-06T12:41:16Z

Hi!

I've implemented the arrayToObject operator for the aggregation pipeline.

I've added tests for all the edge cases I could think of:

Mixed array and document inside the same array
Extra keys in documents
Missing keys in document
Empty document
More than two elements in an array
Just one element in an array
Empty array

Duplicated keys are also accounted for and tested (latest found value is used, overwriting previous values), but there is a caveat in the mongo documentation for earlier versions. I'm not sure if this should be accounted for or mocking the behaviour for the latest versions is enough.

codecov · 2020-05-06T12:46:23Z

Codecov Report

Merging #624 into develop will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #624      +/-   ##
===========================================
+ Coverage    94.76%   94.81%   +0.05%     
===========================================
  Files           16       17       +1     
  Lines         3247     3281      +34     
===========================================
+ Hits          3077     3111      +34     
  Misses         170      170

Impacted Files	Coverage Δ
mongomock/aggregate.py	`97.15% <100.00%> (+0.05%)`	⬆️
mongomock/collection.py	`91.63% <0.00%> (-0.06%)`	⬇️
mongomock/read_concern.py	`100.00% <0.00%> (ø)`
mongomock/filtering.py	`98.92% <0.00%> (+0.35%)`	⬆️

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 d9e8d87...9e7e0e2. Read the comment docs.

hugo19941994 · 2020-05-06T12:51:39Z

I'll add some tests for those missing lines for coverage

hugo19941994 · 2020-05-06T15:11:23Z

It looks like $arrayToObject with a null value is valid too, and returns another null. I've added this edge case too.

pcorpet

Great works, thanks for adding.

pcorpet · 2020-05-23T06:37:13Z

tests/test__mongomock.py

@@ -3065,6 +3065,36 @@ def test_aggregate_date_to_string(self):
        ]
        self.cmp.compare.aggregate(pipeline)

+    def test_aggregate_array_to_object(self):
+        self.cmp.do.drop()
+        self.cmp.do.insert_one({


Please use insert_many to add all those docs at once.

pcorpet · 2020-05-23T06:38:59Z

tests/test__collection_api.py

+        }]
+        self.assertEqual(expect, list(actual))
+
+        with self.assertRaises(TypeError):


Here you could use a table test: create a list of items that should trigger an error and then iterate over them and do an aggregation and an asserRaises for each of them.

pcorpet · 2020-05-23T06:39:43Z

tests/test__collection_api.py

+
+        with self.assertRaises(TypeError):
+            collection.aggregate([
+                {'$addFields': {'items': 100}},


Add all those errors in test__mongomock using compare_exceptions

pcorpet · 2020-05-23T06:40:37Z

mongomock/aggregate.py

+            if parsed is None:
+                return None
+
+            if not isinstance(parsed, list):


In general were list is supported, tuple is supported as well. Can you check on pymongo and update it here please?

pcorpet · 2020-05-23T06:42:26Z

mongomock/aggregate.py

+                raise TypeError('$arrayToObject only supports arrays')
+
+            def check_doc(elements):
+                return all([isinstance(x, dict) and 'k' in x and 'v' in x and len(x) == 2


I don't think you need the [] here; Also you might be able to check with set equality:

return all(isinstance(x, dict) and x.keys() == {'k', 'v'} for x in elements)

pcorpet · 2020-05-23T06:43:04Z

mongomock/aggregate.py

+                            for x in elements])
+
+            def check_arr(elements):
+                return all([isinstance(x, list) and len(x) == 2 for x in elements])


here as well, please drop the [] and allow for tuple

pcorpet · 2020-05-23T06:44:50Z

mongomock/aggregate.py

+
+            arr_to_obj = {}
+            for elements in parsed:
+                if isinstance(elements, list):


This one can just be built with arr_to_obj = dict(parsed)

hugo19941994 · 2020-05-23T15:28:27Z

Thanks for the thorough review and your suggestions. I've implemented your requests and squashed my commits together. In addition to your requests I've made the following changes:

Implementation was further refactored
- Used dictionary comprehension to convert {k, v} values to the expected objects
- Dropped the two defs
- Dropped the any and replaced by plain if/elses
I've replaced the TypeErrors with PyMongo's OperationFailure (so that compare_exceptions worked)
Added a bunch of tests to check that tuples work just as well as lists

While checking PyMongo I've noticed that following exception messages can be thrown:

$arrayToObject requires an array of size 2 arrays,found array of size: 3
$arrayToObject requires an object keys of 'k' and 'v'. Found incorrect number of keys:3
$arrayToObject requires an object with keys 'k' and 'v'. Missing either or both keys from: {v: 1, t: "t"}
$arrayToObject requires a consistent input format. Elements mustall be arrays or all be objects. Array was detected, now found: object

All of the messages can be seen in the mongo repo. My implementation doesn't check the reason for an OperationFailure in most cases and uses a generic error message. If you want I can change the code so that the same messages are used, but I wanted to check with you before adding more complexity to the code.

EDIT: It looks like I broke something with Python 2.7. I'll take a look.

pcorpet

Thanks for the neat changes..

Totally OK not to have the same error message, as long as it's the same type. So let's keep it simpler.
I prefer that we don't account for old versions of pymongo so your solution is best.

Please try to fix the python 2.7 version and my 2 little comments on indentation then I'll merge.

pcorpet · 2020-05-23T21:28:47Z

mongomock/aggregate.py

+                return None
+
+            if not isinstance(parsed, (list, tuple)):
+                raise OperationFailure('$arrayToObject requires an array input, '


Please break after OperationFailure(: that helps with cleaner indentation that does not depend on the length of the class name.

pcorpet · 2020-05-23T21:29:26Z

mongomock/aggregate.py

+            if all(isinstance(x, (list, tuple)) and len(x) == 2 for x in parsed):
+                return dict(parsed)
+
+            raise OperationFailure('arrays used with $arrayToObject must contain documents '


Same here, break after OperationFailure(

hugo19941994 · 2020-05-24T08:54:12Z

Python 2.7 should be fixed now. .keys() returns a list in Python 2.7 which can't be directly compared against a set (unlike in python 3).

pcorpet requested changes May 23, 2020

View reviewed changes

Implement $arrayToObject operator

9e7e0e2

pcorpet approved these changes May 24, 2020

View reviewed changes

pcorpet merged commit c15d173 into mongomock:develop May 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement $arrayToObject operator #624

Implement $arrayToObject operator #624

hugo19941994 commented May 6, 2020

codecov bot commented May 6, 2020 •

edited

hugo19941994 commented May 6, 2020

hugo19941994 commented May 6, 2020 •

edited

pcorpet left a comment

pcorpet May 23, 2020

pcorpet May 23, 2020

pcorpet May 23, 2020

pcorpet May 23, 2020

pcorpet May 23, 2020

pcorpet May 23, 2020

pcorpet May 23, 2020

hugo19941994 commented May 23, 2020 •

edited

pcorpet left a comment

pcorpet May 23, 2020

pcorpet May 23, 2020

hugo19941994 commented May 24, 2020

Implement $arrayToObject operator #624

Implement $arrayToObject operator #624

Conversation

hugo19941994 commented May 6, 2020

codecov bot commented May 6, 2020 • edited

Codecov Report

hugo19941994 commented May 6, 2020

hugo19941994 commented May 6, 2020 • edited

pcorpet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugo19941994 commented May 23, 2020 • edited

pcorpet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugo19941994 commented May 24, 2020

codecov bot commented May 6, 2020 •

edited

hugo19941994 commented May 6, 2020 •

edited

hugo19941994 commented May 23, 2020 •

edited