fix(appversion): fix version filtering and comparison (#551)

* fix(filter): add gte as filter-operator

* fix(appversion): include empty maxDhisVersions

* fix: use paramter-binding

* fix: support for version matching

* fix: add tests for versions

* test: update test

* fix: order by createdAt

* fix: add updatedAt field

* fix: move version-filter--check to applyVersionFilter

* fix: allow valid semver characters in filter

server/seeds/04_appchannel.js

@@ -30,12 +30,14 @@ exports.seed = async knex => {
             app_version_id: dhis2AppVersions[0].id,
             channel_id: stableId,
             min_dhis2_version: '2.28',
+            max_dhis2_version: '2.30',
             created_by_user_id: dhis2AppVersions[0].created_by_user_id,
         },
         {
             app_version_id: dhis2AppVersions[1].id,
             channel_id: developmentId,
             min_dhis2_version: '2.29',
+            max_dhis2_version: '2.34',
             created_by_user_id: dhis2AppVersions[1].created_by_user_id,
         },
         {

server/src/services/appVersion.js

@@ -20,13 +20,15 @@ const getAppVersionQuery = knex =>
             'app_version.version',
             knex.ref('app_version.app_id').as('appId'),
             knex.ref('app_version.created_at').as('createdAt'),
+            knex.ref('app_version.updated_at').as('updatedAt'),
             knex.ref('app_version.source_url').as('sourceUrl'),
             knex.ref('app_version.demo_url').as('demoUrl'),
             knex.ref('channel.name').as('channel'),
             knex.ref('ac.min_dhis2_version').as('minDhisVersion'),
             knex.ref('ac.max_dhis2_version').as('maxDhisVersion')
         )
-        .distinct() // app_version_localised may return multiple versions
+        .where('language_code', 'en') // only english is supported for now
+        .orderBy('app_version.created_at', 'desc')
 
 class AppVersionService extends Schmervice.Service {
     constructor(server, schmerviceOptions) {
@@ -46,6 +48,12 @@ class AppVersionService extends Schmervice.Service {
             })
         }
 
+        // null-values are allowed for maxDhisVersion, so include these if filter is present
+        filters.applyVersionFilter(query, 'maxDhisVersion', {
+            includeEmpty: true,
+        })
+        filters.applyVersionFilter(query, 'minDhisVersion')
+
         return executeQuery(query, {
             filters,
             pager,

server/src/utils/Filter.js

@@ -103,29 +103,73 @@ class Filters {
      * Applies the filter
      * @param {*} query
      * @param {*} fieldName
-     * @param {*} options
+     * @param {*} options options object, merged with options to Filters object
+     * @param {*} overrideColumnName overrides the column name, takes precedence over tableName
+     * @param {*} includeEmpty includes empty values (null or '') for the filter
      */
 
     applyOneToQuery(query, fieldName, options) {
         const colName = this.getFilterColumn(fieldName)
         const filter = this.getFilter(fieldName)
-
+        const settings = {
+            ...this.options,
+            ...options,
+        }
         if (filter) {
             const nameToUse =
                 options.overrideColumnName ||
-                (options.tableName
-                    ? `${options.tableName}.${colName}`
+                (settings.tableName
+                    ? `${settings.tableName}.${colName}`
                     : colName)
             const { value, operator } = filter
+            query.where(builder => {
+                builder.where(nameToUse, toSQLOperator(operator, value), value)
+                if (settings.includeEmpty) {
+                    builder.orWhereRaw(`nullif( ??, ' ') is null`, nameToUse)
+                }
+            })
             this.markApplied(fieldName)
-            query.where(nameToUse, toSQLOperator(operator, value), value)
         } else {
             throw new Error(
                 `Failed to apply filter to query, ${colName} is not a valid filter.`
             )
         }
     }
 
+    /** Applies a filter for comparing versions
+     * eg. this will match correctly for for 1.9.0 < 1.10.0,
+     * opposed to normal string matching.
+     */
+    applyVersionFilter(query, filterName, options = {}) {
+        const filter = this.getFilter(filterName)
+
+        if (!filter) {
+            return
+        }
+
+        const colName =
+            options.overrideColumnName || this.getFilterColumn(filterName)
+
+        if (!filter) {
+            throw new Error(
+                `Failed to apply filter to query, ${colName} is not a valid filter.`
+            )
+        }
+
+        query.where(builder => {
+            builder.whereRaw(
+                `string_to_array( regexp_replace(??, '[^0-9.]+', '', 'g'), '.')::int[] ${toSQLOperator(
+                    filter.operator
+                )} string_to_array( regexp_replace(?, '[^0-9.]+', '', 'g'), '.')::int[]`,
+                [colName, filter.value]
+            )
+            if (options.includeEmpty) {
+                builder.orWhereRaw(`nullif( ??, ' ') is null`, colName)
+            }
+        })
+        this.markApplied(filterName)
+    }
+
     /**
      * Marks the filter as applied. Can be useful when creating a custom query,
      * and you don't want the filter to be applied when running applyAllToQuery().

server/src/utils/filterUtils.js

@@ -15,6 +15,7 @@ const operatorMap = {
     lt: '<',
     gt: '>',
     lte: '<=',
+    gte: '>=',
     ne: '<>',
 }
 const allOperatorsMap = {

server/test/data/index.js

@@ -261,7 +261,7 @@ describe('@data::updateAppVersion', () => {
         )
         expect(app.version_id).to.equal(appVersionIdToUpdate)
         expect(app.version).to.equal('0.1')
-        expect(app.max_dhis2_version).to.equal(null)
+        expect(app.max_dhis2_version).to.equal('2.30')
         expect(app.min_dhis2_version).to.equal('2.28')
         expect(app.demo_url).to.equal(null)
 

server/test/routes/v2/appVersions.js

@@ -99,5 +99,155 @@ describe('v2/appVersions', () => {
                 expect(v.minDhisVersion).to.be.a.string()
             })
         })
+
+        describe('with filters', () => {
+            it('should handle channel filter', async () => {
+                const request = {
+                    method: 'GET',
+                    url: `/api/v2/apps/${dhis2App.id}/versions?channel=development`,
+                }
+
+                const res = await server.inject(request)
+
+                expect(res.statusCode).to.equal(200)
+
+                const result = res.result
+                const versions = res.result.result
+
+                Joi.assert(versions, Joi.array().items(AppVersionModel.def))
+                expect(result.pager).to.be.an.object()
+                // check some keys as well
+                versions.forEach(v => {
+                    expect(v.appId).to.be.a.string()
+                    expect(v.version).to.be.a.string()
+                    expect(v.channel).to.be.equal('development')
+                })
+            })
+
+            it('should handle minDhisVersion', async () => {
+                const request = {
+                    method: 'GET',
+                    url: `/api/v2/apps/${dhis2App.id}/versions?minDhisVersion=eq:2.29`,
+                }
+
+                const res = await server.inject(request)
+
+                expect(res.statusCode).to.equal(200)
+
+                const result = res.result
+                console.log(result)
+                const versions = result.result
+
+                console.log(versions)
+                Joi.assert(versions, Joi.array().items(AppVersionModel.def))
+                versions.forEach(v => {
+                    expect(v.appId).to.be.a.string()
+                    expect(v.version).to.be.a.string()
+                    expect(v.minDhisVersion).to.be.equal('2.29')
+                })
+            })
+
+            it('should handle minDhisVersion with lte and gte', async () => {
+                const request = {
+                    method: 'GET',
+                    url: `/api/v2/apps/${dhis2App.id}/versions?minDhisVersion=lte:2.28`,
+                }
+
+                const res = await server.inject(request)
+
+                expect(res.statusCode).to.equal(200)
+
+                const result = res.result
+                const versions = result.result
+
+                Joi.assert(versions, Joi.array().items(AppVersionModel.def))
+                versions.forEach(v => {
+                    expect(v.appId).to.be.a.string()
+                    expect(v.version).to.be.a.string()
+                    expect(v.minDhisVersion).to.be.below('2.29')
+                })
+            })
+
+            it('should handle maxDhisVersion', async () => {
+                const request = {
+                    method: 'GET',
+                    url: `/api/v2/apps/${dhis2App.id}/versions?maxDhisVersion=lte:2.34`,
+                }
+
+                const res = await server.inject(request)
+
+                expect(res.statusCode).to.equal(200)
+
+                const result = res.result
+                const versions = result.result
+
+                expect(versions.length).to.be.above(0)
+                Joi.assert(versions, Joi.array().items(AppVersionModel.def))
+                versions.forEach(v => {
+                    expect(v.appId).to.be.a.string()
+                    expect(v.version).to.be.a.string()
+                    if (v.maxDhisVersion) {
+                        const [, major] = v.maxDhisVersion
+                            .split('.')
+                            .map(Number)
+                        expect(major).to.be.lessThan(35)
+                    }
+                })
+            })
+
+            it('should handle both minDhisVersion and maxDhisVersion', async () => {
+                const request = {
+                    method: 'GET',
+                    url: `/api/v2/apps/${dhis2App.id}/versions?maxDhisVersion=lte:2.34&minDhisVersion=gte:2.29`,
+                }
+
+                const res = await server.inject(request)
+
+                expect(res.statusCode).to.equal(200)
+
+                const result = res.result
+                const versions = result.result
+
+                expect(versions.length).to.be.above(0)
+                Joi.assert(versions, Joi.array().items(AppVersionModel.def))
+                versions.forEach(v => {
+                    expect(v.appId).to.be.a.string()
+                    expect(v.version).to.be.a.string()
+                    if (v.maxDhisVersion) {
+                        const [, major] = v.maxDhisVersion
+                            .split('.')
+                            .map(Number)
+                        expect(major).to.be.between(28, 35)
+                    }
+                })
+            })
+
+            it('should not fail when version include valid semver characters', async () => {
+                const request = {
+                    method: 'GET',
+                    url: `/api/v2/apps/${dhis2App.id}/versions?maxDhisVersion=lte:2.34-beta&minDhisVersion=gte:2.29-alpha`,
+                }
+
+                const res = await server.inject(request)
+
+                expect(res.statusCode).to.equal(200)
+
+                const result = res.result
+                const versions = result.result
+
+                expect(versions.length).to.be.above(0)
+                Joi.assert(versions, Joi.array().items(AppVersionModel.def))
+                versions.forEach(v => {
+                    expect(v.appId).to.be.a.string()
+                    expect(v.version).to.be.a.string()
+                    if (v.maxDhisVersion) {
+                        const [, major] = v.maxDhisVersion
+                            .split('.')
+                            .map(Number)
+                        expect(major).to.be.between(28, 35)
+                    }
+                })
+            })
+        })
     })
 })