Handle SQL API errors by logging them and requesting NO cache
SQL api is used to determine the list of source tables affected by a query. Before this commit, the X-Cache-Channel header set on sql api error was an arbitrary 'table' string, now the header is omitted, the error logged and Cache-Control and Pragma headers are sent as an attempt to request no caching. The code includes test for this mechanism.
This commit is contained in:
Sandro Santilli
parent
49aad435b9
commit
e8cbc666e2
@ -48,8 +48,10 @@ var config = {
|
||||
}
|
||||
,sqlapi: {
|
||||
protocol: 'http',
|
||||
host: 'localhost.lan',
|
||||
port: 8080,
|
||||
host: '',
|
||||
// This port will be used by "make check" for testing purposes
|
||||
// It must be available
|
||||
port: 1080,
|
||||
version: 'v1'
|
||||
}
|
||||
,varnish: {
|
||||
|
||||
@ -36,7 +36,7 @@ function generateCacheChannel(req, callback){
|
||||
|
||||
// use cache if present
|
||||
if (!_.isNull(channelCache[sql_md5]) && !_.isUndefined(channelCache[sql_md5])) {
|
||||
callback(channelCache[sql_md5]);
|
||||
callback(null, channelCache[sql_md5]);
|
||||
} else{
|
||||
// strip out windshaft/mapnik inserted sql if present
|
||||
var sql = req.params.sql.match(/^\((.*)\)\sas\scdbq$/);
|
||||
@ -54,21 +54,28 @@ function generateCacheChannel(req, callback){
|
||||
}
|
||||
|
||||
// call sql api
|
||||
request.get({url:sqlapi, qs:qs, json:true}, function(err, response, body){
|
||||
if (!err && response.statusCode == 200) {
|
||||
tableNames = body.rows[0].cdb_querytables.split(/^\{(.*)\}$/)[1];
|
||||
} else {
|
||||
//oops, no SQL API. Just cache using fallback 'table' key
|
||||
tableNames = 'table';
|
||||
request.get({url:sqlapi, qs:qs, json:true}, function(err, res, body){
|
||||
var epref = 'could not detect source tables using SQL api at ' + sqlapi;
|
||||
if (err){
|
||||
var msg = err.message ? err.message : err;
|
||||
callback(new Error(epref + ': ' + msg));
|
||||
return;
|
||||
}
|
||||
if (res.statusCode != 200) {
|
||||
var msg = res.body.error ? res.body.error : res.body;
|
||||
callback(new Error(epref + ': ' + msg));
|
||||
return;
|
||||
}
|
||||
var qtables = body.rows[0].cdb_querytables;
|
||||
tableNames = qtables.split(/^\{(.*)\}$/)[1];
|
||||
cacheChannel = buildCacheChannel(dbName,tableNames);
|
||||
channelCache[sql_md5] = cacheChannel; // store for caching
|
||||
callback(cacheChannel);
|
||||
callback(null, cacheChannel);
|
||||
});
|
||||
}
|
||||
} else {
|
||||
cacheChannel = buildCacheChannel(dbName,tableNames);
|
||||
callback(cacheChannel);
|
||||
callback(null, cacheChannel);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -57,17 +57,27 @@ module.exports = function(){
|
||||
// skip non-GET requests, or requests for which there's no response
|
||||
if ( req.method != 'GET' || ! req.res ) { cb(null, null); return; }
|
||||
var res = req.res;
|
||||
var ttl = global.environment.varnish.ttl || 86400;
|
||||
Cache.generateCacheChannel(req, function(channel){
|
||||
res.header('X-Cache-Channel', channel);
|
||||
var cache_policy = req.query.cache_policy;
|
||||
if ( cache_policy == 'persist' ) {
|
||||
res.header('Cache-Control', 'public,max-age=31536000'); // 1 year
|
||||
} else {
|
||||
var ttl = global.environment.varnish.ttl || 86400;
|
||||
res.header('Last-Modified', new Date().toUTCString());
|
||||
res.header('Cache-Control', 'no-cache,max-age='+ttl+',must-revalidate, public');
|
||||
}
|
||||
cb(null, channel); // add last-modified too ?
|
||||
Cache.generateCacheChannel(req, function(err, channel){
|
||||
if ( ! err ) {
|
||||
res.header('X-Cache-Channel', channel);
|
||||
cb(null, channel);
|
||||
} else {
|
||||
// avoid caching this result
|
||||
// (temptative, what Varnish does is out of our control)
|
||||
res.header('Cache-Control', 'no-cache,no-store,max-age=0,must-revalidate');
|
||||
res.header('Pragma', 'no-cache');
|
||||
console.log('ERROR generating cache channel: ' + ( err.message ? err.message : err ));
|
||||
// TODO: evaluate if we should bubble up the error instead
|
||||
cb(null, 'ERROR');
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
@ -127,7 +137,7 @@ module.exports = function(){
|
||||
if (!_.isNull(data))
|
||||
_.extend(req.params, {geom_type: data});
|
||||
|
||||
that.addCacheChannel(req, function(err, chan) {
|
||||
that.addCacheChannel(req, function(err) {
|
||||
callback(err, req);
|
||||
});
|
||||
}
|
||||
|
||||
@ -6,6 +6,8 @@ var querystring = require('querystring');
|
||||
var semver = require('semver');
|
||||
var mapnik = require('mapnik');
|
||||
var Step = require('step');
|
||||
var http = require('http');
|
||||
var url = require('url');
|
||||
|
||||
require(__dirname + '/../support/test_helper');
|
||||
|
||||
@ -17,6 +19,7 @@ server.setMaxListeners(0);
|
||||
suite('server', function() {
|
||||
|
||||
var redis_client = redis.createClient(global.environment.redis.port);
|
||||
var sqlapi_server;
|
||||
|
||||
var default_style = semver.satisfies(mapnik.versions.mapnik, '<2.1.0')
|
||||
?
|
||||
@ -30,7 +33,19 @@ suite('server', function() {
|
||||
var test_style_black_200 = "#test_table{marker-fill:black;marker-line-color:red;marker-width:10}";
|
||||
var test_style_black_210 = "#test_table{marker-fill:black;marker-line-color:red;marker-width:20}";
|
||||
|
||||
suiteSetup(function(){
|
||||
suiteSetup(function(done){
|
||||
sqlapi_server = http.createServer(function(req,res) {
|
||||
var query = url.parse(req.url, true).query;
|
||||
if ( query.q.match('SQLAPIERROR') ) {
|
||||
res.statusCode = 400;
|
||||
res.write(JSON.stringify({'error':'Some error occurred'}));
|
||||
} else {
|
||||
res.write(JSON.stringify({rows: [ { 'cdb_querytables': '{' +
|
||||
JSON.stringify(query) + '}' } ]}));
|
||||
}
|
||||
res.end();
|
||||
});
|
||||
sqlapi_server.listen(global.environment.sqlapi.port, done);
|
||||
});
|
||||
|
||||
/////////////////////////////////////////////////////////////////////////////////
|
||||
@ -916,6 +931,69 @@ suite('server', function() {
|
||||
);
|
||||
});
|
||||
|
||||
test("uses sqlapi to figure source data of query", function(done){
|
||||
var qo = {
|
||||
sql: "SELECT g.cartodb_id, g.codineprov, t.the_geom_webmercator "
|
||||
+ "FROM gadm4 g, test_table t "
|
||||
+ "WHERE g.cartodb_id = t.cartodb_id",
|
||||
map_key: 1234
|
||||
};
|
||||
var sqlapi;
|
||||
Step(
|
||||
function sendRequest(err) {
|
||||
assert.response(server, {
|
||||
headers: {host: 'localhost'},
|
||||
url: '/tiles/gadm4/6/31/24.png?' + querystring.stringify(qo),
|
||||
method: 'GET'
|
||||
},{}, this);
|
||||
},
|
||||
function checkResponse(res) {
|
||||
assert.equal(res.statusCode, 200, res.statusCode + ': ' + res.body);
|
||||
var ct = res.headers['content-type'];
|
||||
assert.equal(ct, 'image/png');
|
||||
var cc = res.headers['x-cache-channel'];
|
||||
var dbname = 'cartodb_test_user_1_db'
|
||||
assert.equal(cc.substring(0, dbname.length), dbname);
|
||||
var jsonquery = cc.substring(dbname.length+1);
|
||||
var sentquery = JSON.parse(jsonquery);
|
||||
assert.equal(sentquery.api_key, qo.map_key);
|
||||
assert.equal(sentquery.q, 'SELECT CDB_QueryTables($windshaft$' + qo.sql + '$windshaft$)');
|
||||
done();
|
||||
}
|
||||
);
|
||||
});
|
||||
|
||||
test("requests to skip cache on sqlapi error", function(done){
|
||||
var qo = {
|
||||
sql: "SELECT g.cartodb_id, g.codineprov, t.the_geom_webmercator "
|
||||
+ ", 'SQLAPIERROR' is not null "
|
||||
+ "FROM gadm4 g, test_table t "
|
||||
+ "WHERE g.cartodb_id = t.cartodb_id",
|
||||
map_key: 1234
|
||||
};
|
||||
var sqlapi;
|
||||
Step(
|
||||
function sendRequest(err) {
|
||||
assert.response(server, {
|
||||
headers: {host: 'localhost'},
|
||||
url: '/tiles/gadm4/6/31/24.png?' + querystring.stringify(qo),
|
||||
method: 'GET'
|
||||
},{}, this);
|
||||
},
|
||||
function checkResponse(res) {
|
||||
assert.equal(res.statusCode, 200, res.statusCode + ': ' + res.body);
|
||||
var ct = res.headers['content-type'];
|
||||
assert.equal(ct, 'image/png');
|
||||
// does NOT send an x-cache-channel
|
||||
assert.ok(!res.headers.hasOwnProperty('x-cache-channel'));
|
||||
// attempts to tell varnish NOT to cache
|
||||
assert.equal(res.headers['cache-control'], 'no-cache,no-store,max-age=0,must-revalidate');
|
||||
assert.equal(res.headers['pragma'], 'no-cache');
|
||||
done();
|
||||
}
|
||||
);
|
||||
});
|
||||
|
||||
/////////////////////////////////////////////////////////////////////////////////
|
||||
//
|
||||
// DELETE CACHE
|
||||
@ -1040,7 +1118,7 @@ suite('server', function() {
|
||||
// 'map_style|null|publicuser|my_table',
|
||||
redis_client.keys("map_style|*", function(err, matches) {
|
||||
_.each(matches, function(k) { redis_client.del(k); });
|
||||
done();
|
||||
sqlapi_server.close(done);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@ -97,8 +97,6 @@ ALTER TABLE ONLY gadm4
|
||||
|
||||
CREATE INDEX bdll25_provincias_4326_2_the_geom_webmercator_idx ON gadm4 USING gist (the_geom_webmercator);
|
||||
|
||||
-- development_cartodb_user_3 role
|
||||
CREATE USER development_cartodb_user_3;
|
||||
GRANT ALL ON TABLE gadm4 TO development_cartodb_user_3;
|
||||
GRANT ALL ON TABLE gadm4 TO test_cartodb_user_1;
|
||||
GRANT SELECT ON TABLE gadm4 TO publicuser;
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user