From bc29587c55900f27f6ee1eb07e582803434dd923 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 3 Jul 2019 17:15:14 +0200 Subject: [PATCH 1/8] Set directive 'max-age' to 5 min when there are affacted tables where we can't know when were updated for the last time, e.g: non cartodified tables or foreing tables without cartodb support --- config/environments/development.js.example | 1 + config/environments/production.js.example | 1 + config/environments/staging.js.example | 1 + config/environments/test.js.example | 1 + .../api/middlewares/cache-control-header.js | 35 +++++++++++++++---- 5 files changed, 32 insertions(+), 7 deletions(-) diff --git a/config/environments/development.js.example b/config/environments/development.js.example index ed0aaf58..1e59ebf3 100644 --- a/config/environments/development.js.example +++ b/config/environments/development.js.example @@ -326,6 +326,7 @@ var config = { purge_enabled: false, // whether the purge/invalidation mechanism is enabled in varnish or not secret: 'xxx', ttl: 86400, + fallbackTtl: 300, layergroupTtl: 86400 // the max-age for cache-control header in layergroup responses } // this [OPTIONAL] configuration enables invalidating by surrogate key in fastly diff --git a/config/environments/production.js.example b/config/environments/production.js.example index bc34db81..42aecabd 100644 --- a/config/environments/production.js.example +++ b/config/environments/production.js.example @@ -326,6 +326,7 @@ var config = { purge_enabled: false, // whether the purge/invalidation mechanism is enabled in varnish or not secret: 'xxx', ttl: 86400, + fallbackTtl: 300, layergroupTtl: 86400 // the max-age for cache-control header in layergroup responses } // this [OPTIONAL] configuration enables invalidating by surrogate key in fastly diff --git a/config/environments/staging.js.example b/config/environments/staging.js.example index 41f8e281..5083ec11 100644 --- a/config/environments/staging.js.example +++ b/config/environments/staging.js.example @@ -326,6 +326,7 @@ var config = { purge_enabled: false, // whether the purge/invalidation mechanism is enabled in varnish or not secret: 'xxx', ttl: 86400, + fallbackTtl: 300, layergroupTtl: 86400 // the max-age for cache-control header in layergroup responses } // this [OPTIONAL] configuration enables invalidating by surrogate key in fastly diff --git a/config/environments/test.js.example b/config/environments/test.js.example index 05c5e94c..34edb917 100644 --- a/config/environments/test.js.example +++ b/config/environments/test.js.example @@ -328,6 +328,7 @@ var config = { purge_enabled: false, // whether the purge/invalidation mechanism is enabled in varnish or not secret: 'xxx', ttl: 86400, + fallbackTtl: 300, layergroupTtl: 86400 // the max-age for cache-control header in layergroup responses } // this [OPTIONAL] configuration enables invalidating by surrogate key in fastly diff --git a/lib/cartodb/api/middlewares/cache-control-header.js b/lib/cartodb/api/middlewares/cache-control-header.js index d72ee0c3..38d05baf 100644 --- a/lib/cartodb/api/middlewares/cache-control-header.js +++ b/lib/cartodb/api/middlewares/cache-control-header.js @@ -1,21 +1,42 @@ 'use strict'; const ONE_YEAR_IN_SECONDS = 60 * 60 * 24 * 365; +const FIVE_MINUTES_IN_SECONDS = 60 * 5; +const FALLBACK_TTL = global.environment.varnish.fallbackTtl || FIVE_MINUTES_IN_SECONDS -module.exports = function setCacheControlHeader ({ ttl = ONE_YEAR_IN_SECONDS, revalidate = false } = {}) { +module.exports = function setCacheControlHeader ({ + ttl = ONE_YEAR_IN_SECONDS, + fallbackTtl = FALLBACK_TTL, + revalidate = false +} = {}) { return function setCacheControlHeaderMiddleware (req, res, next) { if (req.method !== 'GET') { return next(); } - const directives = [ 'public', `max-age=${ttl}` ]; + const { mapConfigProvider = { getAffectedTables: callback => callback() } } = res.locals; - if (revalidate) { - directives.push('must-revalidate'); - } + mapConfigProvider.getAffectedTables((err, affectedTables) => { + if (err) { + global.logger.warn('ERROR generating Cache Control Header:', err); + return next(); + } - res.set('Cache-Control', directives.join(',')); + const directives = [ 'public' ] - next(); + if (affectedTables && !affectedTables.getTables().some(table => !!table.updated_at)) { + directives.push(`max-age=${fallbackTtl}`); + } else { + directives.push(`max-age=${ttl}`); + } + + if (revalidate) { + directives.push('must-revalidate'); + } + + res.set('Cache-Control', directives.join(',')); + + next(); + }); }; }; From a374deaf301e1c0dffd812247c521f26bbc4753c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 3 Jul 2019 17:16:09 +0200 Subject: [PATCH 2/8] Please linter --- lib/cartodb/api/middlewares/cache-control-header.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/api/middlewares/cache-control-header.js b/lib/cartodb/api/middlewares/cache-control-header.js index 38d05baf..7171c44a 100644 --- a/lib/cartodb/api/middlewares/cache-control-header.js +++ b/lib/cartodb/api/middlewares/cache-control-header.js @@ -2,7 +2,7 @@ const ONE_YEAR_IN_SECONDS = 60 * 60 * 24 * 365; const FIVE_MINUTES_IN_SECONDS = 60 * 5; -const FALLBACK_TTL = global.environment.varnish.fallbackTtl || FIVE_MINUTES_IN_SECONDS +const FALLBACK_TTL = global.environment.varnish.fallbackTtl || FIVE_MINUTES_IN_SECONDS; module.exports = function setCacheControlHeader ({ ttl = ONE_YEAR_IN_SECONDS, @@ -22,7 +22,7 @@ module.exports = function setCacheControlHeader ({ return next(); } - const directives = [ 'public' ] + const directives = [ 'public' ]; if (affectedTables && !affectedTables.getTables().some(table => !!table.updated_at)) { directives.push(`max-age=${fallbackTtl}`); From 2e8a5d0d86d60d90ca2164bdddf2849272d58783 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 4 Jul 2019 15:34:37 +0200 Subject: [PATCH 3/8] Add test --- test/acceptance/cache/cache-control-header.js | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 test/acceptance/cache/cache-control-header.js diff --git a/test/acceptance/cache/cache-control-header.js b/test/acceptance/cache/cache-control-header.js new file mode 100644 index 00000000..f318214d --- /dev/null +++ b/test/acceptance/cache/cache-control-header.js @@ -0,0 +1,91 @@ +'use strict'; + +require('../../support/test_helper'); + +const assert = require('../../support/assert'); +const TestClient = require('../../support/test-client'); + +const ONE_YEAR_IN_SECONDS = 60 * 60 * 24 * 365; +const FIVE_MINUTES_IN_SECONDS = 60 * 5; + +const defaultLayers = [{ + type: 'cartodb', + options: { + sql: TestClient.SQL.ONE_POINT, + cartocss: TestClient.CARTOCSS.POINTS, + cartocss_version: '2.3.0' + } +}]; + +function createMapConfig (layers = defaultLayers) { + return { + version: '1.8.0', + layers: layers + }; +} + +describe('cache-control header', function () { + describe('max-age directive', function () { + it('tile from a table wich is included in cdb_tablemetada', function (done) { + const ttl = ONE_YEAR_IN_SECONDS; + const mapConfig = createMapConfig([{ + type: 'cartodb', + options: { + sql: 'select * from test_table', + cartocss: TestClient.CARTOCSS.POINTS, + cartocss_version: '2.3.0' + } + }]); + + const testClient = new TestClient(mapConfig); + + testClient.getTile(0, 0, 0, {}, function (err, res) { + if (err) { + return done(err); + } + + assert.equal(res.headers['cache-control'], `public,max-age=${ttl}`); + testClient.drain(done); + }); + }); + + it('tile from a table wich is NOT included in cdb_tablemetada', function (done) { + const ttl = global.environment.varnish.fallbackTtl || FIVE_MINUTES_IN_SECONDS; + const mapConfig = createMapConfig([{ + type: 'cartodb', + options: { + sql: 'select * from test_table_2', + cartocss: TestClient.CARTOCSS.POINTS, + cartocss_version: '2.3.0' + } + }]); + + const testClient = new TestClient(mapConfig); + + testClient.getTile(0, 0, 0, {}, function (err, res) { + if (err) { + return done(err); + } + + assert.equal(res.headers['cache-control'], `public,max-age=${ttl}`); + testClient.drain(done); + }); + }); + + it('tile from a dynamic query which doesn\'t use a table' , function (done) { + const ttl = global.environment.varnish.fallbackTtl || FIVE_MINUTES_IN_SECONDS; + const mapConfig = createMapConfig(); + + const testClient = new TestClient(mapConfig); + + testClient.getTile(0, 0, 0, {}, function (err, res) { + if (err) { + return done(err); + } + + assert.equal(res.headers['cache-control'], `public,max-age=${ttl}`); + testClient.drain(done); + }); + }); + }); +}); From a894194b6bd455cdc7e65541933c523194ea8ad9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 4 Jul 2019 15:58:39 +0200 Subject: [PATCH 4/8] Add more specific tests --- .../api/middlewares/cache-control-header.js | 6 +++- test/acceptance/cache/cache-control-header.js | 35 ++++++++++++++++++- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/api/middlewares/cache-control-header.js b/lib/cartodb/api/middlewares/cache-control-header.js index 7171c44a..1ef3f2df 100644 --- a/lib/cartodb/api/middlewares/cache-control-header.js +++ b/lib/cartodb/api/middlewares/cache-control-header.js @@ -24,7 +24,7 @@ module.exports = function setCacheControlHeader ({ const directives = [ 'public' ]; - if (affectedTables && !affectedTables.getTables().some(table => !!table.updated_at)) { + if (everyAffectedTablesCanBeInvalidated(affectedTables)) { directives.push(`max-age=${fallbackTtl}`); } else { directives.push(`max-age=${ttl}`); @@ -40,3 +40,7 @@ module.exports = function setCacheControlHeader ({ }); }; }; + +function everyAffectedTablesCanBeInvalidated (affectedTables) { + return affectedTables && affectedTables.getTables().some(table => !table.updated_at); +} diff --git a/test/acceptance/cache/cache-control-header.js b/test/acceptance/cache/cache-control-header.js index f318214d..40bc0355 100644 --- a/test/acceptance/cache/cache-control-header.js +++ b/test/acceptance/cache/cache-control-header.js @@ -72,8 +72,41 @@ describe('cache-control header', function () { }); }); - it('tile from a dynamic query which doesn\'t use a table' , function (done) { + it('tile from joined tables which one of them is NOT included in cdb_tablemetada', function (done) { const ttl = global.environment.varnish.fallbackTtl || FIVE_MINUTES_IN_SECONDS; + const mapConfig = createMapConfig([{ + type: 'cartodb', + options: { + sql: ` + select + t.cartodb_id, + t.the_geom, + t.the_geom_webmercator + from + test_table t, + test_table_2 t2 + where + t.cartodb_id = t2.cartodb_id + `, + cartocss: TestClient.CARTOCSS.POINTS, + cartocss_version: '2.3.0' + } + }]); + + const testClient = new TestClient(mapConfig); + + testClient.getTile(0, 0, 0, {}, function (err, res) { + if (err) { + return done(err); + } + + assert.equal(res.headers['cache-control'], `public,max-age=${ttl}`); + testClient.drain(done); + }); + }); + + it('tile from a dynamic query which doesn\'t use a table' , function (done) { + const ttl = ONE_YEAR_IN_SECONDS; const mapConfig = createMapConfig(); const testClient = new TestClient(mapConfig); From 5ca498d0f39b5db6f3b48213775303b638692dff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 4 Jul 2019 16:18:43 +0200 Subject: [PATCH 5/8] Typo --- test/acceptance/cache/cache-control-header.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/acceptance/cache/cache-control-header.js b/test/acceptance/cache/cache-control-header.js index 40bc0355..6bd4e9e3 100644 --- a/test/acceptance/cache/cache-control-header.js +++ b/test/acceptance/cache/cache-control-header.js @@ -26,7 +26,7 @@ function createMapConfig (layers = defaultLayers) { describe('cache-control header', function () { describe('max-age directive', function () { - it('tile from a table wich is included in cdb_tablemetada', function (done) { + it('tile from a table which is included in cdb_tablemetada', function (done) { const ttl = ONE_YEAR_IN_SECONDS; const mapConfig = createMapConfig([{ type: 'cartodb', @@ -49,7 +49,7 @@ describe('cache-control header', function () { }); }); - it('tile from a table wich is NOT included in cdb_tablemetada', function (done) { + it('tile from a table which is NOT included in cdb_tablemetada', function (done) { const ttl = global.environment.varnish.fallbackTtl || FIVE_MINUTES_IN_SECONDS; const mapConfig = createMapConfig([{ type: 'cartodb', From ec0c0eb810b712262c6b11a9691c7d9f50fe0656 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 4 Jul 2019 16:24:17 +0200 Subject: [PATCH 6/8] Improve readability --- lib/cartodb/api/middlewares/cache-control-header.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/api/middlewares/cache-control-header.js b/lib/cartodb/api/middlewares/cache-control-header.js index 1ef3f2df..c3bb9673 100644 --- a/lib/cartodb/api/middlewares/cache-control-header.js +++ b/lib/cartodb/api/middlewares/cache-control-header.js @@ -25,9 +25,9 @@ module.exports = function setCacheControlHeader ({ const directives = [ 'public' ]; if (everyAffectedTablesCanBeInvalidated(affectedTables)) { - directives.push(`max-age=${fallbackTtl}`); - } else { directives.push(`max-age=${ttl}`); + } else { + directives.push(`max-age=${fallbackTtl}`); } if (revalidate) { @@ -42,5 +42,5 @@ module.exports = function setCacheControlHeader ({ }; function everyAffectedTablesCanBeInvalidated (affectedTables) { - return affectedTables && affectedTables.getTables().some(table => !table.updated_at); + return affectedTables && affectedTables.getTables().every(table => !!table.updated_at); } From 4def4b0341860f76bf871593a3ac0a6ad960f651 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 4 Jul 2019 16:41:30 +0200 Subject: [PATCH 7/8] Improve condition --- lib/cartodb/api/middlewares/cache-control-header.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/api/middlewares/cache-control-header.js b/lib/cartodb/api/middlewares/cache-control-header.js index c3bb9673..c3c57ea0 100644 --- a/lib/cartodb/api/middlewares/cache-control-header.js +++ b/lib/cartodb/api/middlewares/cache-control-header.js @@ -42,5 +42,5 @@ module.exports = function setCacheControlHeader ({ }; function everyAffectedTablesCanBeInvalidated (affectedTables) { - return affectedTables && affectedTables.getTables().every(table => !!table.updated_at); + return affectedTables && affectedTables.getTables().every(table => table.updated_at !== null); } From 8820e348700647c5701cde469aaf6cd8210c9faa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 5 Jul 2019 15:31:17 +0200 Subject: [PATCH 8/8] Update news --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 188102fc..00616ad3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -5,6 +5,7 @@ Released 2019-XX-XX Announcements: +- Cache control header fine tuning. Set a shorter value for "max-age" directive if there is no way to know when to trigger the invalidation. - Update deps: - windshaft@5.3.0: - Update @carto/mapnik to [`3.6.2-carto.15`](https://github.com/CartoDB/node-mapnik/blob/v3.6.2-carto.15/CHANGELOG.carto.md#362-carto15).