From b96c2cb8a20988c610ccb3a100d89ab5ec5b0108 Mon Sep 17 00:00:00 2001 From: Mike Fedosin Date: Thu, 22 Sep 2016 10:02:33 +0300 Subject: [PATCH] Improve 'latest' filter Previous version didn't consider user's filters, and therefore 'latest' filter was applied before them. It led to the fact that some artifacts weren't shown in list output. New version fixes this behaviour. Change-Id: I7ae870d3cb575a6060901ac8321fdc7a7b6d4b4d --- glare/db/sqlalchemy/api.py | 95 +++++++++++-------- .../tests/functional/test_sample_artifact.py | 20 +++- 2 files changed, 73 insertions(+), 42 deletions(-) diff --git a/glare/db/sqlalchemy/api.py b/glare/db/sqlalchemy/api.py index 5eb5789..0d971c0 100644 --- a/glare/db/sqlalchemy/api.py +++ b/glare/db/sqlalchemy/api.py @@ -203,14 +203,46 @@ def get_all(context, session, filters=None, marker=None, limit=None, return [af.to_dict() for af in artifacts] -def _get_all(context, session, filters=None, marker=None, limit=None, - sort=None, latest=False): +def _apply_latest_filter(context, session, query, + basic_conds, tag_conds, prop_conds): + # Subquery to fetch max version suffix for a group (name, + # version_prefix) + ver_suffix_subq = _apply_query_base_filters( + session.query( + models.Artifact.name, + models.Artifact.version_prefix, + func.max(models.Artifact.version_suffix).label( + 'max_suffix')).group_by( + models.Artifact.name, models.Artifact.version_prefix), + context) + ver_suffix_subq = _apply_user_filters( + ver_suffix_subq, basic_conds, tag_conds, prop_conds).subquery() + # Subquery to fetch max version prefix for a name group + ver_prefix_subq = _apply_query_base_filters( + session.query(models.Artifact.name, func.max( + models.Artifact.version_prefix).label('max_prefix')).group_by( + models.Artifact.name), + context) + ver_prefix_subq = _apply_user_filters( + ver_prefix_subq, basic_conds, tag_conds, prop_conds).subquery() + # Combine two subqueries together joining them with Artifact table + query = query.join( + ver_prefix_subq, + and_(models.Artifact.name == ver_prefix_subq.c.name, + models.Artifact.version_prefix == + ver_prefix_subq.c.max_prefix)).join( + ver_suffix_subq, + and_(models.Artifact.name == ver_suffix_subq.c.name, + models.Artifact.version_prefix == + ver_suffix_subq.c.version_prefix, + models.Artifact.version_suffix == + ver_suffix_subq.c.max_suffix) + ) - filters = filters or {} + return query - query = _do_artifacts_query(context, session, latest) - basic_conds, tag_conds, prop_conds = _do_query_filters(filters) +def _apply_user_filters(query, basic_conds, tag_conds, prop_conds): if basic_conds: for basic_condition in basic_conds: @@ -226,6 +258,24 @@ def _get_all(context, session, filters=None, marker=None, limit=None, query = query.join(models.ArtifactProperty, aliased=True).filter( and_(*prop_condition)) + return query + + +def _get_all(context, session, filters=None, marker=None, limit=None, + sort=None, latest=False): + + filters = filters or {} + + query = _do_artifacts_query(context, session) + + basic_conds, tag_conds, prop_conds = _do_query_filters(filters) + + query = _apply_user_filters(query, basic_conds, tag_conds, prop_conds) + + if latest: + query = _apply_latest_filter(context, session, query, + basic_conds, tag_conds, prop_conds) + marker_artifact = None if marker is not None: marker_artifact = get(context, marker, session) @@ -334,37 +384,6 @@ def _do_artifacts_query(context, session, latest=False): query = session.query(models.Artifact) - if latest: - # Subquery to fetch max version suffix for a group (name, - # version_prefix) - ver_suffix_subq = _apply_query_base_filters( - session.query( - models.Artifact.name, - models.Artifact.version_prefix, - func.max(models.Artifact.version_suffix).label( - 'max_suffix')).group_by( - models.Artifact.name, models.Artifact.version_prefix), - context).subquery() - # Subquery to fetch max version prefix for a name group - ver_prefix_subq = _apply_query_base_filters( - session.query(models.Artifact.name, func.max( - models.Artifact.version_prefix).label('max_prefix')).group_by( - models.Artifact.name), - context).subquery() - # Combine two subqueries together joining them with Artifact table - query = query.join( - ver_prefix_subq, - and_(models.Artifact.name == ver_prefix_subq.c.name, - models.Artifact.version_prefix == - ver_prefix_subq.c.max_prefix)).join( - ver_suffix_subq, - and_(models.Artifact.name == ver_suffix_subq.c.name, - models.Artifact.version_prefix == - ver_suffix_subq.c.version_prefix, - models.Artifact.version_suffix == - ver_suffix_subq.c.max_suffix) - ) - query = (query.options(joinedload(models.Artifact.properties)). options(joinedload(models.Artifact.tags)). options(joinedload(models.Artifact.blobs))) @@ -373,10 +392,6 @@ def _do_artifacts_query(context, session, latest=False): def _apply_query_base_filters(query, context): - - # Don't show deleted artifacts - query = query.filter(models.Artifact.status != 'deleted') - # Don't show deleted artifacts query = query.filter(models.Artifact.status != 'deleted') diff --git a/glare/tests/functional/test_sample_artifact.py b/glare/tests/functional/test_sample_artifact.py index 39d8299..ecc8faf 100644 --- a/glare/tests/functional/test_sample_artifact.py +++ b/glare/tests/functional/test_sample_artifact.py @@ -687,17 +687,19 @@ class TestList(TestArtifact): {'name': 'group1', 'version': group1_versions[i], 'tags': ['tag%s' % i], - 'int1': 2048, + 'int1': 2048 + i, 'float1': 123.456, 'str1': 'bugaga', + "string_required": "test_str", 'bool1': True}) self.create_artifact( {'name': 'group2', 'version': group2_versions[i], 'tags': ['tag%s' % i], - 'int1': 2048, + 'int1': 2048 + i, 'float1': 123.456, 'str1': 'bugaga', + "string_required": "test_str", 'bool1': True}) url = '/sample_artifact?version=latest&sort=name:asc' @@ -706,6 +708,20 @@ class TestList(TestArtifact): self.assertEqual('20.0.0', res[0]['version']) self.assertEqual('1000.0.1', res[1]['version']) + self.patch('/sample_artifact/' + res[0]['id'], self.make_active) + + url = '/sample_artifact?version=latest&sort=name:asc&status=drafted' + res = self.get(url=url, status=200)['sample_artifact'] + self.assertEqual(2, len(res)) + self.assertEqual('2.0.1', res[0]['version']) + self.assertEqual('1000.0.1', res[1]['version']) + + url = '/sample_artifact?version=latest&sort=name:asc&int1=2050' + res = self.get(url=url, status=200)['sample_artifact'] + self.assertEqual(2, len(res)) + self.assertEqual('2.0.0', res[0]['version']) + self.assertEqual('99.0.0', res[1]['version']) + url = '/sample_artifact?version=latest&name=group1' res = self.get(url=url, status=200)['sample_artifact'] self.assertEqual(1, len(res))