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

Fix rest api filtering #3526

Merged
merged 5 commits into from Sep 5, 2017

Conversation

Projects
None yet
2 participants
@jangmarker
Contributor

jangmarker commented Aug 15, 2017

Fixes queries like:
/api/v2/builds?builderid__eq=1&builderid__eq=2

This would have yielded an empty result with the previous code.

Contributor Checklist:

  • I have updated the unit tests
  • I have created a file in the master/buildbot/newsfragment directory (and read the README.txt in that directory)
  • I have updated the appropriate documentation
    code is fixed to match the documentation
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 15, 2017

Codecov Report

Merging #3526 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3526      +/-   ##
==========================================
+ Coverage   88.29%   88.36%   +0.06%     
==========================================
  Files         323      323              
  Lines       33878    34047     +169     
==========================================
+ Hits        29912    30085     +173     
+ Misses       3966     3962       -4
Impacted Files Coverage Δ
master/buildbot/data/resultspec.py 91.2% <100%> (+0.21%) ⬆️
master/buildbot/process/builder.py 84.54% <0%> (+1.36%) ⬆️
master/buildbot/process/build.py 94.12% <0%> (+1.37%) ⬆️
master/buildbot/worker/openstack.py 97.4% <0%> (+1.45%) ⬆️

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 e1cbfd9...624ec02. Read the comment docs.

codecov bot commented Aug 15, 2017

Codecov Report

Merging #3526 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3526      +/-   ##
==========================================
+ Coverage   88.29%   88.36%   +0.06%     
==========================================
  Files         323      323              
  Lines       33878    34047     +169     
==========================================
+ Hits        29912    30085     +173     
+ Misses       3966     3962       -4
Impacted Files Coverage Δ
master/buildbot/data/resultspec.py 91.2% <100%> (+0.21%) ⬆️
master/buildbot/process/builder.py 84.54% <0%> (+1.36%) ⬆️
master/buildbot/process/build.py 94.12% <0%> (+1.37%) ⬆️
master/buildbot/worker/openstack.py 97.4% <0%> (+1.45%) ⬆️

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 e1cbfd9...624ec02. Read the comment docs.

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Aug 15, 2017

Member

good catch. This needs unit tests!

does that also fix #3527 ?

Member

tardyp commented Aug 15, 2017

good catch. This needs unit tests!

does that also fix #3527 ?

@jangmarker

This comment has been minimized.

Show comment
Hide comment
@jangmarker

jangmarker Aug 16, 2017

Contributor

I've added unit tests, however, I haven't found a usage of the database contains operator - the tags property in /builders/ is not queried from the database, it seems. Maybe I've been overlooking something.

It doesn't fix #3527, in fact I discovered #3527 while working on this. For a given list property of a database object the contains operator requires all values given in the REST call to be in that list. This is a sensible query, but a query where at least one of the values given in the REST call have to be in the list property is not possible at the moment (at least I haven't found a way).

Contributor

jangmarker commented Aug 16, 2017

I've added unit tests, however, I haven't found a usage of the database contains operator - the tags property in /builders/ is not queried from the database, it seems. Maybe I've been overlooking something.

It doesn't fix #3527, in fact I discovered #3527 while working on this. For a given list property of a database object the contains operator requires all values given in the REST call to be in that list. This is a sensible query, but a query where at least one of the values given in the REST call have to be in the list property is not possible at the moment (at least I haven't found a way).

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Aug 16, 2017

Member

Hi @jangmarker the Rest API filtering is featured around the needs for the UI.
I think nobody actually looked at the plural case for contains.

If you have usecases, I think it makes more sense to have the contains operator have a default 'or' behaviour. It better matches the behaviour for 'eq'.
We could add a xcontains operator, which has a 'and' plural behaviour.

Member

tardyp commented Aug 16, 2017

Hi @jangmarker the Rest API filtering is featured around the needs for the UI.
I think nobody actually looked at the plural case for contains.

If you have usecases, I think it makes more sense to have the contains operator have a default 'or' behaviour. It better matches the behaviour for 'eq'.
We could add a xcontains operator, which has a 'and' plural behaviour.

@tardyp

fix the contain behaviour

@jangmarker jangmarker referenced this pull request Aug 16, 2017

Merged

Change REST API's __contains to OR behavior #3531

3 of 3 tasks complete
@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Aug 21, 2017

Member

@jangmarker I think you wanted to rebase this PR?

Member

tardyp commented Aug 21, 2017

@jangmarker I think you wanted to rebase this PR?

@jangmarker

This comment has been minimized.

Show comment
Hide comment
@jangmarker

jangmarker Aug 23, 2017

Contributor

Yes, sorry, I didn't make it before vacation. My response times will be rather slow until September.

Contributor

jangmarker commented Aug 23, 2017

Yes, sorry, I didn't make it before vacation. My response times will be rather slow until September.

jangmarker added some commits Aug 15, 2017

REST API: Make plural filter operators work with Alchemy
Fixes queries like:
/api/v2/builds?builderid__eq=1&builderid__eq=2

This would have yielded an empty result with the previous code.
@tardyp

tardyp approved these changes Sep 5, 2017

@tardyp tardyp merged commit d4218cc into buildbot:master Sep 5, 2017

31 checks passed

bb Build done.
Details
bb/coverage/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/coverage/3.5/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/docs/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/flake8/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/flake8/3.5/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/isort/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/js/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/pylint/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/smokes/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/smokes/3.5/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:14.0.2/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:15.4.0/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:15.5.0/sqla:0.8.0/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:15.5.0/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:latest/sqla:latest/db:mysql+mysqldb://travis@127.0.0.1/bbtest Build done.
Details
bb/trial/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:latest/sqla:latest/db:sqlite:////tmp/test_db.sqlite Build done.
Details
bb/trial/3.4/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/3.6/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/trial_worker/2.6/tw:14.0.2/sqla:latest/db:sqlite:// Build done.
Details
bb/trial_worker/2.6/tw:15.4.0/sqla:latest/db:sqlite:// Build done.
Details
bb/trial_worker/2.7/tw:10.2.0/sqla:latest/db:sqlite:// Build done.
Details
bb/trial_worker/2.7/tw:11.1.0/sqla:latest/db:sqlite:// Build done.
Details
bb/trial_worker/2.7/tw:12.2.0/sqla:latest/db:sqlite:// Build done.
Details
bb/trial_worker/2.7/tw:13.2.0/sqla:latest/db:sqlite:// Build done.
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 88.29%)
Details
codecov/project 88.36% (+0.06%) compared to e1cbfd9
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jangmarker jangmarker referenced this pull request Sep 7, 2017

Merged

Builders UI: Load builds based on tags filter #3590

1 of 1 task complete

@jangmarker jangmarker deleted the jangmarker:fix-rest-api-filtering branch Sep 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment