QueryTablesApi only caches affected tables and always retrieve last modification

app/controllers/query_controller.js

@@ -133,9 +133,7 @@ QueryController.prototype.handleQuery = function (req, res) {
 
                 checkAborted('queryExplain');
 
-                var skipCache = !!dbopts.authenticated;
+                self.queryTablesApi.getAffectedTablesAndLastUpdatedTime(authDbParams, sql, this);
-
-                self.queryTablesApi.getAffectedTablesAndLastUpdatedTime(authDbParams, sql, skipCache, this);
             },
             function setHeaders(err, queryExplainResult) {
                 assert.ifError(err);

app/services/query-tables-api.js

@@ -9,22 +9,41 @@ function QueryTablesApi(tableCache) {
 
 module.exports = QueryTablesApi;
 
-QueryTablesApi.prototype.getAffectedTablesAndLastUpdatedTime = function (connectionParams, sql, skipCache, callback) {
+QueryTablesApi.prototype.getAffectedTablesAndLastUpdatedTime = function (connectionParams, sql, callback) {
     var self = this;
 
     var cacheKey = sqlCacheKey(connectionParams.user, sql);
-    var queryExplainResult;
+    var queryExplainResult = this.tableCache.get(cacheKey);
-
-    if (!skipCache) {
-        queryExplainResult = this.tableCache.get(cacheKey);
-    }
 
     if (queryExplainResult) {
         queryExplainResult.hits++;
-        return callback(null, queryExplainResult);
+        getLastUpdatedTime(connectionParams, queryExplainResult.affectedTables, function(err, lastUpdatedTime) {
+            return callback(null, {
+                affectedTables: queryExplainResult.affectedTables,
+                lastModified: lastUpdatedTime,
+                mayWrite: queryExplainResult.mayWrite
+            });
+        });
+    } else {
+        getAffectedTablesAndLastUpdatedTime(connectionParams, sql, function(err, affectedTablesAndLastUpdatedTime) {
+            var queryExplainResult = {
+                affectedTables: affectedTablesAndLastUpdatedTime.affectedTables,
+                mayWrite: queryMayWrite(sql),
+                hits: 1
+            };
+
+            self.tableCache.set(cacheKey, queryExplainResult);
+
+            return callback(null, {
+                affectedTables: queryExplainResult.affectedTables,
+                lastModified: affectedTablesAndLastUpdatedTime.lastUpdatedTime,
+                mayWrite: queryExplainResult.mayWrite
+            });
+        });
     }
+};
 
-
+function getAffectedTablesAndLastUpdatedTime(connectionParams, sql, callback) {
     var query = [
         'WITH querytables AS (',
             'SELECT * FROM CDB_QueryTablesText($quotesql$' + sql + '$quotesql$) as tablenames',
@@ -48,18 +67,38 @@ QueryTablesApi.prototype.getAffectedTablesAndLastUpdatedTime = function (connect
         var tableNames = result.tablenames || [];
         var lastUpdatedTime = (Number.isFinite(result.max)) ? (result.max * 1000) : Date.now();
 
-        var queryExplainResult = {
+        return callback(null, {
             affectedTables: tableNames,
-            lastModified: lastUpdatedTime,
+            lastUpdatedTime: lastUpdatedTime
-            mayWrite: queryMayWrite(sql),
+        });
-            hits: 1
-        };
-
-        self.tableCache.set(cacheKey, queryExplainResult);
-
-        return callback(null, queryExplainResult);
     }, true);
-};
+}
+
+function getLastUpdatedTime(connectionParams, tableNames, callback) {
+    if (!Array.isArray(tableNames) || tableNames.length === 0) {
+        return callback(null, Date.now());
+    }
+
+    var query = [
+        'SELECT EXTRACT(EPOCH FROM max(updated_at)) as max',
+        'FROM CDB_TableMetadata m WHERE m.tabname = any (ARRAY[',
+            tableNames.map(function(t) { return "'" + t + "'::regclass"; }).join(','),
+        '])'
+    ].join(' ');
+
+    var pg = new PSQL(connectionParams, {}, { destroyOnError: true });
+
+    pg.query(query, function handleLastUpdatedTimeRows (err, resultSet) {
+        resultSet = resultSet || {};
+        var rows = resultSet.rows || [];
+
+        var result = rows[0] || {};
+
+        var lastUpdatedTime = (Number.isFinite(result.max)) ? (result.max * 1000) : Date.now();
+
+        return callback(null, lastUpdatedTime);
+    }, true);
+}
 
 function logIfError(err, sql, rows) {
     if (err || rows.length !== 1) {

test/acceptance/query-tables-api-cache.js

@@ -5,67 +5,63 @@ var qs = require('querystring');
 var app = require(global.settings.app_root + '/app/app')();
 var assert = require('../support/assert');
 
-var QueryTablesApi = require('../../app/services/query-tables-api');
-
 describe('query-tables-api', function() {
 
-    var scenarios = [
+    function getCacheStatus(callback) {
-        {
+        assert.response(
-            apiKey: 1234,
+            app,
-            shouldSkipCache: true
+            {
+                method: 'GET',
+                url: '/api/v1/cachestatus'
+            },
+            {
+                status: 200
+            },
+            function(res) {
+                callback(null, JSON.parse(res.body));
+            }
+        );
+    }
+
+    var request = {
+        url: '/api/v1/sql?' + qs.stringify({
+            q: 'SELECT * FROM untitle_table_4'
+        }),
+        headers: {
+            host: 'vizzuality.cartodb.com'
         },
-        {
+        method: 'GET'
-            apiKey: null,
+    };
-            shouldSkipCache: false
-        }
-    ];
 
-    scenarios.forEach(function(scenario) {
+    var RESPONSE_OK = {
-        var shouldOrShouldNot = scenario.shouldSkipCache ? 'should' : 'should NOT';
+        status: 200
-        var desc = 'authenticated=' + JSON.stringify(!!scenario.apiKey) + ' requests' +
+    };
-            ' ' + shouldOrShouldNot + ' skip internal query-tables-api cache';
-        it(desc, function(done) {
-            var getAffectedTablesCalled = false;
-            var skippedCache = null;
-            var getAffectedTablesFn = QueryTablesApi.prototype.getAffectedTablesAndLastUpdatedTime;
-            QueryTablesApi.prototype.getAffectedTablesAndLastUpdatedTime =
-                function(connectionParams, sql, skipCache, callback) {
-                    getAffectedTablesCalled = true;
-                    skippedCache = skipCache;
-                    return callback(null, {
-                        affectedTables: [],
-                        lastModified: Date.now(),
-                        mayWrite: false,
-                        hits: 1
-                    });
-                };
-            assert.response(
-                app,
-                {
-                    url: '/api/v1/sql?' + qs.stringify({
-                        api_key: scenario.apiKey,
-                        q: 'SELECT * FROM untitle_table_4'
-                    }),
-                    headers: {
-                        host: 'vizzuality.cartodb.com'
-                    },
-                    method: 'GET'
-                },
-                {
-                    // status: 200 // not using this as we cannot restore getAffectedTablesAndLastUpdatedTime
-                },
-                function(res, err) {
-                    QueryTablesApi.prototype.getAffectedTablesAndLastUpdatedTime = getAffectedTablesFn;
 
-                    assert.ok(!err, err);
+    it('should create a key in affected tables cache', function(done) {
-                    assert.equal(res.statusCode, 200, res.statusCode + ': ' + res.body);
+        assert.response(app, request, RESPONSE_OK, function(res, err) {
+            assert.ok(!err, err);
 
-                    assert.equal(skippedCache, scenario.shouldSkipCache, 'skip cache expected as true');
+            getCacheStatus(function(err, cacheStatus) {
-                    assert.ok(getAffectedTablesCalled, 'getAffectedTablesAndLastUpdatedTime NOT called');
+                assert.ok(!err, err);
+                assert.equal(cacheStatus.explain.keys, 1);
+                assert.equal(cacheStatus.explain.hits, 1);
 
-                    done();
+                done();
-                }
+            });
-            );
+        });
+    });
+
+    it('should use cache to retrieve affected tables', function(done) {
+        assert.response(app, request, RESPONSE_OK, function(res, err) {
+            assert.ok(!err, err);
+
+            getCacheStatus(function(err, cacheStatus) {
+                assert.ok(!err, err);
+                assert.equal(cacheStatus.explain.keys, 1);
+                assert.equal(cacheStatus.explain.hits, 2);
+
+                done();
+            });
         });
     });
 });