Add contains and contains_any filtering #1592

Natim · 2018-04-16T13:08:39Z

Fixes #343

Add documentation.
Add tests.
Add a changelog entry.
Add your name in the contributors file.
If you changed the HTTP API, update the API_VERSION constant and add an API changelog entry in the docs

Natim · 2018-04-16T13:09:18Z

kinto/core/storage/memory.py

@@ -336,6 +342,8 @@ def apply_filters(records, filters):
        COMPARISON.IN: operator.contains,
        COMPARISON.EXCLUDE: lambda x, y: not operator.contains(x, y),
        COMPARISON.LIKE: lambda x, y: re.search(y, x, re.IGNORECASE),
+        COMPARISON.CONTAINS_ALL: contains_all_filtering,
+        COMPARISON.CONTAINS: lambda record_value, search_term: set(record_value[1]).intersection(set(search_term[1]))


I'd like to understand where this [1] comes from it is a bit unclear to me right now.

Fixes #343 Co-authored-by: Julien Lebunetel <julien@lebunetel.com>

Co-authored-by: Julien Lebunetel <julien@lebunetel.com>

@jlebunetel

Paired with @jlebunetel Co-authored-by: Julien Lebunetel <julien@lebunetel.com>

glasserc · 2018-04-16T15:36:51Z

This is kind of bikesheddy, but I think contains_foo=x,y should mean "the foo attribute contains ["x", "y"]" (i.e. what you called contains_all here). If you want to write "the foo attribute overlaps with ["x", "y"]", I think you should write intersect or contains_any.

Natim · 2018-04-16T15:50:31Z

If you have any idea why pip is failling, I would be interested in knowing.

Edited: 503 Backend is unhealthy

glasserc · 2018-04-16T16:02:50Z

kinto/core/storage/postgresql/__init__.py

+                                                    filtr.operator.value)
+                # Thanks https://dba.stackexchange.com/a/157186
+                cond = "TRANSLATE(({})\:\:jsonb\:\:text, '[]','{}')\:\:{}[] {} :{}".format(
+                    sql_field, '{}', array_type, sql_operator, value_holder)


I think this is all way too complicated and I am not crazy about the use of string processing to convert from one type to another. I think you should be converting the filter value to JSONB. This way we can support checking for fields containing complex objects like [1, 2] (in a field like [[1,2], [5, 9]]). There's no way to do "overlap" testing with this type, but I think that's OK -- do we really need contains_any at this point in time or did you just figure it was easy to implement at the same time?

do we really need contains_any

Yes I do

I think this is all way too complicated and I am not crazy about the use of string processing to convert from one type to another.

That's the only way I found so far to be able to add it in the WHERE clause.

We could use jsonb_to_text_array but it would be have to be a JOIN.

If you find another way of doing it, I would be pleased to use it.

It was also a bit complex to use the SQLAlchemy execute parameters with an array.

glasserc · 2018-04-16T16:07:09Z

Pip failing might not be your fault... on mozilla/pyjexl#13 I'm getting "too many redirects".

glasserc · 2018-04-16T19:20:55Z

tox.ini

@@ -9,7 +9,6 @@ commands =
    py.test --cov-report term-missing --cov-branch --cov-fail-under 100 --cov kinto {posargs}
 deps =
    -rdev-requirements.txt
-    -crequirements.txt


What was this about?

Yeah maybe we should keep it.

leplatrem · 2018-04-16T20:09:40Z

kinto/core/storage/postgresql/__init__.py

+                # However, it does support Postgres arrays of any
+                # type. Assume that the referenced field is a JSON
+                # array and convert it to a Postgres array.
+                # N.B. This won't work on JSON objects.


What does it mean "won't work" ? "will always be false" ?

If you've got an array of json objects it won't work. The array should be only strings or int.

Oh I just understood your remark, I guess we should add a test to make sure of it.

leplatrem · 2018-04-16T20:11:21Z

docs/api/index.rst

+'''''''''''''''''
+
+- New `contains_` and `contains_any` filter operators (fixes #343)
+


Could you please add them to https://github.com/Kinto/kinto/blob/master/docs/api/1.x/filtering.rst ?

Yes @jlebunetel is working on the documentation part 👍

leplatrem · 2018-04-16T20:12:34Z

CHANGELOG.rst

+
+**New feature**
+
+- Add ``contains`` and ``contains_any`` filtering (Fixes #343).


nitpick: usually when we add new features to the HTTP API we highlight it as such (see older entries) :)

glasserc · 2018-04-17T15:32:00Z

kinto/core/storage/postgresql/__init__.py

+                # However, it does support Postgres arrays of any
+                # type. Assume that the referenced field is a JSON
+                # array and convert it to a Postgres array.
+                # N.B. This won't work on JSON objects and will never match.


No, that's not right.

http --auth 'user:user' PUT 'http://localhost:8888/v1/buckets/default/collections/def/records/r1' 'data:={"user": {"first": "Ethan", "id": 1234}, "aliases": [{"ll": "ls -l"}, {"gti": "git"}, {"vi": "vim"}]}' http --auth 'user:user' 'http://localhost:8888/v1/buckets/default/collections/def/records?contains_any_aliases=[{"ll":"ls -l"}]' # matches http --auth 'user:user' 'http://localhost:8888/v1/buckets/default/collections/def/records?contains_any_aliases=[{"sl":"ls -l"}]' #doesn't match http --auth 'user:user' 'http://localhost:8888/v1/buckets/default/collections/def/records?contains_any_user={"id":1234}' # 503

Compare with contains:

http --auth 'user:user' 'http://localhost:8888/v1/buckets/default/collections/def/records?contains_aliases=[{"ll":"ls -l"}]' # matches http --auth 'user:user' 'http://localhost:8888/v1/buckets/default/collections/def/records?contains_aliases=[{"sl":"ls -l"}]' #doesn't match http --auth 'user:user' 'http://localhost:8888/v1/buckets/default/collections/def/records?contains_user={"id":1234}' # matches

N.B. these behaviors may not match in the memory backend.

glasserc · 2018-04-17T15:35:40Z

docs/api/1.x/filtering.rst

+* ``/collection?contains_colors=[red,blue]``
+
+  Matches any record whose ``colors`` array field contains ``red`` and
+  ``blue`` elements.


This doesn't work.

http --auth 'user:user' POST 'http://localhost:8888/v1/buckets/default/collections/def/records' data:='{"name": "Superman", "colors": ["red", "blue"]}' http --auth 'user:user' GET 'http://localhost:8888/v1/buckets/default/collections/def/records?contains_colors=[red,blue]'

Doesn't match..

glasserc · 2018-04-17T15:35:59Z

docs/api/1.x/filtering.rst

+* ``/collection?contains_any_colors=[red,blue]``
+
+  Matches any record whose ``colors`` array field contains ``red`` or
+  ``blue`` elements.


The same problem above is present here.

glasserc · 2018-04-17T15:37:20Z

docs/api/1.x/filtering.rst

+  Matches any records whose ``field`` array field contains ``value``. Values can
+  be an integer, a string, a list of integers or a list of strings. In case of
+  values are a list, only matches records whose field contains all the values
+  listed.


What happens if field is not an array?

This can also match objects, but I guess it doesn't matter because almost any object contains you could consider doing is better expressed as a bunch of equality filters?

I guess we should raise a 400 for objects here.

glasserc · 2018-04-17T15:39:20Z

tests/core/resource/test_filter.py

+        result = self.resource.collection_get()
+        values = [item['fib'] for item in result['data']]
+        for value in values:
+            assert 3 in value or 5 in value


These tests are inaccurately named.

Whould test_contains_with_string_select_records_with_color be better? Any suggestion?

Natim commented Apr 16, 2018

View reviewed changes

Natim force-pushed the 343-add-contains-filtering branch 2 times, most recently from 0370823 to 84b21dc Compare April 16, 2018 15:09

Rémy HUBSCHER and others added 4 commits April 16, 2018 17:10

Add contains and contains_all filters.

26d64ed

Fixes #343 Co-authored-by: Julien Lebunetel <julien@lebunetel.com>

Add a CHANGELOG.

19a78df

Co-authored-by: Julien Lebunetel <julien@lebunetel.com>

Ignore schwartzian_transform for CONTAINS and CONTAINS_ALL filters.

e519fc1

Co-authored-by: Julien Lebunetel <julien@lebunetel.com>

Add postgresql support.

a886fcf

Paired with @jlebunetel Co-authored-by: Julien Lebunetel <julien@lebunetel.com>

Natim force-pushed the 343-add-contains-filtering branch from 84b21dc to a886fcf Compare April 16, 2018 15:11

Rémy HUBSCHER added 3 commits April 16, 2018 17:17

Add API changelog

55e2745

Fix tox.

1027b16

Import sqlachemy

9ca38f5

Rémy HUBSCHER added 3 commits April 16, 2018 17:39

Add contains and contains_all support for INT arrays

8e532a0

Support missing fields.

ce9d421

@glasserc review.

999762e

Natim requested a review from leplatrem April 16, 2018 15:51

Natim changed the title ~~343 add contains filtering~~ Add contains and contains_any filtering Apr 16, 2018

Add a test for contains_any and missing fields.

71bda88

glasserc reviewed Apr 16, 2018

View reviewed changes

Rémy HUBSCHER and others added 4 commits April 16, 2018 18:14

Add querystring tests.

5d7e890

Simplify the CONTAINS case

bf42841

Simplify CONTAINS_ANY

96b742d

No need for this sqlalchemy.text thing

f451566

glasserc force-pushed the 343-add-contains-filtering branch from 506a3b1 to f451566 Compare April 16, 2018 19:18

Tiny grammar fix

71ef509

glasserc reviewed Apr 16, 2018

View reviewed changes

leplatrem reviewed Apr 16, 2018

View reviewed changes

@leplatrem review.

1429235

Rémy HUBSCHER and others added 2 commits April 17, 2018 10:52

Ignore pytest_cache

1ea67d4

Add support of contains_ and contains_any

bc60b17

leplatrem approved these changes Apr 17, 2018

View reviewed changes

Merge branch 'master' into 343-add-contains-filtering

58cb8a1

Natim merged commit a9b8d07 into master Apr 17, 2018

Natim deleted the 343-add-contains-filtering branch April 17, 2018 12:25

glasserc reviewed Apr 17, 2018

View reviewed changes

Natim mentioned this pull request Apr 23, 2018

contains and contains_any @glasserc review. #1604

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add contains and contains_any filtering #1592

Add contains and contains_any filtering #1592

Natim commented Apr 16, 2018 •

edited

Loading

Natim Apr 16, 2018 •

edited

Loading

glasserc commented Apr 16, 2018

Natim commented Apr 16, 2018 •

edited

Loading

glasserc Apr 16, 2018

Natim Apr 16, 2018

Natim Apr 16, 2018

glasserc commented Apr 16, 2018

glasserc Apr 16, 2018

Natim Apr 17, 2018

leplatrem Apr 16, 2018

Natim Apr 17, 2018

Natim Apr 17, 2018

leplatrem Apr 16, 2018

Natim Apr 17, 2018

leplatrem Apr 16, 2018

glasserc Apr 17, 2018 •

edited by Natim

Loading

glasserc Apr 17, 2018 •

edited by Natim

Loading

glasserc Apr 17, 2018

glasserc Apr 17, 2018

Natim Apr 18, 2018

glasserc Apr 17, 2018

Natim Apr 23, 2018

		'''''''''''''''''

		- New `contains_` and `contains_any` filter operators (fixes #343)


		New feature

		- Add ``contains`` and ``contains_any`` filtering (Fixes #343).

Add contains and contains_any filtering #1592

Add contains and contains_any filtering #1592

Conversation

Natim commented Apr 16, 2018 • edited Loading

Natim Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

glasserc commented Apr 16, 2018

Natim commented Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasserc commented Apr 16, 2018

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

glasserc Apr 17, 2018 • edited by Natim Loading

Choose a reason for hiding this comment

glasserc Apr 17, 2018 • edited by Natim Loading

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

Natim commented Apr 16, 2018 •

edited

Loading

Natim Apr 16, 2018 •

edited

Loading

Natim commented Apr 16, 2018 •

edited

Loading

glasserc Apr 17, 2018 •

edited by Natim

Loading

glasserc Apr 17, 2018 •

edited by Natim

Loading