Set Last-Modified header based on affected tables

Closes #101

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
 -------------------

app/controllers/app.js

@@ -365,26 +365,39 @@ function handleQuery(req, res) {
                    tableCacheItem.hits++;
                    return false;
                 } else {
-                   //TODO: sanitize cdbuser
+                    var affectedTablesAndLastUpdatedTimeQuery = [
-                   pg.query("SELECT CDB_QueryTables($quotesql$" + sql + "$quotesql$)", function (err, result) {
+                        'WITH querytables AS (',
-                      if (err) {
+                            'SELECT * FROM CDB_QueryTablesText($quotesql$' + sql + '$quotesql$) as tablenames',
-                        self(err);
+                        ')',
-                        return;
+                        '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;
                             }
-                      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(
+                            var errorMessage = (err && err.message) || 'unknown error';
-                            "Unexpected result from CDB_QueryTables($quotesql$" + sql + "$quotesql$): " + result
+                            console.error("Error on query explain '%s': %s", sql, errorMessage);
-                        );
-                        self(null, []);
                         }
+
+                        return self(null, {
+                            affectedTables: tableNames,
+                            lastUpdatedTime: lastUpdatedTime
+                        });
                     });
                 }
             },
-            function setHeaders(err, tables){
+            function setHeaders(err, result) {
                 assert.ifError(err);
 
                 if ( req.profiler ) {
@@ -394,9 +407,10 @@ 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,
+                        affected_tables: result.affectedTables,
+                        last_modified: result.lastUpdatedTime,
                         // check if query may possibly write
                         may_write: queryMayWrite(sql),
                         // initialise hit counter
@@ -449,14 +463,10 @@ function handleQuery(req, res) {
                   res.header('X-Cache-Channel', generateCacheKey(dbopts.dbname, tableCacheItem, authenticated));
                 }
 
-                // Set Last-Modified header
+                var lastModified = (tableCacheItem && tableCacheItem.last_modified) ?
-                //
+                    tableCacheItem.last_modified :
-                // Currently sets it to NOW
+                    Date.now();
-                //
+                res.header('Last-Modified', new Date(lastModified).toUTCString());
-                // TODO: use a real value, querying for most recent change in
-                //       any of the source tables
-                //
-                res.header('Last-Modified', new Date().toUTCString());
 
                 return null;
             },

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"
             }
           }
         },

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"

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();
+            }
+        );
+    });
+
+});

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;