From f8160930364fdf9bb7b7eb816b68d74c92841b37 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 2 Sep 2015 16:25:56 +0200 Subject: [PATCH] Set `Last-Modified` header based on affected tables Closes #101 --- NEWS.md | 6 +- app/controllers/app.js | 74 ++++++++------- npm-shrinkwrap.json | 5 +- package.json | 2 +- test/acceptance/last-modified-header.js | 117 ++++++++++++++++++++++++ test/test.sql | 13 +++ 6 files changed, 181 insertions(+), 36 deletions(-) create mode 100644 test/acceptance/last-modified-header.js diff --git a/NEWS.md b/NEWS.md index c56a98b6..7fb71cef 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,6 +1,10 @@ -1.24.1 - 2015-mm-dd +1.25.0 - 2015-mm-dd ------------------- +New features: + + * Set `Last-Modified` header based on affected tables (#101) + 1.24.0 - 2015-08-04 ------------------- diff --git a/app/controllers/app.js b/app/controllers/app.js index 8798bded..d5efb5b6 100755 --- a/app/controllers/app.js +++ b/app/controllers/app.js @@ -365,26 +365,39 @@ function handleQuery(req, res) { tableCacheItem.hits++; return false; } else { - //TODO: sanitize cdbuser - pg.query("SELECT CDB_QueryTables($quotesql$" + sql + "$quotesql$)", function (err, result) { - if (err) { - self(err); - return; - } - if ( result.rowCount === 1 ) { - var raw_tables = result.rows[0].cdb_querytables; - var tables = raw_tables.split(/^\{(.*)\}$/)[1].split(','); - self(null, tables); - } else { - console.error( - "Unexpected result from CDB_QueryTables($quotesql$" + sql + "$quotesql$): " + result - ); - self(null, []); - } - }); + var affectedTablesAndLastUpdatedTimeQuery = [ + 'WITH querytables AS (', + 'SELECT * FROM CDB_QueryTablesText($quotesql$' + sql + '$quotesql$) as tablenames', + ')', + 'SELECT (SELECT tablenames FROM querytables), EXTRACT(EPOCH FROM max(updated_at)) as max', + 'FROM CDB_TableMetadata m', + 'WHERE m.tabname = any ((SELECT tablenames from querytables)::regclass[])' + ].join(' '); + + pg.query(affectedTablesAndLastUpdatedTimeQuery, function (err, resultSet) { + var tableNames = []; + var lastUpdatedTime = Date.now(); + + if (!err && resultSet.rowCount === 1) { + var result = resultSet.rows[0]; + // This is an Array, so no need to split into parts + tableNames = result.tablenames; + if (Number.isFinite(result.max)) { + lastUpdatedTime = result.max * 1000; + } + } else { + var errorMessage = (err && err.message) || 'unknown error'; + console.error("Error on query explain '%s': %s", sql, errorMessage); + } + + return self(null, { + affectedTables: tableNames, + lastUpdatedTime: lastUpdatedTime + }); + }); } }, - function setHeaders(err, tables){ + function setHeaders(err, result) { assert.ifError(err); if ( req.profiler ) { @@ -394,13 +407,14 @@ function handleQuery(req, res) { checkAborted('setHeaders'); // store explain result in local Cache - if ( ! tableCacheItem && tables.length ) { + if ( ! tableCacheItem && result && result.affectedTables ) { tableCacheItem = { - affected_tables: tables, - // check if query may possibly write - may_write: queryMayWrite(sql), - // initialise hit counter - hits: 1 + affected_tables: result.affectedTables, + last_modified: result.lastUpdatedTime, + // check if query may possibly write + may_write: queryMayWrite(sql), + // initialise hit counter + hits: 1 }; tableCache.set(sql_md5, tableCacheItem); } @@ -449,14 +463,10 @@ function handleQuery(req, res) { res.header('X-Cache-Channel', generateCacheKey(dbopts.dbname, tableCacheItem, authenticated)); } - // Set Last-Modified header - // - // Currently sets it to NOW - // - // TODO: use a real value, querying for most recent change in - // any of the source tables - // - res.header('Last-Modified', new Date().toUTCString()); + var lastModified = (tableCacheItem && tableCacheItem.last_modified) ? + tableCacheItem.last_modified : + Date.now(); + res.header('Last-Modified', new Date(lastModified).toUTCString()); return null; }, diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 1f5cb467..99bcf900 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -1,6 +1,6 @@ { "name": "cartodb_sql_api", - "version": "1.24.1", + "version": "1.25.0", "dependencies": { "cartodb-psql": { "version": "0.6.0", @@ -142,7 +142,8 @@ }, "inherits": { "version": "2.0.1", - "from": "inherits@~2.0.1" + "from": "inherits@2", + "resolved": "https://registry.npmjs.org/inherits/-/inherits-2.0.1.tgz" } } }, diff --git a/package.json b/package.json index 2f784660..8bb21efe 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "keywords": [ "cartodb" ], - "version": "1.24.1", + "version": "1.25.0", "repository": { "type": "git", "url": "git://github.com/CartoDB/CartoDB-SQL-API.git" diff --git a/test/acceptance/last-modified-header.js b/test/acceptance/last-modified-header.js new file mode 100644 index 00000000..58a438a4 --- /dev/null +++ b/test/acceptance/last-modified-header.js @@ -0,0 +1,117 @@ +require('../helper'); + +var app = require(global.settings.app_root + '/app/controllers/app')(); +var assert = require('../support/assert'); +var qs = require('querystring'); + +describe('last modified header', function() { + + var scenarios = [ + { + tables: ['untitle_table_4'], + desc: 'should use last updated time from public table', + expectedLastModified: 'Wed, 01 Jan 2014 23:31:30 GMT' + }, + { + tables: ['private_table'], + desc: 'should use last updated time from private table', + expectedLastModified: 'Thu, 01 Jan 2015 23:31:30 GMT' + }, + { + tables: ['untitle_table_4', 'private_table'], + desc: 'should use most recent last updated time from private and public table', + expectedLastModified: 'Thu, 01 Jan 2015 23:31:30 GMT' + }, + { + tables: ['populated_places_simple_reduced', 'private_table'], + desc: 'should use last updated time from table in cdb_tablemetadata instead of now() from unknown table', + expectedLastModified: 'Thu, 01 Jan 2015 23:31:30 GMT' + } + ]; + + scenarios.forEach(function(scenario) { + it(scenario.desc, function(done) { + var query = qs.stringify({ + q: scenario.tables.map(function(table) { + return 'select cartodb_id from ' + table; + }).join(' UNION ALL '), + api_key: 1234 + }); + assert.response(app, + { + url: '/api/v1/sql?' + query, + headers: { + host: 'vizzuality.cartodb.com' + }, + method: 'GET' + }, + { + statusCode: 200 + }, + function(res) { + assert.equal(res.headers['last-modified'], scenario.expectedLastModified); + done(); + } + ); + }); + }); + + it('should use Date.now() for tables not present in cdb_tablemetadata', function(done) { + var query = qs.stringify({ + q: 'select cartodb_id from populated_places_simple_reduced limit 1', + api_key: 1234 + }); + var fixedDateNow = Date.now(); + var dateNowFn = Date.now; + Date.now = function() { + return fixedDateNow; + }; + assert.response(app, + { + url: '/api/v1/sql?' + query, + headers: { + host: 'vizzuality.cartodb.com' + }, + method: 'GET' + }, + { + statusCode: 200 + }, + function(res) { + Date.now = dateNowFn; + assert.equal(res.headers['last-modified'], new Date(fixedDateNow).toUTCString()); + done(); + } + ); + }); + + it('should use Date.now() for functions or results with no table associated', function(done) { + var query = qs.stringify({ + q: 'select 1', + api_key: 1234 + }); + var fixedDateNow = Date.now(); + var dateNowFn = Date.now; + Date.now = function() { + return fixedDateNow; + }; + assert.response(app, + { + url: '/api/v1/sql?' + query, + headers: { + host: 'vizzuality.cartodb.com' + }, + method: 'GET' + }, + { + statusCode: 200 + }, + function(res) { + Date.now = dateNowFn; + assert.equal(res.headers['last-modified'], new Date(fixedDateNow).toUTCString()); + done(); + } + ); + }); + +}); diff --git a/test/test.sql b/test/test.sql index 07ecc201..7dc09320 100644 --- a/test/test.sql +++ b/test/test.sql @@ -137,3 +137,16 @@ DROP TABLE IF EXISTS cpg_test; CREATE TABLE cpg_test (a int); GRANT ALL ON TABLE cpg_test TO :TESTUSER; GRANT SELECT ON TABLE cpg_test TO :PUBLICUSER; + + +CREATE TABLE IF NOT EXISTS + CDB_TableMetadata ( + tabname regclass not null primary key, + updated_at timestamp with time zone not null default now() + ); + +INSERT INTO CDB_TableMetadata (tabname, updated_at) VALUES ('untitle_table_4'::regclass, '2014-01-01T23:31:30.123Z'); +INSERT INTO CDB_TableMetadata (tabname, updated_at) VALUES ('private_table'::regclass, '2015-01-01T23:31:30.123Z'); + +GRANT SELECT ON CDB_TableMetadata TO :PUBLICUSER; +GRANT SELECT ON CDB_TableMetadata TO :TESTUSER;