#748 Default to reverse ordering by ID in APIs
Closed 6 years ago by mprahl. Opened 6 years ago by ncoghlan.
ncoghlan/fm-orchestrator issue-739-reverse-default-api-ordering  into  master

file modified
+30 -24
@@ -404,6 +404,34 @@ 

      return pagination_data

  

  

+ def _add_order_by_clause(flask_request, query, column_source):

+     """

+     Orders the given SQLAlchemy query based on the GET arguments provided

+     :param flask_request: a Flask request object

+     :param query: a SQLAlchemy query object

+     :param column_source: a SQLAlchemy database model

+     :return: a SQLAlchemy query object

+     """

+     colname = "id"

+     descending = True

+     order_desc_by = flask_request.args.get("order_desc_by", None)

+     if order_desc_by:

+         colname = order_desc_by

+     else:

+         order_by = flask_request.args.get("order_by", None)

+         if order_by:

+             colname = order_by

+             descending = False

+ 

+     column = getattr(column_source, colname, None)

+     if not column:

+         raise ValidationError('An invalid order_by or order_desc_by key '

+                               'was supplied')

+     if descending:

+         column = column.desc()

+     return query.order_by(column)

+ 

+ 

  def filter_component_builds(flask_request):

      """

      Returns a flask_sqlalchemy.Pagination object based on the request parameters
@@ -433,18 +461,7 @@ 

      if search_query:

          query = query.filter_by(**search_query)

  

-     # Order the results by any column in the ModuleBuild table but default to id.

-     order_by = flask_request.args.get('order_by', 'id')

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

-     if order_by or order_desc_by:

-         column = getattr(models.ComponentBuild, order_desc_by or order_by, None)

-         if column:

-             if order_desc_by:

-                 column = column.desc()

-             query = query.order_by(column)

-         else:

-             raise ValidationError('An invalid order_by or order_desc_by key '

-                                   'was supplied')

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

  

      page = flask_request.args.get('page', 1, type=int)

      per_page = flask_request.args.get('per_page', 10, type=int)
@@ -507,18 +524,7 @@ 

                  elif context == 'before':

                      query = query.filter(column <= item_datetime)

  

-     # Order the results by any column in the ModuleBuild table.

-     order_by = flask_request.args.get("order_by", None)

-     order_desc_by = flask_request.args.get("order_desc_by", None)

-     if order_by or order_desc_by:

-         column = getattr(models.ModuleBuild, order_desc_by or order_by, None)

-         if column:

-             if order_desc_by:

-                 column = column.desc()

-             query = query.order_by(column)

-         else:

-             raise ValidationError('An invalid order_by or order_desc_by key '

-                                   'was supplied')

+     query = _add_order_by_clause(flask_request, query, models.ModuleBuild)

  

      page = flask_request.args.get('page', 1, type=int)

      per_page = flask_request.args.get('per_page', 10, type=int)

file modified
+26 -55
@@ -225,74 +225,45 @@ 

  

      def test_query_builds(self):

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

-         item = json.loads(rv.data)['items'][0]

-         self.assertEquals(item['id'], 1)

-         self.assertEquals(item['name'], 'nginx')

-         self.assertEquals(item['owner'], 'Moe Szyslak')

-         self.assertEquals(item['state'], 3)

-         self.assertDictEqual(item['tasks'], {

-             'rpms': {

-                 'module-build-macros': {

-                     'task_id': 12312321,

-                     'state': 1,

-                     'state_reason': None,

-                     'nvr': 'module-build-macros-01-1.module_nginx_1_2',

-                 },

-                 'nginx': {

-                     'task_id': 12312345,

-                     'state': 1,

-                     'state_reason': None,

-                     'nvr': 'nginx-1.10.1-2.module_nginx_1_2',

-                 },

-             },

-         })

-         self.assertEquals(item['time_completed'], '2016-09-03T11:25:32Z')

-         self.assertEquals(item['time_modified'], '2016-09-03T11:25:32Z')

-         self.assertEquals(item['time_submitted'], '2016-09-03T11:23:20Z')

- 

-     def test_query_builds_not_verbose(self):

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

          items = json.loads(rv.data)['items']

          expected = [

              {

-                 'id': 1,

-                 'koji_tag': 'module-nginx-1.2',

-                 'name': 'nginx',

-                 'owner': 'Moe Szyslak',

-                 'scmurl': ('git://pkgs.domain.local/modules/nginx?#ba95886c7a443b36a9ce31abda1f9b'

-                            'ef22f2f8c9'),

-                 'state': 3,

-                 'state_name': 'done',

+                 'id': 30,

+                 'koji_tag': None,

+                 'name': 'testmodule',

+                 'owner': 'some_other_user',

+                 'scmurl': 'git://pkgs.domain.local/modules/testmodule?#ca95886c7a443b36a9ce31abda1f9bef22f2f8c9',

+                 'state': 1,

+                 'state_name': 'wait',

                  'state_reason': None,

-                 'stream': '1',

+                 'stream': '4.3.43',

                  'tasks': {

                      'rpms': {

                          'module-build-macros': {

-                             'nvr': 'module-build-macros-01-1.module_nginx_1_2',

+                             'nvr': 'module-build-macros-01-1.module_postgresql_1_2',

                              'state': 1,

                              'state_reason': None,

-                             'task_id': 12312321

+                             'task_id': 47384002

                          },

-                         'nginx': {

-                             'nvr': 'nginx-1.10.1-2.module_nginx_1_2',

-                             'state': 1,

+                         'rubygem-rails': {

+                             'nvr': 'postgresql-9.5.3-4.module_postgresql_1_2',

+                             'state': 3,

                              'state_reason': None,

-                             'task_id': 12312345

+                             'task_id': 2433442

                          }

                      }

                  },

-                 'time_completed': '2016-09-03T11:25:32Z',

-                 'time_modified': '2016-09-03T11:25:32Z',

-                 'time_submitted': '2016-09-03T11:23:20Z',

-                 'version': '2'

+                 'time_completed': None,

+                 'time_modified': '2016-09-03T13:58:40Z',

+                 'time_submitted': '2016-09-03T13:58:33Z',

+                 'version': '6'

              },

-             {

-                 'id': 2,

+                 {

+                 'id': 29,

                  'koji_tag': 'module-postgressql-1.2',

                  'name': 'postgressql',

                  'owner': 'some_user',

-                 'scmurl': ('git://pkgs.domain.local/modules/postgressql?#aa95886c7a443b36a'

-                            '9ce31abda1f9bef22f2f8c9'),

+                 'scmurl': 'git://pkgs.domain.local/modules/postgressql?#aa95886c7a443b36a9ce31abda1f9bef22f2f8c9',

                  'state': 3,

                  'state_name': 'done',

                  'state_reason': None,
@@ -303,19 +274,19 @@ 

                              'nvr': 'module-build-macros-01-1.module_postgresql_1_2',

                              'state': 1,

                              'state_reason': None,

-                             'task_id': 47383993

+                             'task_id': 47384002

                          },

                          'postgresql': {

                              'nvr': 'postgresql-9.5.3-4.module_postgresql_1_2',

                              'state': 1,

                              'state_reason': None,

-                             'task_id': 2433433

+                             'task_id': 2433442

                          }

                      }

                  },

-                 'time_completed': '2016-09-03T11:27:19Z',

-                 'time_modified': '2016-09-03T12:27:19Z',

-                 'time_submitted': '2016-09-03T12:25:33Z',

+                 'time_completed': '2016-09-03T12:57:19Z',

+                 'time_modified': '2016-09-03T13:57:19Z',

+                 'time_submitted': '2016-09-03T13:55:33Z',

                  'version': '2'

              }

          ]

WIP, as two tests fail with this change, and it
isn't clear how best to fix them (changes to
the test DB aren't isolated, so assorted
additional entries build up)

Fixes #739

@ncoghlan, :+1: to the gist.

We'll just have to fix the index assumptions in the test suite by hand, for now.

Do you plan to follow up on this?

@ralph Aye, happy to follow up, just not sure how best to work around the test issue (as per https://pagure.io/fm-orchestrator/issue/739#comment-473898).

One option would be to rely on the "last" link in the initial response, follow that, and hence get to the known IDs, even without the entire test suite reliably cleaning up after itself.

(It's also possible I've misunderstood what's happening with the tests, and it would actually be fine to just change the test expectations to match the new results)

@ncoghlan as stated in #739, in the setUp function, init_data() is called which deletes the database entirely and adds the default entries back in. Where are you seeing the data changes persist?

rebased onto 8180b1b

6 years ago

Can you please fix this indentation?

Could you add the following docstring here please?

"""
Orders the SQLAlchemy query based on the GET arguments provided
:param flask_request: a Flask request object
:param query: a SQLAlchemy query object
:param column_source: a SQLAlchemy database model
:return: a SQLAlchemy query object
"""

@ncoghlan thanks for the PR. Can you please review the comments?

Oops, it seems I failed to git add the test fixes before pushing the rebased PR branch.

2 new commits added

  • Address review comments
  • Fix tests for rebased PR
6 years ago

I merged this PR as part of https://pagure.io/fm-orchestrator/c/d159350fff1b17a1e41f30c7d682451364905638.

I had to tweak the README but otherwise, it looks good.

Pull-Request has been closed by mprahl

6 years ago