From 68aa8544774f8c3451f568736a4fa4660a8c0076 Mon Sep 17 00:00:00 2001 From: Sandro Santilli Date: Thu, 20 Mar 2014 17:11:56 +0100 Subject: [PATCH 01/36] Prepare for 1.10.0 --- NEWS.md | 3 +++ package.json | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index e0f0f8c2..7f532581 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,6 @@ +1.10.0 - 2014-MM-DD +------------------- + 1.9.0 - 2014-03-20 ------------------ diff --git a/package.json b/package.json index ac9e902f..b07b7195 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "keywords": [ "cartodb" ], - "version": "1.9.0", + "version": "1.10.0", "repository": { "type": "git", "url": "git://github.com/CartoDB/CartoDB-SQL-API.git" From ee61ccfd944884b731ffaab4f1ef23de67702b22 Mon Sep 17 00:00:00 2001 From: Sandro Santilli Date: Thu, 27 Mar 2014 11:18:02 +0100 Subject: [PATCH 02/36] Extend paging test Tests page=0, POST and authentication --- test/acceptance/app.test.js | 78 ++++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 18 deletions(-) diff --git a/test/acceptance/app.test.js b/test/acceptance/app.test.js index 0bba121e..a3d596c5 100644 --- a/test/acceptance/app.test.js +++ b/test/acceptance/app.test.js @@ -230,24 +230,66 @@ function(done){ // Test page and rows_per_page params test("paging", function(done){ - assert.response(app, { - url: '/api/v1/sql?' + querystring.stringify({ - // note: select casing intentionally mixed - q: 'SELECT * FROM (VALUES(1),(2),(3),(4),(5),(6),(7),(8),(9)) t(v)', - rows_per_page: 3, - page: 2 - }), - headers: {host: 'vizzuality.cartodb.com'}, - method: 'GET' - },{ }, function(res) { - assert.equal(res.statusCode, 200, res.body); - var parsed = JSON.parse(res.body); - assert.equal(parsed.rows.length, 3); - assert.equal(parsed.rows[0].v, 7); - assert.equal(parsed.rows[1].v, 8); - assert.equal(parsed.rows[2].v, 9); - done(); - }); + var sql = 'SELECT * FROM (VALUES(1),(2),(3),(4),(5),(6),(7),(8),(9)) t(v)'; + var pr = [ [2,3], [0,4] ]; // page and rows + var methods = [ 'GET', 'POST' ]; + var authorized = 0; + var testing = 0; + var method = 0; + var testNext = function() { + if ( testing >= pr.length ) { + if ( method+1 >= methods.length ) { + if ( authorized ) { + done(); + return; + } else { + authorized = 1; + method = 0; + testing = 0; + } + } else { + testing = 0; + ++method; + } + } + var prcur = pr[testing++]; + console.log("Test " + testing + "/" + pr.length + + " method " + methods[method] + " " + + ( authorized ? "authenticated" : "" ) ); + var page = prcur[0]; + var nrows = prcur[1]; + var data_obj = { + q: sql, + rows_per_page: nrows, + page: page + }; + if ( authorized ) data_obj['api_key'] = '1234'; + var data = querystring.stringify(data_obj); + var req = { + url: '/api/v1/sql', + headers: {host: 'vizzuality.cartodb.com'} + }; + if ( methods[method] == 'GET' ) { + req.method = 'GET'; + req.url += '?' + data; + } else { + req.method = 'POST'; + req.headers['Content-Type'] = 'application/x-www-form-urlencoded'; + req.data = data; + } + assert.response(app, req, {}, function(res) { + assert.equal(res.statusCode, 200, res.body); + var parsed = JSON.parse(res.body); + assert.equal(parsed.rows.length, nrows); + for (var i=0; i Date: Thu, 27 Mar 2014 12:47:34 +0100 Subject: [PATCH 03/36] Fix paging with queries starting with comments Closes #144 Includes testcase --- app/models/psql.js | 3 +++ test/acceptance/app.test.js | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/app/models/psql.js b/app/models/psql.js index 633f948f..043a61da 100644 --- a/app/models/psql.js +++ b/app/models/psql.js @@ -269,6 +269,9 @@ PSQL.window_sql = function(sql, limit, offset) { // only window select functions (NOTE: "values" will be broken, "with" will be broken) if (!_.isNumber(limit) || !_.isNumber(offset) ) return sql; + // Strip comments + sql = sql.replace(/(^|\n)\s*--.*\n/g, ''); + var cte = ''; if ( sql.match(/^\s*WITH\s/i) ) { diff --git a/test/acceptance/app.test.js b/test/acceptance/app.test.js index a3d596c5..76d2d655 100644 --- a/test/acceptance/app.test.js +++ b/test/acceptance/app.test.js @@ -292,6 +292,33 @@ test("paging", function(done){ testNext(); }); +// Test paging with WITH queries +test("paging starting with comment", function(done){ + var sql = "-- this is a comment\n" + + "SELECT * FROM (VALUES(1),(2),(3),(4),(5),(6),(7),(8),(9)) t(v)"; + var nrows = 3; + var page = 2; + assert.response(app, { + url: '/api/v1/sql?' + querystring.stringify({ + q: sql, + rows_per_page: nrows, + page: page + }), + headers: {host: 'vizzuality.cartodb.com'}, + method: 'GET' + }, {}, function(res) { + assert.equal(res.statusCode, 200, res.body); + var parsed = JSON.parse(res.body); + assert.equal(parsed.rows.length, 3); + for (var i=0; i Date: Thu, 27 Mar 2014 13:01:51 +0100 Subject: [PATCH 04/36] Add 1.9.1 section in NEWS file --- NEWS.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/NEWS.md b/NEWS.md index 7f532581..b7640d40 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,6 +1,13 @@ 1.10.0 - 2014-MM-DD ------------------- +1.9.1 - 2014-03-27 +------------------ + +Bug fixes: + + * Fix paging with queries starting with comments (#144) + 1.9.0 - 2014-03-20 ------------------ From 4606a449175872d151bf46d50546e5733e702944 Mon Sep 17 00:00:00 2001 From: Sandro Santilli Date: Wed, 2 Apr 2014 17:17:59 +0200 Subject: [PATCH 05/36] Fix testsuite with GDAL-1.11dev installed (current master) --- test/acceptance/export/kml.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/test/acceptance/export/kml.js b/test/acceptance/export/kml.js index 1d4c7f1f..2cc7cc22 100644 --- a/test/acceptance/export/kml.js +++ b/test/acceptance/export/kml.js @@ -271,8 +271,11 @@ test('GET /api/v1/sql as kml with no rows', function(done){ method: 'GET' },{ }, function(res){ assert.equal(res.statusCode, 200, res.body); - var body = 'cartodb_query'; - assert.equal(res.body.replace(/\n/g,''), body); + // NOTE: GDAL-1.11+ added 'id="root_doc"' attribute to the output + var pat = RegExp('^<\\?xml version="1.0" encoding="utf-8" \\?>cartodb_query$'); + var body = res.body.replace(/\n/g,''); + assert.ok(body.match(pat), + "Response:\n" + body + '\ndoes not match pattern:\n' + pat); done(); }); }); @@ -288,8 +291,11 @@ test('GET /api/v1/sql as kml with ending semicolon', function(done){ method: 'GET' },{ }, function(res){ assert.equal(res.statusCode, 200, res.body); - var body = 'cartodb_query'; - assert.equal(res.body.replace(/\n/g,''), body); + // NOTE: GDAL-1.11+ added 'id="root_doc"' attribute to the output + var pat = RegExp('^<\\?xml version="1.0" encoding="utf-8" \\?>cartodb_query$'); + var body = res.body.replace(/\n/g,''); + assert.ok(body.match(pat), + "Response:\n" + body + '\ndoes not match pattern:\n' + pat); done(); }); }); From 49ef1bc0c776fc330ff5c575f7115e5d76f55f09 Mon Sep 17 00:00:00 2001 From: Sandro Santilli Date: Thu, 3 Apr 2014 15:43:34 +0200 Subject: [PATCH 06/36] Stream JSON responses Reduces memory usage for big datasets. JIRA CDB-2600 #resolve --- NEWS.md | 4 ++ app/models/formats/json.js | 92 ++++++++++++++++++++++++++++++++++---- 2 files changed, 87 insertions(+), 9 deletions(-) diff --git a/NEWS.md b/NEWS.md index b7640d40..bea11914 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,6 +1,10 @@ 1.10.0 - 2014-MM-DD ------------------- +Enhancements: + + * Stream JSON responses + 1.9.1 - 2014-03-27 ------------------ diff --git a/app/models/formats/json.js b/app/models/formats/json.js index f30eff27..16c4817b 100644 --- a/app/models/formats/json.js +++ b/app/models/formats/json.js @@ -1,4 +1,5 @@ var pg = require('./pg'); +var util = require('util'); function json() {} @@ -42,23 +43,96 @@ p.formatResultFields = function(flds) { return nfields; } - -p.transform = function(result, options, callback) { - var j = { - time: options.total_time, - fields: this.formatResultFields(result.fields), - total_rows: result.rowCount, - rows: result.rows +p.startStreaming = function() { + if ( this.opts.profiler ) this.opts.profiler.done('startStreaming'); + this.total_rows = 0; + if ( this.opts.beforeSink ) { + this.opts.beforeSink(); + delete this.opts.beforeSink; } + var out = '{"rows":['; + this.opts.sink.write(out); + this._streamingStarted = true; +}; + +p.handleQueryRow = function(row) { + if ( ! this._streamingStarted ) { + this.startStreaming(); + } + var sf = this.opts.skipfields; + if ( sf.length ){ + for ( var j=0; j Date: Wed, 9 Apr 2014 09:21:16 +0200 Subject: [PATCH 07/36] Update release document --- HOWTO_RELEASE | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/HOWTO_RELEASE b/HOWTO_RELEASE index 99eebf60..1d65b876 100644 --- a/HOWTO_RELEASE +++ b/HOWTO_RELEASE @@ -1,11 +1,20 @@ -1. Ensure proper version in package.json -2. Ensure NEWS section exists for the new version, review it, add release date -3. Drop npm-shrinkwrap.json -4. Run npm install -5. Test (make check or npm test), fix if broken before proceeding -6. Run npm shrinkwrap -7. Commit package.json, npm-shrinwrap.json, NEWS -8. Tag Major.Minor.Patch -9. Announce -10. Stub NEWS/package for next version +1. Test (make clean all check), fix if broken before proceeding +2. Ensure proper version in package.json +3. Ensure NEWS section exists for the new version, review it, add release date +4. Drop npm-shrinkwrap.json +5. Run npm shrinkwrap to recreate npm-shrinkwrap.json +6. Commit package.json, npm-shrinwrap.json, NEWS +7. git tag -a Major.Minor.Patch # use NEWS section as content +8. Announce on cartodb@googlegroups.com +9. Stub NEWS/package for next version +Versions: + +Bugfix releases increment Patch component of version. +Feature releases increment Minor and set Patch to zero. +If backward compatibility is broken, increment Major and +set to zero Minor and Patch. + +Branches named 'b.' are kept for any critical +fix that might need to be shipped before next feature release +is ready. From d10bd8e3b01bf19592a148a6bde023a00177231d Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Fri, 11 Apr 2014 12:03:43 +0200 Subject: [PATCH 08/36] precompiled query may write regex --- app/controllers/app.js | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/app/controllers/app.js b/app/controllers/app.js index 6fe84a93..627c76df 100755 --- a/app/controllers/app.js +++ b/app/controllers/app.js @@ -71,7 +71,7 @@ Date.prototype.toJSON = function() { s += ( offset < 0 ? '+' : '-' ) + pad(Math.abs(offset / 60)) + pad(Math.abs(offset % 60)) - + } return s; } @@ -165,19 +165,16 @@ app.get(global.settings.base_url+'/version', function(req, res) { res.send(getVersion()); }); -// Return true of the given query may write to the database -// -// NOTE: this is a fuzzy check, the return could be true even -// if the query doesn't really write anything. -// But you can be pretty sure of a false return. -// +var sqlQueryMayWriteRegex = new RegExp("\\b(alter|insert|update|delete|create|drop|reindex|truncate)\\b", "i"); +/** + * This is a fuzzy check, the return could be true even if the query doesn't really write anything. But you can be + * pretty sure of a false return. + * + * @param sql + * @returns {boolean} Return true of the given query may write to the database + */ function queryMayWrite(sql) { - var mayWrite = false; - var pattern = RegExp("\\b(alter|insert|update|delete|create|drop|reindex|truncate)\\b", "i"); - if ( pattern.test(sql) ) { - mayWrite = true; - } - return mayWrite; + return sqlQueryMayWriteRegex.test(sql); } function sanitize_filename(filename) { From 909530cfb30bcb8afe68d90b88df16d8b00772ba Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Fri, 11 Apr 2014 12:13:44 +0200 Subject: [PATCH 09/36] documentation for sql parameter --- app/controllers/app.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/app.js b/app/controllers/app.js index 627c76df..dc1992bb 100755 --- a/app/controllers/app.js +++ b/app/controllers/app.js @@ -170,7 +170,7 @@ var sqlQueryMayWriteRegex = new RegExp("\\b(alter|insert|update|delete|create|dr * This is a fuzzy check, the return could be true even if the query doesn't really write anything. But you can be * pretty sure of a false return. * - * @param sql + * @param sql The SQL statement to check against * @returns {boolean} Return true of the given query may write to the database */ function queryMayWrite(sql) { From cdb4e5f19bdd7d9dae1c70387ea685a8e4d5857c Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Fri, 11 Apr 2014 12:03:43 +0200 Subject: [PATCH 10/36] precompiled query may write regex --- app/controllers/app.js | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/app/controllers/app.js b/app/controllers/app.js index 6fe84a93..627c76df 100755 --- a/app/controllers/app.js +++ b/app/controllers/app.js @@ -71,7 +71,7 @@ Date.prototype.toJSON = function() { s += ( offset < 0 ? '+' : '-' ) + pad(Math.abs(offset / 60)) + pad(Math.abs(offset % 60)) - + } return s; } @@ -165,19 +165,16 @@ app.get(global.settings.base_url+'/version', function(req, res) { res.send(getVersion()); }); -// Return true of the given query may write to the database -// -// NOTE: this is a fuzzy check, the return could be true even -// if the query doesn't really write anything. -// But you can be pretty sure of a false return. -// +var sqlQueryMayWriteRegex = new RegExp("\\b(alter|insert|update|delete|create|drop|reindex|truncate)\\b", "i"); +/** + * This is a fuzzy check, the return could be true even if the query doesn't really write anything. But you can be + * pretty sure of a false return. + * + * @param sql + * @returns {boolean} Return true of the given query may write to the database + */ function queryMayWrite(sql) { - var mayWrite = false; - var pattern = RegExp("\\b(alter|insert|update|delete|create|drop|reindex|truncate)\\b", "i"); - if ( pattern.test(sql) ) { - mayWrite = true; - } - return mayWrite; + return sqlQueryMayWriteRegex.test(sql); } function sanitize_filename(filename) { From 32f2d2ce45eeeef73d577684ca003fa408945fac Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Fri, 11 Apr 2014 12:13:44 +0200 Subject: [PATCH 11/36] documentation for sql parameter --- app/controllers/app.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/app.js b/app/controllers/app.js index 627c76df..dc1992bb 100755 --- a/app/controllers/app.js +++ b/app/controllers/app.js @@ -170,7 +170,7 @@ var sqlQueryMayWriteRegex = new RegExp("\\b(alter|insert|update|delete|create|dr * This is a fuzzy check, the return could be true even if the query doesn't really write anything. But you can be * pretty sure of a false return. * - * @param sql + * @param sql The SQL statement to check against * @returns {boolean} Return true of the given query may write to the database */ function queryMayWrite(sql) { From 537cb238c64f2d316e90db242bc18541e6d9315f Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Mon, 14 Apr 2014 15:01:12 +0200 Subject: [PATCH 12/36] CDB-2081 Adds support for order_by through http query params. --- app/controllers/app.js | 5 +- app/models/psql.js | 95 ++++----------------- app/sql/psql_wrapper.js | 129 +++++++++++++++++++++++++++++ doc/API.md | 10 +++ test/unit/psql.test.js | 34 -------- test/unit/sql/psql_wrapper.test.js | 97 ++++++++++++++++++++++ 6 files changed, 257 insertions(+), 113 deletions(-) create mode 100644 app/sql/psql_wrapper.js create mode 100644 test/unit/sql/psql_wrapper.test.js diff --git a/app/controllers/app.js b/app/controllers/app.js index dc1992bb..8c973210 100755 --- a/app/controllers/app.js +++ b/app/controllers/app.js @@ -37,6 +37,7 @@ var express = require('express') // global.settings.app_root + '/app/models/metadata') , oAuth = require(global.settings.app_root + '/app/models/oauth') , PSQL = require(global.settings.app_root + '/app/models/psql') + , PSQLWrapper = require(global.settings.app_root + '/app/sql/psql_wrapper') , CdbRequest = require(global.settings.app_root + '/app/models/cartodb_request') , ApiKeyAuth = require(global.settings.app_root + '/app/models/apikey_auth') , _ = require('underscore') @@ -195,6 +196,8 @@ function handleQuery(req, res) { var database = params.database; // TODO: Deprecate var limit = parseInt(params.rows_per_page); var offset = parseInt(params.page); + var orderBy = params.order_by; + var sortOrder = params.sort_order; var requestedFormat = params.format; var format = _.isArray(requestedFormat) ? _.last(requestedFormat) : requestedFormat; var requestedFilename = params.filename; @@ -447,7 +450,7 @@ function handleQuery(req, res) { checkAborted('generateFormat'); // TODO: drop this, fix UI! - sql = PSQL.window_sql(sql,limit,offset); + sql = new PSQLWrapper(sql).orderBy(orderBy, sortOrder).window(limit, offset).query(); var opts = { dbopts: dbopts, diff --git a/app/models/psql.js b/app/models/psql.js index 043a61da..eb403b56 100644 --- a/app/models/psql.js +++ b/app/models/psql.js @@ -1,6 +1,7 @@ -var _ = require('underscore') - , Step = require('step') - , pg = require('pg');//.native; // disabled for now due to: https://github.com/brianc/node-postgres/issues/48 +var _ = require('underscore'), + PSQLWrapper = require('../sql/psql_wrapper'), + Step = require('step'), + pg = require('pg');//.native; // disabled for now due to: https://github.com/brianc/node-postgres/issues/48 _.mixin(require('underscore.string')); // Max database connections in the pool @@ -261,81 +262,19 @@ var PSQL = function(dbopts) { return me; }; -// little hack for UI -// -// TODO:drop, fix in the UI (it's not documented in doc/API) -// + +/** + * Little hack for UI + * TODO: drop, fix in the UI (it's not documented in doc/API) + * + * @param {string} sql + * @param {number} limit + * @param {number} offset + * @returns {string} The wrapped SQL query with the limit window + */ PSQL.window_sql = function(sql, limit, offset) { - // only window select functions (NOTE: "values" will be broken, "with" will be broken) - if (!_.isNumber(limit) || !_.isNumber(offset) ) return sql; - - // Strip comments - sql = sql.replace(/(^|\n)\s*--.*\n/g, ''); - - var cte = ''; - - if ( sql.match(/^\s*WITH\s/i) ) { - - var rem = sql; // analyzed portion of sql - var q; // quote char - var n = 0; // nested parens level - var s = 0; // 0:outQuote, 1:inQuote - var l; - while (1) { - l = rem.search(/[("')]/); -//console.log("REM Is " + rem); - if ( l < 0 ) { - console.log("Malformed SQL"); - return sql; - } - var f = rem.charAt(l); -//console.log("n:" + n + " s:" + s + " l:" + l + " charAt(l):" + f + " charAt(l+1):" + rem.charAt(l+1)); - if ( s == 0 ) { - if ( f == '(' ) ++n; - else if ( f == ')' ) { - if ( ! --n ) { // end of CTE - cte += rem.substr(0, l+1); - rem = rem.substr(l+1); - //console.log("Out of cte, rem is " + rem); - if ( rem.search(/^s*,/) < 0 ) break; - else continue; // cte and rem already updated - } - } - else { // enter quoting - s = 1; q = f; - } - } - else if ( f == q ) { - if ( rem.charAt(l+1) == f ) ++l; // escaped - else s = 0; // exit quoting - } - cte += rem.substr(0, l+1); - rem = rem.substr(l+1); - } -/* -console.log("cte: " + cte); -console.log("rem: " + rem); -*/ - sql = rem; //sql.substr(l+1); - } - - - var re_SELECT = RegExp(/^\s*SELECT\s/i); - var re_INTO = RegExp(/\sINTO\s+([^\s]+|"([^"]|"")*")\s*$/i); - - //console.log("SQL " + sql); - //console.log(" does " + ( sql.match(re_SELECT) ? '' : 'not ' ) + "match re_SELECT " + re_SELECT); - //console.log(" does " + ( sql.match(re_INTO) ? '' : 'not ' ) + "match re_INTO " + re_INTO); - - if ( - sql.match(re_SELECT) && - ! sql.match(re_INTO) - ) - { - return cte + "SELECT * FROM (" + sql + ") AS cdbq_1 LIMIT " + limit + " OFFSET " + offset; - } - - return cte + sql; -} + // keeping it here for backwards compatibility + return new PSQLWrapper(sql).window(limit, offset).query(); +}; module.exports = PSQL; diff --git a/app/sql/psql_wrapper.js b/app/sql/psql_wrapper.js new file mode 100644 index 00000000..599db0b4 --- /dev/null +++ b/app/sql/psql_wrapper.js @@ -0,0 +1,129 @@ +'use strict'; + +var _ = require('underscore'), + util = require('util'), + SORT_ORDER_OPTIONS = {ASC: 1, DESC: 1}, + REGEX_SELECT = /^\s*SELECT\s/i, + REGEX_INTO = /\sINTO\s+([^\s]+|"([^"]|"")*")\s*$/i; + +function PSQLWrapper(sql) { + this.sqlQuery = sql; + this.sqlClauses = { + orderBy: '', + limit: '' + }; +} + +/** + * Only window select functions (NOTE: "values" will be broken, "with" will be broken) + * + * @param {number} limit + * @param {number} offset + * @returns {PSQLWrapper} + */ +PSQLWrapper.prototype.window = function (limit, offset) { + if (!_.isNumber(limit) || !_.isNumber(offset)) { + return this; + } + this.sqlClauses.limit = util.format(' LIMIT %d OFFSET %d', limit, offset); + return this; +}; + +/** + * + * @param {string} column The name of the column to sort by + * @param {string} sortOrder Whether it's ASC or DESC ordering + * @returns {PSQLWrapper} + */ +PSQLWrapper.prototype.orderBy = function (column, sortOrder) { + if (!_.isString(column) || _.isEmpty(column)) { + return this; + } + this.sqlClauses.orderBy = util.format(' ORDER BY "%s"', column); + + if (!_.isUndefined(sortOrder)) { + sortOrder = sortOrder.toUpperCase(); + if (SORT_ORDER_OPTIONS[sortOrder]) { + this.sqlClauses.orderBy += util.format(' %s', sortOrder); + } + } + return this; +}; + +/** + * Builds an SQL query with extra clauses based on the builder calls. + * + * @returns {string} The SQL query with the extra clauses + */ +PSQLWrapper.prototype.query = function () { + // Strip comments + this.sqlQuery = this.sqlQuery.replace(/(^|\n)\s*--.*\n/g, ''); + + var cte = ''; + + if (this.sqlQuery.match(/^\s*WITH\s/i)) { + + var rem = this.sqlQuery, // analyzed portion of sql + q, // quote char + n = 0, // nested parens level + s = 0, // 0:outQuote, 1:inQuote + l; + while (1) { + l = rem.search(/[("')]/); + // console.log("REM Is " + rem); + if (l < 0) { + // console.log("Malformed SQL"); + return this.sqlQuery; + } + var f = rem.charAt(l); + // console.log("n:" + n + " s:" + s + " l:" + l + " charAt(l):" + f + " charAt(l+1):" + rem.charAt(l+1)); + if (s == 0) { + if (f == '(') { + ++n; + } else if (f == ')') { + if (!--n) { // end of CTE + cte += rem.substr(0, l + 1); + rem = rem.substr(l + 1); + //console.log("Out of cte, rem is " + rem); + if (rem.search(/^s*,/) < 0) { + break; + } else { + continue; // cte and rem already updated + } + } + } else { // enter quoting + s = 1; + q = f; + } + } else if (f == q) { + if (rem.charAt(l + 1) == f) { + ++l; // escaped + } else { + s = 0; // exit quoting + } + } + cte += rem.substr(0, l + 1); + rem = rem.substr(l + 1); + } + /* + console.log("cte: " + cte); + console.log("rem: " + rem); + */ + this.sqlQuery = rem; //sql.substr(l+1); + } + + //console.log("SQL " + sql); + //console.log(" does " + ( sql.match(REGEX_SELECT) ? '' : 'not ' ) + "match REGEX_SELECT " + REGEX_SELECT); + //console.log(" does " + ( sql.match(REGEX_INTO) ? '' : 'not ' ) + "match REGEX_INTO " + REGEX_INTO); + + if (this.sqlQuery.match(REGEX_SELECT) && !this.sqlQuery.match(REGEX_INTO)) { + return util.format( + '%sSELECT * FROM (%s) AS cdbq_1%s%s', + cte, this.sqlQuery, this.sqlClauses.orderBy, this.sqlClauses.limit + ); + } + + return cte + this.sqlQuery; +}; + +module.exports = PSQLWrapper; diff --git a/doc/API.md b/doc/API.md index 7f1bba84..a808ebe0 100644 --- a/doc/API.md +++ b/doc/API.md @@ -43,6 +43,16 @@ Supported query string parameters: used to specify which page to start returning rows from. First page has index 0. + 'order_by': + Causes the result rows to be sorted according to the specified + case sensitive column name. See also 'sort_order'. + + 'sort_order': + Optional param combined with 'order_by' one. Values are limited + to ASC (ascending) and DESC (descending), case insensitive. If + not specified or wrongly specified, ASC is assumed by default. + + Response formats ---------------- diff --git a/test/unit/psql.test.js b/test/unit/psql.test.js index ce44a7a6..77646d94 100644 --- a/test/unit/psql.test.js +++ b/test/unit/psql.test.js @@ -86,40 +86,6 @@ test('test public user cannot execute INSERT on db', function(done){ }); }); -test('Windowed SQL with simple select', function(){ - // NOTE: intentionally mixed-case and space-padded - var sql = "\n \tSEleCT * from table1"; - var out = PSQL.window_sql(sql, 1, 0); - assert.equal(out, "SELECT * FROM (" + sql + ") AS cdbq_1 LIMIT 1 OFFSET 0"); -}); - -test('Windowed SQL with CTE select', function(){ - // NOTE: intentionally mixed-case and space-padded - var cte = "\n \twiTh x as( update test set x=x+1)"; - var select = "\n \tSEleCT * from x"; - var sql = cte + select; - var out = PSQL.window_sql(sql, 1, 0); - assert.equal(out, cte + "SELECT * FROM (" + select + ") AS cdbq_1 LIMIT 1 OFFSET 0"); -}); - -test('Windowed SQL with CTE update', function(){ - // NOTE: intentionally mixed-case and space-padded - var cte = "\n \twiTh a as( update test set x=x+1)"; - var upd = "\n \tupdate tost set y=x from x"; - var sql = cte + upd; - var out = PSQL.window_sql(sql, 1, 0); - assert.equal(out, sql); -}); - -test('Windowed SQL with complex CTE and insane quoting', function(){ - // NOTE: intentionally mixed-case and space-padded - var cte = "\n \twiTh \"('a\" as( update \"\"\"test)\" set x='x'+1), \")b(\" as ( select ')))\"' from z )"; - var sel = "\n \tselect '\"' from x"; - var sql = cte + sel; - var out = PSQL.window_sql(sql, 1, 0); - assert.equal(out, cte + "SELECT * FROM (" + sel + ") AS cdbq_1 LIMIT 1 OFFSET 0"); -}); - test('dbkey depends on dbopts', function(){ var opt1 = _.clone(dbopts_anon); opt1.dbname = 'dbname1'; diff --git a/test/unit/sql/psql_wrapper.test.js b/test/unit/sql/psql_wrapper.test.js new file mode 100644 index 00000000..937833e3 --- /dev/null +++ b/test/unit/sql/psql_wrapper.test.js @@ -0,0 +1,97 @@ +var _ = require('underscore'), + PSQLWrapper = require('../../../app/sql/psql_wrapper'), + assert = require('assert'); + + +// NOTE: intentionally mixed-case and space-padded +var simpleSql = "\n \tSEleCT * from table1"; + +suite('psql_wrapper', function() { + + test('Windowed SQL with simple select', function(){ + var out = new PSQLWrapper(simpleSql).window(1, 0).query(); + + assert.equal(out, "SELECT * FROM (" + simpleSql + ") AS cdbq_1 LIMIT 1 OFFSET 0"); + }); + + test('Windowed SQL with CTE select', function(){ + // NOTE: intentionally mixed-case and space-padded + var cte = "\n \twiTh x as( update test set x=x+1)"; + var select = "\n \tSEleCT * from x"; + var sql = cte + select; + + var out = new PSQLWrapper(sql).window(1, 0).query(); + + assert.equal(out, cte + "SELECT * FROM (" + select + ") AS cdbq_1 LIMIT 1 OFFSET 0"); + }); + + test('Windowed SQL with CTE update', function(){ + // NOTE: intentionally mixed-case and space-padded + var cte = "\n \twiTh a as( update test set x=x+1)"; + var upd = "\n \tupdate tost set y=x from x"; + var sql = cte + upd; + + var out = new PSQLWrapper(sql).window(1, 0).query(); + + assert.equal(out, sql); + }); + + test('Windowed SQL with complex CTE and insane quoting', function(){ + // NOTE: intentionally mixed-case and space-padded + var cte = "\n \twiTh \"('a\" as( update \"\"\"test)\" set x='x'+1), \")b(\" as ( select ')))\"' from z )"; + var sel = "\n \tselect '\"' from x"; + var sql = cte + sel; + + var out = new PSQLWrapper(sql).window(1, 0).query(); + + assert.equal(out, cte + "SELECT * FROM (" + sel + ") AS cdbq_1 LIMIT 1 OFFSET 0"); + }); + + test('Different instances return different queries', function() { + var aWrapper = new PSQLWrapper('select 1'); + var bWrapper = new PSQLWrapper('select * from databaseB'); + + assert.notEqual(aWrapper, bWrapper); + assert.notEqual(aWrapper.query(), bWrapper.query(), 'queries should be different'); + }); + + test('Order by SQL with simple select and empty column name', function() { + var expectedSql = 'SELECT * FROM (' + simpleSql + ') AS cdbq_1'; + + var outputSql = new PSQLWrapper(simpleSql).orderBy('').query(); + + assert.equal(outputSql, expectedSql); + }); + + test('Order by SQL with simple select and no sort order', function() { + var expectedSql = 'SELECT * FROM (' + simpleSql + ') AS cdbq_1 ORDER BY "foo"'; + + var outputSql = new PSQLWrapper(simpleSql).orderBy('foo').query(); + + assert.equal(outputSql, expectedSql); + }); + + test('Order by SQL with simple select and invalid sort order use no sort order', function() { + var expectedSql = 'SELECT * FROM (' + simpleSql + ') AS cdbq_1 ORDER BY "foo"'; + + var outputSql = new PSQLWrapper(simpleSql).orderBy('foo', "BAD_SORT_ORDER").query(); + + assert.equal(outputSql, expectedSql); + }); + + test('Order by SQL with simple select and asc order', function() { + var expectedSql = 'SELECT * FROM (' + simpleSql + ') AS cdbq_1 ORDER BY "foo" ASC'; + + var outputSql = new PSQLWrapper(simpleSql).orderBy('foo', "asc").query(); + + assert.equal(outputSql, expectedSql); + }); + + test('Order by SQL with simple select and DESC order', function() { + var expectedSql = 'SELECT * FROM (' + simpleSql + ') AS cdbq_1 ORDER BY "foo" DESC'; + + var outputSql = new PSQLWrapper(simpleSql).orderBy('foo', "DESC").query(); + + assert.equal(outputSql, expectedSql); + }); +}); From 3e6b63c9d6d92b2b41f3ac2d61f2eed1e6327cf6 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Mon, 14 Apr 2014 15:44:43 +0200 Subject: [PATCH 13/36] CDB-2081 Returns original query in case there are no clauses to apply. --- app/sql/psql_wrapper.js | 3 +++ test/unit/sql/psql_wrapper.test.js | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/sql/psql_wrapper.js b/app/sql/psql_wrapper.js index 599db0b4..b9c5caf9 100644 --- a/app/sql/psql_wrapper.js +++ b/app/sql/psql_wrapper.js @@ -56,6 +56,9 @@ PSQLWrapper.prototype.orderBy = function (column, sortOrder) { * @returns {string} The SQL query with the extra clauses */ PSQLWrapper.prototype.query = function () { + if (_.isEmpty(this.sqlClauses.orderBy) && _.isEmpty(this.sqlClauses.limit)) { + return this.sqlQuery; + } // Strip comments this.sqlQuery = this.sqlQuery.replace(/(^|\n)\s*--.*\n/g, ''); diff --git a/test/unit/sql/psql_wrapper.test.js b/test/unit/sql/psql_wrapper.test.js index 937833e3..27e6ef45 100644 --- a/test/unit/sql/psql_wrapper.test.js +++ b/test/unit/sql/psql_wrapper.test.js @@ -55,8 +55,8 @@ suite('psql_wrapper', function() { assert.notEqual(aWrapper.query(), bWrapper.query(), 'queries should be different'); }); - test('Order by SQL with simple select and empty column name', function() { - var expectedSql = 'SELECT * FROM (' + simpleSql + ') AS cdbq_1'; + test('Order by SQL with simple select and empty column name returns original query', function() { + var expectedSql = simpleSql; var outputSql = new PSQLWrapper(simpleSql).orderBy('').query(); From ff70d8bcf9fdbfdd772bdfdefe5af8d1408b488b Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 15 Apr 2014 14:57:38 +0200 Subject: [PATCH 14/36] Adds sql unit tests directory for sql_wrapper to npm package. --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index b07b7195..515bc7d9 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "libxmljs": "~0.8.1" }, "scripts": { - "test": "test/run_tests.sh ${RUNTESTFLAGS} test/unit/*.js test/unit/model/*.js test/acceptance/*.js test/acceptance/export/*.js" + "test": "test/run_tests.sh ${RUNTESTFLAGS} test/unit/*.js test/unit/model/*.js test/unit/sql/*.js test/acceptance/*.js test/acceptance/export/*.js" }, "engines": { "node": ">= 0.4.1 < 0.9" } } From 8dbf1d9c453bde5eafea9f34454fe9bb335c9860 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 15 Apr 2014 15:29:03 +0200 Subject: [PATCH 15/36] Removes ending semicolons from SQL queries. Fixes #147 --- app/sql/psql_wrapper.js | 2 +- test/unit/sql/psql_wrapper.test.js | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/app/sql/psql_wrapper.js b/app/sql/psql_wrapper.js index b9c5caf9..f2691b5c 100644 --- a/app/sql/psql_wrapper.js +++ b/app/sql/psql_wrapper.js @@ -7,7 +7,7 @@ var _ = require('underscore'), REGEX_INTO = /\sINTO\s+([^\s]+|"([^"]|"")*")\s*$/i; function PSQLWrapper(sql) { - this.sqlQuery = sql; + this.sqlQuery = sql.replace(/;\s*$/, ''); this.sqlClauses = { orderBy: '', limit: '' diff --git a/test/unit/sql/psql_wrapper.test.js b/test/unit/sql/psql_wrapper.test.js index 27e6ef45..9b6d0def 100644 --- a/test/unit/sql/psql_wrapper.test.js +++ b/test/unit/sql/psql_wrapper.test.js @@ -94,4 +94,22 @@ suite('psql_wrapper', function() { assert.equal(outputSql, expectedSql); }); + + test('Query with ending semicolon returns without it', function() { + var expectedSql = 'select a, ( a - min(a) over() ) / ( ( max(a) over () - min(a) over () ) / 4 ) as interval from ( select test as a from quantile_test ) as f', + query = expectedSql + ';'; + + var outputSql = new PSQLWrapper(query).query(); + + assert.equal(outputSql, expectedSql); + }); + + test('Several queries with semicolon get only last semicolon removed', function() { + var expectedSql = 'SELECT 1; SELECT 2; SELECT 3', + query = expectedSql + ';'; + + var outputSql = new PSQLWrapper(query).query(); + + assert.equal(outputSql, expectedSql); + }); }); From 2d2c056d6b0ced43069b906e42e84d1eebf8071f Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 22 Apr 2014 11:15:35 +0200 Subject: [PATCH 16/36] Removes spawn unused dependency --- app/controllers/app.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/controllers/app.js b/app/controllers/app.js index 8c973210..4b157882 100755 --- a/app/controllers/app.js +++ b/app/controllers/app.js @@ -27,7 +27,6 @@ var express = require('express') , os = require('os') , zlib = require('zlib') , util = require('util') - , spawn = require('child_process').spawn , Profiler = require('step-profiler') , StatsD = require('node-statsd').StatsD , Meta = require('cartodb-redis')({ From fa94cc5718ece3d5f8a36ca24e8b052861a7910a Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 7 May 2014 11:29:49 +0200 Subject: [PATCH 17/36] Updates NEWS with latest features and bug fixes --- NEWS.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/NEWS.md b/NEWS.md index bea11914..f9f5790f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,9 +1,18 @@ 1.10.0 - 2014-MM-DD ------------------- +New features: + + * Order by and sort order through http query params + Enhancements: * Stream JSON responses + * Pre-compiling may write regex + +Bug fixes: + + * Support trailing semicolons (#147) 1.9.1 - 2014-03-27 ------------------ From 51b135c0accca2bc20e24ced3ea052df4b690aae Mon Sep 17 00:00:00 2001 From: Sandro Santilli Date: Wed, 7 May 2014 16:14:17 +0200 Subject: [PATCH 18/36] Set default PostgreSQL application name to "cartodb_sqlapi" --- NEWS.md | 1 + app/controllers/app.js | 3 +++ test/run_tests.sh | 1 + 3 files changed, 5 insertions(+) diff --git a/NEWS.md b/NEWS.md index f9f5790f..f8543ec4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -9,6 +9,7 @@ Enhancements: * Stream JSON responses * Pre-compiling may write regex + * Set default PostgreSQL application name to "cartodb_sqlapi" Bug fixes: diff --git a/app/controllers/app.js b/app/controllers/app.js index 4b157882..fce05e81 100755 --- a/app/controllers/app.js +++ b/app/controllers/app.js @@ -15,6 +15,9 @@ // // +if ( ! process.env['PGAPPNAME'] ) + process.env['PGAPPNAME']='cartodb_sqlapi_XXX'; + function App() { var path = require('path'); diff --git a/test/run_tests.sh b/test/run_tests.sh index 5306b65e..a1a3ceab 100755 --- a/test/run_tests.sh +++ b/test/run_tests.sh @@ -2,6 +2,7 @@ # To make output dates deterministic export TZ='Europe/Rome' +export PGAPPNAME='cartodb_sqlapi_tester' OPT_CREATE_PGSQL=yes # create/prepare the postgresql test database OPT_CREATE_REDIS=yes # create/prepare the redis test databases From 4b5a5921d5554035f70f3aa0f941db9e79dfaa33 Mon Sep 17 00:00:00 2001 From: Sandro Santilli Date: Wed, 7 May 2014 16:15:46 +0200 Subject: [PATCH 19/36] Fix application_name to "cartodb_sqlapi" --- app/controllers/app.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/app.js b/app/controllers/app.js index fce05e81..8af1a41f 100755 --- a/app/controllers/app.js +++ b/app/controllers/app.js @@ -16,7 +16,7 @@ // if ( ! process.env['PGAPPNAME'] ) - process.env['PGAPPNAME']='cartodb_sqlapi_XXX'; + process.env['PGAPPNAME']='cartodb_sqlapi'; function App() { From dd1c5cc5eb1f27d0331c76fd80dba98b58fa73c7 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 27 May 2014 11:51:44 +0200 Subject: [PATCH 20/36] Adds support to append logs to file with log4js. It listens to HUP signal to re-open the log file. In case the specified log filename is not writeable the server will not start. --- app.js | 33 ++++++++++++++++++---- config/environments/development.js.example | 3 ++ config/environments/production.js.example | 3 ++ config/environments/staging.js.example | 3 ++ config/environments/test.js.example | 3 ++ 5 files changed, 40 insertions(+), 5 deletions(-) diff --git a/app.js b/app.js index d8c8c8d1..3bd03412 100755 --- a/app.js +++ b/app.js @@ -9,7 +9,9 @@ * environments: [development, test, production] * */ -var _ = require('underscore'); +var _ = require('underscore'), + fs = require('fs'), + path = require('path'); if ( process.argv[2] ) ENV = process.argv[2]; else if ( process.env['NODE_ENV'] ) ENV = process.env['NODE_ENV']; @@ -31,12 +33,28 @@ _.extend(global.settings, env); global.log4js = require('log4js') log4js_config = { - appenders: [ - { type: "console", layout: { type:'basic' } } - ], + appenders: [], replaceConsole:true }; +if ( env.log_filename ) { + var logdir = path.dirname(env.log_filename); + // See cwd inlog4js.configure call below + logdir = path.resolve(__dirname, logdir); + if ( ! fs.existsSync(logdir) ) { + console.error("Log filename directory does not exist: " + logdir); + process.exit(1); + } + console.log("Logs will be written to " + env.log_filename); + log4js_config.appenders.push( + { type: "file", filename: env.log_filename } + ); +} else { + log4js_config.appenders.push( + { type: "console", layout: { type:'basic' } } + ); +} + if ( global.settings.rollbar ) { log4js_config.appenders.push({ type: __dirname + "/app/models/log4js_rollbar.js", @@ -44,7 +62,7 @@ if ( global.settings.rollbar ) { }); } -log4js.configure(log4js_config); +log4js.configure(log4js_config, { cwd: __dirname }); global.logger = log4js.getLogger(); @@ -64,3 +82,8 @@ app.listen(global.settings.node_port, global.settings.node_host, function() { process.on('uncaughtException', function(err) { logger.error('Uncaught exception: ' + err.stack); }); + +process.on('SIGHUP', function() { + log4js.configure(log4js_config); + console.log('Log files reloaded'); +}); diff --git a/config/environments/development.js.example b/config/environments/development.js.example index bc0365c0..a2f6f497 100644 --- a/config/environments/development.js.example +++ b/config/environments/development.js.example @@ -4,6 +4,9 @@ module.exports.base_url = '/api/:version'; // steps taken for producing the response. module.exports.useProfiler = true; module.exports.log_format = '[:date] :req[X-Real-IP] :method :req[Host]:url :status :response-time ms -> :res[Content-Type] (:res[X-SQLAPI-Profiler])'; +// If log_filename is given logs will be written there, in append mode. Otherwise stdout is used (default). +// Log file will be re-opened on receiving the HUP signal +module.exports.log_filename = 'logs/cartodb-sql-api.log'; // Regular expression pattern to extract username // from hostname. Must have a single grabbing block. module.exports.user_from_host = '^(.*)\\.localhost'; diff --git a/config/environments/production.js.example b/config/environments/production.js.example index 772b5584..032bfd6d 100644 --- a/config/environments/production.js.example +++ b/config/environments/production.js.example @@ -4,6 +4,9 @@ module.exports.base_url = '/api/:version'; // steps taken for producing the response. module.exports.useProfiler = true; module.exports.log_format = '[:date] :req[X-Real-IP] :method :req[Host]:url :status :response-time ms -> :res[Content-Type] (:res[X-SQLAPI-Profiler])'; +// If log_filename is given logs will be written there, in append mode. Otherwise stdout is used (default). +// Log file will be re-opened on receiving the HUP signal +module.exports.log_filename = 'logs/cartodb-sql-api.log'; // Regular expression pattern to extract username // from hostname. Must have a single grabbing block. module.exports.user_from_host = '^(.*)\\.cartodb\\.com$'; diff --git a/config/environments/staging.js.example b/config/environments/staging.js.example index 07332b72..1841c368 100644 --- a/config/environments/staging.js.example +++ b/config/environments/staging.js.example @@ -4,6 +4,9 @@ module.exports.base_url = '/api/:version'; // steps taken for producing the response. module.exports.useProfiler = true; module.exports.log_format = '[:date] :req[X-Real-IP] :method :req[Host]:url :status :response-time ms -> :res[Content-Type] (:res[X-SQLAPI-Profiler])'; +// If log_filename is given logs will be written there, in append mode. Otherwise stdout is used (default). +// Log file will be re-opened on receiving the HUP signal +module.exports.log_filename = 'logs/cartodb-sql-api.log'; // Regular expression pattern to extract username // from hostname. Must have a single grabbing block. module.exports.user_from_host = '^(.*)\\.cartodb\\.com$'; diff --git a/config/environments/test.js.example b/config/environments/test.js.example index e87969f3..264833e2 100644 --- a/config/environments/test.js.example +++ b/config/environments/test.js.example @@ -4,6 +4,9 @@ module.exports.base_url = '/api/:version'; // steps taken for producing the response. module.exports.useProfiler = true; module.exports.log_format = '[:date] :req[X-Real-IP] :method :req[Host]:url :status :response-time ms -> :res[Content-Type] (:res[X-SQLAPI-Profiler])'; +// If log_filename is given logs will be written there, in append mode. Otherwise stdout is used (default). +// Log file will be re-opened on receiving the HUP signal +// module.exports.log_filename = 'logs/cartodb-sql-api.log'; // Regular expression pattern to extract username // from hostname. Must have a single grabbing block. module.exports.user_from_host = '^([^.]*)\\.'; From d045ca1e2e4249fe7b20e4354ea2c90bab5b260a Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 28 May 2014 16:07:06 +0200 Subject: [PATCH 21/36] adds document about what metrics we are tracking and some comments for each one --- doc/metrics.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 doc/metrics.md diff --git a/doc/metrics.md b/doc/metrics.md new file mode 100644 index 00000000..42673f3a --- /dev/null +++ b/doc/metrics.md @@ -0,0 +1,28 @@ +CartoDB-SQL-API metrics +======================= + +- **sqlapi.query**: time to return a query resultset from the API, splitted into: + + **sqlapi.query.init**: time to prepare params from the request + + **sqlapi.query.getDatabaseName**: time to retrieve the database associated to the query + + **sqlapi.query.verifyRequest_apikey**: time to retrieve user and verify access with api key + + **sqlapi.query.verifyRequest_oauth**: time to retrieve user and verify access with oauht + + **sqlapi.query.getUserDBHost**: time to retrieve the host for the database + + **sqlapi.query.getUserDBPass**: time to retrieve the user password for the database connection + + **sqlapi.query.queryExplain**: time to retrieve affected tables from the query + + **sqlapi.query.setHeaders**: time to set the headers + + **sqlapi.query.sendResponse**: time to start sending the response. + + **sqlapi.query.finish**: time to handle an exception + + **sqlapi.query.startStreaming**: (json) time to start streaming, from the moment the query it was requested. + * It's not getting into graphite right now. + + **sqlapi.query.gotRows**: (json) Time until it finished processing all rows in the resultset. + * It's sharing the key with pg so stats in graphite can have mixed numbers. + + **sqlapi.query.endStreaming** (json) Time to finish the preparation of the response data. + * It's not getting into graphite right now. + + **sqlapi.query.generate**: (ogr) Time to prepare and generate a response from ogr + + **sqlapi.query.gotRows**: (pg) Time until it finished processing all rows in the resultset. + * It's sharing the key with json so stats in graphite can have mixed numbers. + + **sqlapi.query.packageResult**: (pg) Time to transform between different formats + * It's not getting into graphite right now. + + **sqlapi.query.eventedQuery**: (pg) Time to prepare and execute the query +- **sqlapi.query.success**: number of successful queries +- **sqlapi.query.error**: number of failed queries From 0831756fa2f0f815947e77de17eb8f362ba62ffd Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 28 May 2014 17:29:50 +0200 Subject: [PATCH 22/36] adds headers to differentiate between timers and counters --- doc/metrics.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/metrics.md b/doc/metrics.md index 42673f3a..5dfe7b9c 100644 --- a/doc/metrics.md +++ b/doc/metrics.md @@ -1,6 +1,7 @@ CartoDB-SQL-API metrics ======================= +## Timers - **sqlapi.query**: time to return a query resultset from the API, splitted into: + **sqlapi.query.init**: time to prepare params from the request + **sqlapi.query.getDatabaseName**: time to retrieve the database associated to the query @@ -24,5 +25,7 @@ CartoDB-SQL-API metrics + **sqlapi.query.packageResult**: (pg) Time to transform between different formats * It's not getting into graphite right now. + **sqlapi.query.eventedQuery**: (pg) Time to prepare and execute the query + +## Counters - **sqlapi.query.success**: number of successful queries - **sqlapi.query.error**: number of failed queries From b3609696a3366232fb739bca14e47db872c7ea50 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Mon, 2 Jun 2014 14:48:38 +0200 Subject: [PATCH 23/36] requests associated with formats based on postgres expose a cancel method that will be called on client request abortion/cancelling so postgres can cancel ongoing queries --- app/controllers/app.js | 9 ++++++--- app/models/formats/pg.js | 11 ++++++++++- app/models/psql.js | 10 +++++++--- test/unit/psql.test.js | 8 ++++++++ 4 files changed, 31 insertions(+), 7 deletions(-) diff --git a/app/controllers/app.js b/app/controllers/app.js index 8af1a41f..86502144 100755 --- a/app/controllers/app.js +++ b/app/controllers/app.js @@ -217,8 +217,10 @@ function handleQuery(req, res) { req.aborted = false; req.on("close", function() { - console.log("Request closed unexpectedly (aborted?)"); - req.aborted = true; // TODO: there must be a builtin way to check this + if (req.formatter && _.isFunction(req.formatter.cancel)) { + req.formatter.cancel(); + } + req.aborted = true; // TODO: there must be a builtin way to check this }); function checkAborted(step) { @@ -404,8 +406,9 @@ function handleQuery(req, res) { } - var fClass = formats[format] + var fClass = formats[format]; formatter = new fClass(); + req.formatter = formatter; // configure headers for given format diff --git a/app/models/formats/pg.js b/app/models/formats/pg.js index 2002287e..13118106 100644 --- a/app/models/formats/pg.js +++ b/app/models/formats/pg.js @@ -40,6 +40,8 @@ pg.prototype.handleNotice = function(msg, result) { }; pg.prototype.handleQueryEnd = function(result) { + this.queryCanceller = undefined; + if ( this.error ) { this.callback(this.error); return; @@ -111,7 +113,8 @@ pg.prototype.sendResponse = function(opts, callback) { this.start_time = Date.now(); this.client = new PSQL(opts.dbopts); - this.client.eventedQuery(sql, function(err, query) { + this.client.eventedQuery(sql, function(err, query, queryCanceller) { + that.queryCanceller = queryCanceller; if (err) { callback(err); return; @@ -127,4 +130,10 @@ pg.prototype.sendResponse = function(opts, callback) { }); }; +pg.prototype.cancel = function() { + if (this.queryCanceller) { + this.queryCanceller.call(); + } +}; + module.exports = pg; diff --git a/app/models/psql.js b/app/models/psql.js index eb403b56..d96ce567 100644 --- a/app/models/psql.js +++ b/app/models/psql.js @@ -182,6 +182,7 @@ var PSQL = function(dbopts) { that.connect(this); }, function(err, client, done){ + var next = this; if (err) throw err; var query = client.query(sql); @@ -198,10 +199,13 @@ var PSQL = function(dbopts) { client.removeListener('notice', noticeListener); done(); }); - return query; + next(null, query, client); }, - function(err, query){ - callback(err, query) + function(err, query, client){ + var queryCanceller = function() { + pg.cancel(undefined, client, query); + }; + callback(err, query, queryCanceller); } ); }; diff --git a/test/unit/psql.test.js b/test/unit/psql.test.js index 77646d94..4cfd21e0 100644 --- a/test/unit/psql.test.js +++ b/test/unit/psql.test.js @@ -101,4 +101,12 @@ test('dbkey depends on dbopts', function(){ assert.ok(_.isString(pg1.dbkey()), "pg1 dbkey is " + pg1.dbkey()); }); + test('eventedQuery provisions a cancel mechanism to abort queries', function (done) { + var psql = new PSQL(dbopts_auth); + psql.eventedQuery("SELECT 1 as foo", function(err, query, queryCanceller) { + assert.ok(_.isFunction(queryCanceller)); + done(); + }); + }); + }); From 23254513e68ca933dddf024327f1e5c112c5c805 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Mon, 2 Jun 2014 18:57:45 +0200 Subject: [PATCH 24/36] removes unneeded comma --- app/models/formats/pg.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/formats/pg.js b/app/models/formats/pg.js index 13118106..fa2acdef 100644 --- a/app/models/formats/pg.js +++ b/app/models/formats/pg.js @@ -15,7 +15,7 @@ pg.prototype = { getFileExtension: function() { return this.id; - }, + } }; From 46ba38aa5fb9f5636b4a69d76667f962994d7c80 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 3 Jun 2014 15:18:14 +0200 Subject: [PATCH 25/36] try to use postg(res|is)-9.3 instead of 9.1 --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 45a2ba3c..ad3d11b1 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,8 +4,8 @@ before_install: # Removal of postgresql-9.1-postgis-scripts is needed due to a # bug in some upstream package. # See http://trac.osgeo.org/ubuntugis/ticket/39 - - sudo apt-get remove --purge -q postgresql-9.1-postgis-scripts - - sudo apt-get install -q postgresql-9.1-postgis gdal-bin + - sudo apt-get remove --purge -q postgresql-9.3-postgis-scripts + - sudo apt-get install -q postgresql-9.3-postgis gdal-bin - createdb template_postgis - psql -c "CREATE EXTENSION postgis" template_postgis - ./configure From 88438c4ba51a77625ed5d940288b68f49ad2efec Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 3 Jun 2014 15:45:01 +0200 Subject: [PATCH 26/36] CDB-3079 simplify installation relying on default postg(res|is) --- .travis.yml | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/.travis.yml b/.travis.yml index ad3d11b1..1008c932 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,13 +1,7 @@ before_install: - #- sudo apt-add-repository --yes ppa:ubuntugis/ppa - - sudo apt-get update -q - # Removal of postgresql-9.1-postgis-scripts is needed due to a - # bug in some upstream package. - # See http://trac.osgeo.org/ubuntugis/ticket/39 - - sudo apt-get remove --purge -q postgresql-9.3-postgis-scripts - - sudo apt-get install -q postgresql-9.3-postgis gdal-bin - - createdb template_postgis - - psql -c "CREATE EXTENSION postgis" template_postgis + - psql -c 'create database template_postgis;' -U postgres + - psql -c 'CREATE EXTENSION postgis;' -U postgres -d template_postgis + - psql -c 'CREATE EXTENSION postgis_topology;' -U postgres -d template_postgis - ./configure language: node_js From d31a0a01d58d9df01acf2cc5ebd202a7b277ecce Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 3 Jun 2014 16:15:31 +0200 Subject: [PATCH 27/36] CDB-3079 uses addons as per http://docs.travis-ci.com/user/using-postgresql/ --- .travis.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 1008c932..39c68b22 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,7 +1,9 @@ -before_install: +addons: + postgresql: "9.3" + +before_script: - psql -c 'create database template_postgis;' -U postgres - psql -c 'CREATE EXTENSION postgis;' -U postgres -d template_postgis - - psql -c 'CREATE EXTENSION postgis_topology;' -U postgres -d template_postgis - ./configure language: node_js From f2b8bf793d15d78550016184c58dce9888343d4a Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 3 Jun 2014 17:31:52 +0200 Subject: [PATCH 28/36] CDB-3079 installs gdal --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index 39c68b22..5b7f84b7 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,6 +2,7 @@ addons: postgresql: "9.3" before_script: + - sudo apt-get install -q gdal-bin - psql -c 'create database template_postgis;' -U postgres - psql -c 'CREATE EXTENSION postgis;' -U postgres -d template_postgis - ./configure From b109343a256d7d524512cad7f8e53468740d9fdd Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 3 Jun 2014 17:53:06 +0200 Subject: [PATCH 29/36] CDB-3079 tries to use cartodb flavour of postgres/postgis --- .travis.yml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 5b7f84b7..cd86b68b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,8 +1,9 @@ -addons: - postgresql: "9.3" - before_script: - sudo apt-get install -q gdal-bin + - sudo apt-add-repository --yes ppa:cartodb/postgresql-9.3 + - sudo apt-add-repository --yes ppa:cartodb/gis + - sudo apt-get update + - sudo apt-get install -q postgresql-9.3-postgis-2.1 - psql -c 'create database template_postgis;' -U postgres - psql -c 'CREATE EXTENSION postgis;' -U postgres -d template_postgis - ./configure From c38ff94492327d0ef0983bfa5a7f59a81f4ff7dd Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 3 Jun 2014 18:11:32 +0200 Subject: [PATCH 30/36] CDB-3079 yet another try to use cartodb flavour of postgres/postgis --- .travis.yml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index cd86b68b..6d6f2893 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,9 +1,17 @@ before_script: - - sudo apt-get install -q gdal-bin + - lsb_release -a + - sudo mv /etc/apt/sources.list.d/pgdg-source.list* /tmp + - sudo apt-get -qq purge postgis* postgresql* + - sudo rm -Rf /var/lib/postgresql /etc/postgresql - sudo apt-add-repository --yes ppa:cartodb/postgresql-9.3 - sudo apt-add-repository --yes ppa:cartodb/gis - sudo apt-get update - sudo apt-get install -q postgresql-9.3-postgis-2.1 + - sudo apt-get install -q postgresql-contrib-9.3 + - sudo apt-get install -q postgis + - sudo apt-get install -q gdal-bin + - echo -e "local\tall\tall\ttrust\nhost\tall\tall\t127.0.0.1/32\ttrust\nhost\tall\tall\t::1/128\ttrust" |sudo tee /etc/postgresql/9.3/main/pg_hba.conf + - sudo service postgresql restart - psql -c 'create database template_postgis;' -U postgres - psql -c 'CREATE EXTENSION postgis;' -U postgres -d template_postgis - ./configure From 277a34cb8381e8da4fa2ce92d04c158618822c0a Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 3 Jun 2014 18:33:40 +0200 Subject: [PATCH 31/36] CDB-3079 enforces postgres user in preparation script --- test/prepare_db.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/prepare_db.sh b/test/prepare_db.sh index 324bdc46..b4f075c9 100755 --- a/test/prepare_db.sh +++ b/test/prepare_db.sh @@ -65,8 +65,8 @@ export PGHOST PGPORT if test x"$PREPARE_PGSQL" = xyes; then echo "preparing postgres..." - dropdb ${TEST_DB} # 2> /dev/null # error expected if doesn't exist, but not otherwise - createdb -Ttemplate_postgis -EUTF8 ${TEST_DB} || die "Could not create test database" + dropdb -U postgres ${TEST_DB} # 2> /dev/null # error expected if doesn't exist, but not otherwise + createdb -U postgres -Ttemplate_postgis -EUTF8 ${TEST_DB} || die "Could not create test database" cat test.sql | sed "s/:PUBLICUSER/${PUBLICUSER}/" | sed "s/:PUBLICPASS/${PUBLICPASS}/" | From 987b3e03182382e26f46f51c2934055c89b2f94b Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 3 Jun 2014 18:49:29 +0200 Subject: [PATCH 32/36] CDB-3079 enforces postgres user in preparation script --- test/prepare_db.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/prepare_db.sh b/test/prepare_db.sh index b4f075c9..3c130510 100755 --- a/test/prepare_db.sh +++ b/test/prepare_db.sh @@ -72,11 +72,11 @@ if test x"$PREPARE_PGSQL" = xyes; then sed "s/:PUBLICPASS/${PUBLICPASS}/" | sed "s/:TESTUSER/${TESTUSER}/" | sed "s/:TESTPASS/${TESTPASS}/" | - psql -v ON_ERROR_STOP=1 ${TEST_DB} || exit 1 + psql -U postgres -v ON_ERROR_STOP=1 ${TEST_DB} || exit 1 # TODO: send in a single run, togheter with test.sql - psql -f support/CDB_QueryStatements.sql ${TEST_DB} - psql -f support/CDB_QueryTables.sql ${TEST_DB} + psql -U postgres -f support/CDB_QueryStatements.sql ${TEST_DB} + psql -U postgres -f support/CDB_QueryTables.sql ${TEST_DB} fi From 4aef9d2fd1aee84adc5c6c82c985759ab0480477 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 4 Jun 2014 15:21:36 +0200 Subject: [PATCH 33/36] CDB-2038 adds entry to news related to query cancelling --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index f8543ec4..c441e157 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,6 +4,7 @@ New features: * Order by and sort order through http query params + * Cancelling queries in Postgresql when HTTP request is aborted/closed Enhancements: From 6db9c3a074d8bf43a20de37ffd546d838c11c1e7 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 4 Jun 2014 15:38:52 +0200 Subject: [PATCH 34/36] Release 1.10.0 --- NEWS.md | 2 +- npm-shrinkwrap.json | 50 +++++++++++++++++++++++++++++++++------------ 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/NEWS.md b/NEWS.md index c441e157..f2a5690f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,4 +1,4 @@ -1.10.0 - 2014-MM-DD +1.10.0 - 2014-06-04 ------------------- New features: diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 027dd512..536102b0 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -1,6 +1,6 @@ { "name": "cartodb_sql_api", - "version": "1.9.1", + "version": "1.10.0", "dependencies": { "underscore": { "version": "1.3.3" @@ -66,7 +66,7 @@ "version": "0.1.16", "dependencies": { "bindings": { - "version": "1.1.1" + "version": "1.2.0" } } } @@ -102,7 +102,7 @@ "version": "2.2.4" }, "log4js": { - "version": "0.6.12", + "version": "0.6.14", "dependencies": { "async": { "version": "0.1.15" @@ -111,20 +111,32 @@ "version": "1.1.4" }, "readable-stream": { - "version": "1.0.26-2", + "version": "1.0.27-1", "dependencies": { + "core-util-is": { + "version": "1.0.1" + }, + "isarray": { + "version": "0.0.1" + }, "string_decoder": { "version": "0.10.25-1" + }, + "inherits": { + "version": "2.0.1" } } } } }, "rollbar": { - "version": "0.3.1", + "version": "0.3.6", "dependencies": { "node-uuid": { "version": "1.4.1" + }, + "json-stringify-safe": { + "version": "5.0.0" } } }, @@ -170,7 +182,7 @@ "version": "1.0.7" }, "debug": { - "version": "0.7.4" + "version": "0.8.1" }, "mkdirp": { "version": "0.3.5" @@ -179,7 +191,7 @@ "version": "3.2.3", "dependencies": { "minimatch": { - "version": "0.2.12", + "version": "0.2.14", "dependencies": { "sigmund": { "version": "1.0.0" @@ -187,7 +199,7 @@ } }, "graceful-fs": { - "version": "2.0.1" + "version": "2.0.3" }, "inherits": { "version": "2.0.1" @@ -197,10 +209,10 @@ } }, "zipfile": { - "version": "0.5.0", + "version": "0.5.2", "dependencies": { "node-pre-gyp": { - "version": "0.5.5", + "version": "0.5.8", "dependencies": { "nopt": { "version": "2.2.0", @@ -318,7 +330,7 @@ "version": "0.1.25", "dependencies": { "graceful-fs": { - "version": "2.0.2" + "version": "2.0.3" } } } @@ -340,7 +352,7 @@ "version": "0.1.25", "dependencies": { "graceful-fs": { - "version": "2.0.2" + "version": "2.0.3" }, "inherits": { "version": "2.0.1" @@ -353,6 +365,9 @@ "minimatch": { "version": "0.2.14", "dependencies": { + "lru-cache": { + "version": "2.5.0" + }, "sigmund": { "version": "1.0.0" } @@ -364,10 +379,19 @@ } }, "readable-stream": { - "version": "1.0.26-2", + "version": "1.0.26-4", "dependencies": { + "core-util-is": { + "version": "1.0.1" + }, + "isarray": { + "version": "0.0.1" + }, "string_decoder": { "version": "0.10.25-1" + }, + "inherits": { + "version": "2.0.1" } } }, From 55dcb508097211d10defbda1f0f702399ab35112 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 4 Jun 2014 15:44:13 +0200 Subject: [PATCH 35/36] Prepares for 1.10.1 --- NEWS.md | 3 +++ package.json | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index f2a5690f..c3819160 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,6 @@ +1.10.1 - 2014-mm-dd +------------------- + 1.10.0 - 2014-06-04 ------------------- diff --git a/package.json b/package.json index 515bc7d9..f545f932 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "keywords": [ "cartodb" ], - "version": "1.10.0", + "version": "1.10.1", "repository": { "type": "git", "url": "git://github.com/CartoDB/CartoDB-SQL-API.git" From 7906707130490318f569beef659417fabc0a9fea Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 4 Jun 2014 20:57:11 +0200 Subject: [PATCH 36/36] Revert "Stream JSON responses" This reverts commit 49ef1bc0c776fc330ff5c575f7115e5d76f55f09. Conflicts: NEWS.md --- NEWS.md | 3 ++ app/models/formats/json.js | 92 ++++---------------------------------- 2 files changed, 12 insertions(+), 83 deletions(-) diff --git a/NEWS.md b/NEWS.md index c3819160..314097b9 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ 1.10.1 - 2014-mm-dd ------------------- +Bug fixes: + + * Backing out Stream JSON responses 1.10.0 - 2014-06-04 ------------------- diff --git a/app/models/formats/json.js b/app/models/formats/json.js index 16c4817b..f30eff27 100644 --- a/app/models/formats/json.js +++ b/app/models/formats/json.js @@ -1,5 +1,4 @@ var pg = require('./pg'); -var util = require('util'); function json() {} @@ -43,96 +42,23 @@ p.formatResultFields = function(flds) { return nfields; } -p.startStreaming = function() { - if ( this.opts.profiler ) this.opts.profiler.done('startStreaming'); - this.total_rows = 0; - if ( this.opts.beforeSink ) { - this.opts.beforeSink(); - delete this.opts.beforeSink; + +p.transform = function(result, options, callback) { + var j = { + time: options.total_time, + fields: this.formatResultFields(result.fields), + total_rows: result.rowCount, + rows: result.rows } - var out = '{"rows":['; - this.opts.sink.write(out); - this._streamingStarted = true; -}; - -p.handleQueryRow = function(row) { - if ( ! this._streamingStarted ) { - this.startStreaming(); - } - var sf = this.opts.skipfields; - if ( sf.length ){ - for ( var j=0; j