Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate list LENGTH queries with validator #1637

Merged
merged 3 commits into from
May 18, 2023

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented May 16, 2023

As above.

Also tweaks the test for bad versions by hitting /v1234/info instead of a landing page.

@ml-evs ml-evs added enhancement New feature or request validator Related to the OPTIMADE validator labels May 16, 2023
@ml-evs ml-evs requested review from CasperWA and JPBergsma as code owners May 16, 2023 15:27
@ml-evs ml-evs force-pushed the ml-evs/add_length_validation branch from 9ee9c08 to 581e8e9 Compare May 16, 2023 15:27
Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not see anything wrong with this code, but we have to at least prevent the triggering of the error we are currently getting for LENGTH queries on the structure features. Although, I think it would be better if we could support the LENGTH operator in a more universal manner. For MongoDB this should be possible with the size operator:
db.structures.find( {"property_name": { $size: 3 } } )

@ml-evs
Copy link
Member Author

ml-evs commented May 17, 2023

I did not see anything wrong with this code, but we have to at least prevent the triggering of the error we are currently getting for LENGTH queries on the structure features. Although, I think it would be better if we could support the LENGTH operator in a more universal manner. For MongoDB this should be possible with the size operator: db.structures.find( {"property_name": { $size: 3 } } )

We already do this generically for MongoDB but Elastic is causing the problem, simply because structure_features isn't really indexed as a list field. We should at least get it to return a nice error (501 Not Implemented) and probably allow that through the validator.

@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Merging #1637 (c86b126) into master (1713044) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1637   +/-   ##
=======================================
  Coverage   91.04%   91.04%           
=======================================
  Files          74       74           
  Lines        4578     4580    +2     
=======================================
+ Hits         4168     4170    +2     
  Misses        410      410           
Flag Coverage Δ
project 91.04% <100.00%> (+<0.01%) ⬆️
validator 90.93% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/validator/config.py 100.00% <ø> (ø)
optimade/validator/validator.py 83.75% <100.00%> (+0.05%) ⬆️

@ml-evs ml-evs force-pushed the ml-evs/add_length_validation branch from 11429ec to c86b126 Compare May 18, 2023 12:59
@ml-evs
Copy link
Member Author

ml-evs commented May 18, 2023

If you're good with this now @JPBergsma I think I will merge and release to get this up on the dashboard!

@ml-evs ml-evs merged commit e5e7fc2 into master May 18, 2023
@ml-evs ml-evs deleted the ml-evs/add_length_validation branch May 18, 2023 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request validator Related to the OPTIMADE validator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants