diff --git a/app/controllers/query_controller.js b/app/controllers/query_controller.js index 2340e55f..0ca2f819 100644 --- a/app/controllers/query_controller.js +++ b/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, skipCache, this); + self.queryTablesApi.getAffectedTablesAndLastUpdatedTime(authDbParams, sql, this); }, function setHeaders(err, queryExplainResult) { assert.ifError(err); diff --git a/app/services/query-tables-api.js b/app/services/query-tables-api.js index e0295e4c..904bc43f 100644 --- a/app/services/query-tables-api.js +++ b/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; - - if (!skipCache) { - queryExplainResult = this.tableCache.get(cacheKey); - } + var 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, - mayWrite: queryMayWrite(sql), - hits: 1 - }; - - self.tableCache.set(cacheKey, queryExplainResult); - - return callback(null, queryExplainResult); + lastUpdatedTime: lastUpdatedTime + }); }, 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) { diff --git a/test/acceptance/query-tables-api-cache.js b/test/acceptance/query-tables-api-cache.js index 354a0ff1..4e36189f 100644 --- a/test/acceptance/query-tables-api-cache.js +++ b/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 = [ - { - apiKey: 1234, - shouldSkipCache: true + function getCacheStatus(callback) { + assert.response( + app, + { + 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' }, - { - apiKey: null, - shouldSkipCache: false - } - ]; + method: 'GET' + }; - scenarios.forEach(function(scenario) { - var shouldOrShouldNot = scenario.shouldSkipCache ? 'should' : 'should NOT'; - 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; + var RESPONSE_OK = { + status: 200 + }; - assert.ok(!err, err); - assert.equal(res.statusCode, 200, res.statusCode + ': ' + res.body); + it('should create a key in affected tables cache', function(done) { + assert.response(app, request, RESPONSE_OK, function(res, err) { + assert.ok(!err, err); - assert.equal(skippedCache, scenario.shouldSkipCache, 'skip cache expected as true'); - assert.ok(getAffectedTablesCalled, 'getAffectedTablesAndLastUpdatedTime NOT called'); + getCacheStatus(function(err, cacheStatus) { + 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(); + }); }); }); });