#950 New: Support querying of modules/components with multiple state value filtering
Merged 5 years ago by mprahl. Opened 5 years ago by fivaldi.
fivaldi/fm-orchestrator fivaldi_api_improvements  into  master

file modified
+5 -2
@@ -498,7 +498,8 @@ 

  - ``owner``

  - ``rebuild_strategy``

  - ``scmurl``

- - ``state`` (can be the state name or the state ID e.g. ``state=done``)

+ - ``state`` (Can be the state name or the state ID e.g. ``state=done``. This

+   parameter can be given multiple times, in which case or-ing will be used.)

  - ``state_reason``

  - ``stream``

  - ``submitted_after`` (Zulu ISO 8601 format e.g. ``submitted_after=2016-08-22T09:40:07Z``)
@@ -662,7 +663,9 @@ 

  - ``package``

  - ``ref``

  - ``scmurl``

- - ``state``

+ - ``state`` (Can be the state name or the state ID. Koji states are used

+   for resolving to IDs. This parameter can be given multiple times, in which

+   case or-ing will be used.)

  - ``state_reason``

  - ``tagged`` (boolean e.g. "true" or "false")

  - ``tagged_in_final`` (boolean e.g. "true" or "false")

@@ -134,6 +134,9 @@ 

      """

      search_query = dict()

      for key in request.args.keys():

+         # Search by state will be handled separately

+         if key == 'state':

+             continue

          # Only filter on valid database columns

          if key in models.ComponentBuild.__table__.columns.keys():

              if isinstance(models.ComponentBuild.__table__.columns[key].type, sqlalchemy_boolean):
@@ -141,10 +144,12 @@ 

              else:

                  search_query[key] = flask_request.args[key]

  

-     state = flask_request.args.get('state', None)

-     if state:

+     # Multiple states can be supplied => or-ing will take place

+     states = flask_request.args.getlist('state')

+     search_states = []

+     for state in states:

          if state.isdigit():

-             search_query['state'] = state

+             search_states.append(state)

          else:

              try:

                  import koji
@@ -152,9 +157,9 @@ 

                  raise ValidationError('Cannot filter by state names because koji isn\'t installed')

  

              if state.upper() in koji.BUILD_STATES:

-                 search_query['state'] = koji.BUILD_STATES[state.upper()]

+                 search_states.append(koji.BUILD_STATES[state.upper()])

              else:

-                 raise ValidationError('An invalid state was supplied')

+                 raise ValidationError('Invalid state was supplied: %s' % state)

  

      # Allow the user to specify the module build ID with a more intuitive key name

      if 'module_build' in flask_request.args:
@@ -164,6 +169,8 @@ 

  

      if search_query:

          query = query.filter_by(**search_query)

+     if search_states:

+         query = query.filter(models.ComponentBuild.state.in_(search_states))

  

      query = _add_order_by_clause(flask_request, query, models.ComponentBuild)

  
@@ -186,20 +193,24 @@ 

          if key not in special_columns and key in models.ModuleBuild.__table__.columns.keys():

              search_query[key] = flask_request.args[key]

  

-     state = flask_request.args.get('state', None)

-     if state:

+     # Multiple states can be supplied => or-ing will take place

+     states = flask_request.args.getlist('state')

+     search_states = []

+     for state in states:

          if state.isdigit():

-             search_query['state'] = state

+             search_states.append(state)

          else:

              if state in models.BUILD_STATES:

-                 search_query['state'] = models.BUILD_STATES[state]

+                 search_states.append(models.BUILD_STATES[state])

              else:

-                 raise ValidationError('An invalid state was supplied')

+                 raise ValidationError('Invalid state was supplied: %s' % state)

  

      query = models.ModuleBuild.query

  

      if search_query:

          query = query.filter_by(**search_query)

+     if search_states:

+         query = query.filter(models.ModuleBuild.state.in_(search_states))

  

      # This is used when filtering the date request parameters, but it is here to avoid recompiling

      utc_iso_datetime_regex = re.compile(

@@ -442,6 +442,16 @@ 

          data = json.loads(rv.data)

          assert data['meta']['total'] == 1

  

+     def test_query_component_builds_filter_state(self):

+         rv = self.client.get('/module-build-service/1/component-builds/?state=3')

+         data = json.loads(rv.data)

+         assert data['meta']['total'] == 2

+ 

+     def test_query_component_builds_filter_multiple_states(self):

+         rv = self.client.get('/module-build-service/1/component-builds/?state=3&state=1')

+         data = json.loads(rv.data)

+         assert data['meta']['total'] == 12

+ 

      def test_query_builds_filter_name(self):

          rv = self.client.get('/module-build-service/1/module-builds/?name=nginx')

          data = json.loads(rv.data)
@@ -500,6 +510,12 @@ 

          data = json.loads(rv.data)

          assert data['meta']['total'] == 2

  

+     def test_query_builds_filter_multiple_states(self):

+         rv = self.client.get(

+             '/module-build-service/1/module-builds/?state=3&state=1')

+         data = json.loads(rv.data)

+         assert data['meta']['total'] == 4

+ 

      def test_query_builds_two_filters(self):

          rv = self.client.get('/module-build-service/1/module-builds/?owner=Moe%20Szyslak'

                               '&modified_after=2016-09-03T11:35:00Z')

no initial comment

There'll be some failing test for sure. I was unable to test it locally using the Dockerfile-tests. tox doesn't work, hand-made virtualenv doesn't work either. What's the right way to run tests on MBS?

On the master branch, running the tests as described in the docs worked for me. It also worked in CentOS CI the last time a commit was merged into master.

These doc changes should ideally be in a separate commit.

Edit: I mean the doc changes that are unrelated to this PR should be in a separate commit at the very least.

Why not add the following to the for loop that populates the search_query?

if key == 'state':
    continue

This should probably default to an empty list instead of None.

If you follow my advice by defaulting states to an empty list, then this will if statement can be removed since the for loop will not execute if states is an empty list.

Please use [] instead of list(). It's more readable and faster.

You should tell the user which state was invalid.

If the query with state=3 and the query with state=3&state=5 both yield two results, then what are you actually testing here since the behavior didn't change?

@fivaldi i am not sure we need this. The question in the issue was if we can query the the mbs by state not by multiple states at the same time. Also the params are now confusing a little bit. Just do 2 requests if you need two sets of diff states. I would close this.
@rbean @acorvin @mprahl wdyt?

@mcurlej I'd leave it up to @fivaldi if he wants to put the effort to fix his PR or not. Although the feature isn't useful now, it could be in the future.

I don't think we should spend much time working on adding this feature right now. I think Martin is right that the original request to be able to query by a single state is already possible today. We can revisit this later on once we've removed the dependency on PDC everywhere else.

rebased onto a8fba2d6b670aec81a72c8f5fdcc0da28b4bffcc

5 years ago

rebased onto 8a13909eddf60f6cad34d15116465260d75e2f92

5 years ago

rebased onto ae4f2fe6feef4faf7dafddd952b6d7ad9d8cbc80

5 years ago

I addressed all comments - please check. Tests ran successfully. After revising, consider merging the PR or dropping it accto the discussion above.

The unrelated README patch is proposed here: https://paste.fedoraproject.org/paste/Rd0QgWSEPk9UnDsnJZG1cA/raw

The new issue arising from the inconvenient/undescribed testing within the docker env is at #951.

rebased onto f1fc7ed

5 years ago

:thumbsup: @fivaldi can you propose a separate PR with the README patch?

Commit 764c467 fixes this pull-request

Pull-Request has been merged by mprahl

5 years ago

Pull-Request has been merged by mprahl

5 years ago