Validate all non-optional :filter: examples from the spec #213

ml-evs · 2020-03-05T17:39:52Z

This PR builds on #205 by scraping the spec for :filter: tags and pushing them through the validator.

Changes:

invoke task to update filter example list (done, but requires manual tweaking of the results unless the spec is updated Tag optional filters in a parseable way OPTIMADE#262)
- provide mechanism to automatically update filter examples based on spec
- provide manual mechanism to skip filters that are tagged as optional but not directly parseable as optional
- add concrete test values to spec examples when validating (i.e. HAS LENGTH value becomes HAS LENGTH 1)
fix the failing queries in Some mandatory filter examples from spec do not work #217
- fix alias_mapper when dealing with nested queries
  - added some tests and tweaks to ResourceMapper to properly test for this
- fix mongo non-compliant queries involving $not and test operator precedence
- fix misleading mongo error when non-int char is passed to LENGTH by updating grammar

CasperWA

Casually looking through this to see what you've been up to - looking good! :)
Found some missing spaces in the query tests, you should just be able to apply the suggestions directly to fix them.
Also a question about making ResourceMapper a meta-class.

tests/server/test_query_params.py

tests/filtertransformers/test_mongo.py

optimade/server/mappers/entries.py

optimade/server/entry_collections.py

ml-evs · 2020-03-09T12:03:24Z

Casually looking through this to see what you've been up to - looking good! :)
Found some missing spaces in the query tests, you should just be able to apply the suggestions directly to fix them.
Also a question about making ResourceMapper a meta-class.

Cheers! There's something going on with the actual mongo tests, I think we may just be testing more queries against mongo than we were previously, so I'll have to set up a better test environment locally to fix that. Mongo has lots of annoying things like {"$size": {"$eq": 1}} being invalid, instead requiring {"$size": 1}.

ml-evs · 2020-03-09T18:22:19Z

This should all work now, some final queries:

should the example filters be run against every endpoint (currently its just structures)?

CasperWA · 2020-03-09T18:28:27Z

This should all work now, some final queries:

should the example filters be run against every endpoint (currently its just structures)?

No. We are testing the filter, which is currently mostly, if not only, applicable to structures.
However, we maybe should introduce simple tests for querying each known field in references.

But on the other hand, this is testing the filter, not the endpoint(s). So I would say it's fine just testing structures.

ml-evs · 2020-03-09T19:05:27Z

But on the other hand, this is testing the filter, not the endpoint(s). So I would say it's fine just testing structures.

Cool, agreed. This needed a couple of extra tweaks after the optimism in my last comment, but this now seems to be working fine. I've summarised the changes in more detail above, but the missing piece over the weekend was adding an int class to the grammar to catch e.g. LENGTH = value from the spec examples, instead returning a parsing error, not a mongo error.

codecov · 2020-03-09T19:06:36Z

Codecov Report

Merging #213 into master will increase coverage by 0.39%.
The diff coverage is 68.81%.

@@            Coverage Diff             @@
##           master     #213      +/-   ##
==========================================
+ Coverage   87.15%   87.55%   +0.39%     
==========================================
  Files          42       43       +1     
  Lines        1892     1912      +20     
==========================================
+ Hits         1649     1674      +25     
+ Misses        243      238       -5

Flag	Coverage Δ
#unittests	`87.55% <68.81%> (+0.39%)`	⬆️

Impacted Files	Coverage Δ
optimade/validator/__init__.py	`10% <0%> (-2.5%)`	⬇️
optimade/server/mappers/links.py	`100% <100%> (ø)`	⬆️
optimade/filtertransformers/mongo.py	`95.45% <100%> (+0.16%)`	⬆️
optimade/server/mappers/entries.py	`97.77% <100%> (ø)`	⬆️
optimade/server/entry_collections.py	`87.68% <100%> (+0.36%)`	⬆️
optimade/server/mappers/references.py	`100% <100%> (ø)`	⬆️
optimade/server/mappers/structures.py	`100% <100%> (ø)`	⬆️
optimade/validator/data/__init__.py	`100% <100%> (ø)`
optimade/validator/validator.py	`67.38% <43.9%> (+3.48%)`	⬆️
... and 1 more

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 b89791d...1fc320b. Read the comment docs.

CasperWA

An impressive PR and finally some more love to the validator 👍

I've left some comments, thoughts, and suggested changes - all more or less minor things.

tests/server/test_query_params.py

tests/server/test_mappers.py

tasks.py

optimade/server/entry_collections.py

optimade/grammar/v0.10.1.lark

CasperWA · 2020-03-09T23:48:28Z

optimade/filtertransformers/mongo.py

@@ -92,6 +92,9 @@ def expression_phrase(self, arg):
            # without NOT
            return arg[0]

+        if list(arg[1].keys()) == ["$or"]:
+            return {"$nor": arg[1]["$or"]}
+
        # with NOT
        # TODO: This implementation probably fails in the case of `"(" expression ")"`


Is this still true? I guess it is.

Yeah, not sure. I don't think the example is valid for the grammar anymore?

I have no idea either. Although the example was put in for this grammar by @fekad I believe?

CasperWA

Final thing(s), then it's good to go on my part 👍 Great work @ml-evs !

optimade/filtertransformers/mongo.py

optimade/server/mappers/entries.py

tests/server/test_query_params.py

ml-evs · 2020-03-10T13:16:23Z

As a final change, I've just added the no cover pragma to the validator client, as this is tested outside of py.test.

CasperWA · 2020-03-10T13:44:09Z

As a final change, I've just added the no cover pragma to the validator client, as this is tested outside of py.test.

Sure - or we should manually run those tests using coverage run?
But maybe we can add this later?

ml-evs · 2020-03-10T14:04:06Z

As a final change, I've just added the no cover pragma to the validator client, as this is tested outside of py.test.

Sure - or we should manually run those tests using coverage run?
But maybe we can add this later?

I'm fine with later...

CasperWA

This is looking good to me - thanks for this @ml-evs !

Please squash your commits into reasonable chunks, and I'll approve for you to merge 👍

- This avoids a user setting up an alias that users a mongo keyword.

CasperWA

Again, many thanks @ml-evs ! Great work.

ml-evs changed the base branch from master to ml-evs/validator_updates March 5, 2020 17:40

ml-evs force-pushed the ml-evs/validator_updates branch from b4808a5 to 4c30c05 Compare March 5, 2020 21:29

ml-evs force-pushed the ml-evs/add_more_validator_queries branch from 26b1ef6 to 32d3501 Compare March 5, 2020 21:29

This was referenced Mar 6, 2020

Test mandatory queries in validator #205

Merged

Some mandatory filter examples from spec do not work #217

Closed

ml-evs changed the title ~~Add all non-optional :filter: examples from the spec~~ Validate all non-optional :filter: examples from the spec Mar 6, 2020

ml-evs force-pushed the ml-evs/add_more_validator_queries branch from 6d4d10a to cd69a9e Compare March 6, 2020 18:23

ml-evs changed the base branch from ml-evs/validator_updates to master March 6, 2020 18:23

ml-evs force-pushed the ml-evs/add_more_validator_queries branch 2 times, most recently from 747117b to 8e7078d Compare March 6, 2020 19:01

CasperWA requested changes Mar 9, 2020

View reviewed changes

ml-evs force-pushed the ml-evs/add_more_validator_queries branch from b83c45c to ac85717 Compare March 9, 2020 19:21

ml-evs marked this pull request as ready for review March 9, 2020 19:25

ml-evs requested a review from CasperWA March 9, 2020 21:23

CasperWA reviewed Mar 9, 2020

View reviewed changes

ml-evs force-pushed the ml-evs/add_more_validator_queries branch 3 times, most recently from f56a42c to e549df4 Compare March 10, 2020 11:17

ml-evs requested a review from CasperWA March 10, 2020 11:18

CasperWA requested changes Mar 10, 2020

View reviewed changes

optimade/filtertransformers/mongo.py Outdated Show resolved Hide resolved

optimade/server/mappers/entries.py Show resolved Hide resolved

tests/server/test_query_params.py Outdated Show resolved Hide resolved

ml-evs requested a review from CasperWA March 10, 2020 13:16

CasperWA reviewed Mar 10, 2020

View reviewed changes

ml-evs force-pushed the ml-evs/add_more_validator_queries branch from 7201edf to d9ca8f8 Compare March 10, 2020 14:30

ml-evs added 12 commits March 10, 2020 14:40

Added validator.data submodule to hold filter examples

a984a23

Renamed ResourceMapper->BaseResourceMapper and added docs

264b1fd

Switched all invoke tasks to use absolute paths

51262f8

Added invoke task to generate filter examples from spec

d2e75a7

Improved validator output clarity and configurability

3674d0e

Added more challenging query and filter tests

a996f00

Added some tests for the ResourceMapper directly

3f7616a

Fixed chained $or->$not queries in mongo with $nor

06f990b

Added signed_int to grammar for use in length queries

87281c0

Use inline_args for number and signed int

e9ccf09

Check mapper aliases are valid for mongo

aaa80d2

- This avoids a user setting up an alias that users a mongo keyword.

Generalised alias mapper for all nested filter structures

1fc320b

ml-evs force-pushed the ml-evs/add_more_validator_queries branch from d9ca8f8 to 1fc320b Compare March 10, 2020 14:40

CasperWA approved these changes Mar 10, 2020

View reviewed changes

ml-evs merged commit 5a5e903 into master Mar 10, 2020

ml-evs mentioned this pull request Mar 10, 2020

Validator only validates what we have working, not what is required by the spec #182

Closed

CasperWA mentioned this pull request Mar 10, 2020

Add more tests #104

Closed

10 tasks

ml-evs added this to In progress in Road to optimade-python-tools 1.0 via automation Mar 10, 2020

ml-evs added enhancement New feature or request validator Related to the OPTIMADE validator labels Mar 10, 2020

This was referenced Mar 11, 2020

Tag more filter examples as optional Materials-Consortia/OPTIMADE#263

Merged

v0.7.0 release #228

Merged

Update OPTIMADE capitalisation #233

Merged

ml-evs moved this from In progress to Done in Road to optimade-python-tools 1.0 Mar 31, 2020

CasperWA deleted the ml-evs/add_more_validator_queries branch June 2, 2020 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate all non-optional :filter: examples from the spec #213

Validate all non-optional :filter: examples from the spec #213

ml-evs commented Mar 5, 2020 •

edited

CasperWA left a comment •

edited

ml-evs commented Mar 9, 2020

ml-evs commented Mar 9, 2020 •

edited

CasperWA commented Mar 9, 2020

ml-evs commented Mar 9, 2020

codecov bot commented Mar 9, 2020 •

edited

CasperWA left a comment

CasperWA Mar 9, 2020

ml-evs Mar 10, 2020

CasperWA Mar 10, 2020

CasperWA left a comment

ml-evs commented Mar 10, 2020

CasperWA commented Mar 10, 2020 •

edited

ml-evs commented Mar 10, 2020

CasperWA left a comment •

edited

CasperWA left a comment

Validate all non-optional :filter: examples from the spec #213

Validate all non-optional :filter: examples from the spec #213

Conversation

ml-evs commented Mar 5, 2020 • edited

CasperWA left a comment • edited

Choose a reason for hiding this comment

ml-evs commented Mar 9, 2020

ml-evs commented Mar 9, 2020 • edited

CasperWA commented Mar 9, 2020

ml-evs commented Mar 9, 2020

codecov bot commented Mar 9, 2020 • edited

Codecov Report

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA Mar 9, 2020

Choose a reason for hiding this comment

ml-evs Mar 10, 2020

Choose a reason for hiding this comment

CasperWA Mar 10, 2020

Choose a reason for hiding this comment

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Mar 10, 2020

CasperWA commented Mar 10, 2020 • edited

ml-evs commented Mar 10, 2020

CasperWA left a comment • edited

Choose a reason for hiding this comment

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Mar 5, 2020 •

edited

CasperWA left a comment •

edited

ml-evs commented Mar 9, 2020 •

edited

codecov bot commented Mar 9, 2020 •

edited

CasperWA commented Mar 10, 2020 •

edited

CasperWA left a comment •

edited