From d294ec4ffa270df3ac401bb696dc8085bc5c44a0 Mon Sep 17 00:00:00 2001 From: javi Date: Wed, 11 Feb 2015 12:37:50 +0100 Subject: [PATCH 01/16] use geometry from the query instead that backend --- lib/assets/javascripts/cartodb/models/table.js | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/assets/javascripts/cartodb/models/table.js b/lib/assets/javascripts/cartodb/models/table.js index 2f4f5a116478..84e542b33042 100644 --- a/lib/assets/javascripts/cartodb/models/table.js +++ b/lib/assets/javascripts/cartodb/models/table.js @@ -99,6 +99,15 @@ this._data.bind('error', function(e, resp) { this.notice('error loading rows, check your SQL query', 'error', 5000); }, this); + + this._data.bind('reset', function() { + var view = this._data; + this.set({ + schema: view.schemaFromData(this.get('schema')), + geometry_types: view.getGeometryTypes() + }); + }, this); + this.retrigger('change', this._data, 'data:changed'); this.retrigger('saved', this._data, 'data:saved'); @@ -121,10 +130,8 @@ if(resp.name) { resp.id = resp.name; } - if (this.isInSQLView()) { - delete resp.schema; - delete resp.geometry_types; - } + delete resp.geometry_types; + delete resp.schema; return resp; }, From dfc3e7ac9406c33ce052135d820ffcdeafb423df Mon Sep 17 00:00:00 2001 From: javi Date: Wed, 11 Feb 2015 12:38:36 +0100 Subject: [PATCH 02/16] move methods from sqldata to table data. Ideally sqldata should not exists right now --- .../javascripts/cartodb/models/sqlview.js | 138 ----------------- .../javascripts/cartodb/models/tabledata.js | 143 +++++++++++++++++- .../test/spec/cartodb/models/table.spec.js | 88 ++++------- .../test/spec/cartodb/table/tableview.spec.js | 7 +- 4 files changed, 166 insertions(+), 210 deletions(-) diff --git a/lib/assets/javascripts/cartodb/models/sqlview.js b/lib/assets/javascripts/cartodb/models/sqlview.js index a2d0628ec143..71396a822886 100644 --- a/lib/assets/javascripts/cartodb/models/sqlview.js +++ b/lib/assets/javascripts/cartodb/models/sqlview.js @@ -25,35 +25,6 @@ cdb.admin.SQLViewData = cdb.admin.CartoDBTableData.extend({ } }, - /** - * we need to override sync to avoid the sql request to be sent by GET. - * For security reasons, we need them to be send as a PUT request. - * @method sync - * @param method {'save' || 'read' || 'delete' || 'create'} - * @param model {Object} - * @param options {Object} - */ - sync: function(method, model, options) { - if(!options) { options = {}; } - options.data = this._createUrlOptions(function(d) { - return d !== 'sql'; - }); - - if (cdb.admin.CartoDBTableMetadata.alterTableData(this.options.get('sql'))) { - options.data += "&q=" + encodeURIComponent(this.options.get('sql')); - options.type = 'POST'; - } else { - // when a geometry can be lazy fetched, don't fetch it - var fetchGeometry = 'cartodb_id' in this.query_schema; - options.data += "&q=" + encodeURIComponent(this.wrappedSQL(this.query_schema, [], !fetchGeometry)); - - if (options.data.length > 2048) { - options.type = 'POST'; - } - } - - return Backbone.sync.call(this, method, this, options); - }, _parseSQL: function(sql) { sql = sql.replace(/([^\\]){x}/g, '$10').replace(/\\{x}/g, '{x}') @@ -101,115 +72,6 @@ cdb.admin.SQLViewData = cdb.admin.CartoDBTableData.extend({ return this.options.get('sql'); }, - /** - * with the data from the rows fetch create an schema - * if the schema from original table is passed the method - * set the column types according to it - * return an empty list if no data was fetch - */ - schemaFromData: function(originalTableSchema) { - // build schema in format [ [field, type] , ...] - return cdb.admin.CartoDBTableMetadata.sortSchema(_(this.query_schema).map(function(v, k) { - return [k, v]; - })); - }, - - geometryTypeFromGeoJSON: function(geojson) { - try { - var geo = JSON.parse(geojson); - return geo.type - } catch(e) { - } - }, - - geometryTypeFromWKT: function(wkt) { - if(!wkt) return null; - var types = cdb.admin.WKT.types; - wkt = wkt.toUpperCase(); - for(var i = 0; i < types.length; ++i) { - var t = types[i]; - if (wkt.indexOf(t) !== -1) { - return t; - } - } - }, - - geometryTypeFromWKB: function(wkb) { - if(!wkb) return null; - - var typeMap = { - '0001': 'Point', - '0002': 'LineString', - '0003': 'Polygon', - '0004': 'MultiPoint', - '0005': 'MultiLineString', - '0006': 'MultiPolygon' - }; - - var bigendian = wkb[0] === '0' && wkb[1] === '0'; - var type = wkb.substring(2, 6); - if(!bigendian) { - // swap '0100' => '0001' - type = type[2] + type[3] + type[0] + type[1]; - } - return typeMap[type]; - - }, - - - // - // guesses from the first row the geometry types involved - // returns an empty array where there is no rows - // return postgist types, like st_GEOTYPE - // - getGeometryTypes: function() { - var row = null; - var i = this.size(); - while (i-- && !(row && row.get('the_geom'))) { - row = this.at(i); - } - if(!row) return []; - var geom = row.get('the_geom') || row.get('the_geom_webmercator'); - var geoType = this.geometryTypeFromWKB(geom) || this.geometryTypeFromWKT(geom); - if(geoType) { - return ['ST_' + geoType[0].toUpperCase() + geoType.substring(1).toLowerCase()]; - } - return []; - }, - - // fixes schema. Special geometry columns (the_geom) are fetch as geojson - // so the returned type is string. Fix it - _schemaFromQueryFields: function(fields) { - if ('the_geom' in fields) { - fields['the_geom'].type = 'geometry'; - } - if ('the_geom_webmercator' in fields) { - fields['the_geom_webmercator'].type = 'geometry'; - } - return cdb.admin.CartoDBTableData.prototype._schemaFromQueryFields.call(this, fields); - }, - - fetch: function(opts) { - var MAX_GET_LENGTH = 1024; - var self = this; - this.trigger('loading', opts); - // if the query changes the database just send it - if (cdb.admin.CartoDBTableMetadata.alterTableData(this.options.get('sql'))) { - self.elder('fetch', opts); - return; - } - - // use get to fetch the schema, probably cached - var sql = this.options.get('sql') || ''; - this._sqlQuery(_.template('select * from (<%= sql %>) __wrapped limit 0')(this.options.attributes), function(data) { - // get schema - self.query_schema = self._schemaFromQueryFields(data.fields); - self.elder('fetch', opts); - }, function (err) { - self.trigger('error', self, err); - }, sql.length > MAX_GET_LENGTH ? "POST" : "GET"); - }, - url: function() { return this.sqlApiUrl(); }, diff --git a/lib/assets/javascripts/cartodb/models/tabledata.js b/lib/assets/javascripts/cartodb/models/tabledata.js index bc8e1e3381b5..bc3776fd23b9 100644 --- a/lib/assets/javascripts/cartodb/models/tabledata.js +++ b/lib/assets/javascripts/cartodb/models/tabledata.js @@ -131,6 +131,7 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ return true; } + this.table.bind('change', function() { if (needsFetch(self.table) && self.table.get('name') && self.table.has('schema')) { self.fetch(); @@ -146,17 +147,17 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ this.modify_rows = d.rows.length === 0 && _.size(d.fields) === 0; this.affected_rows = d.affected_rows; this.lastPage = false; - this.query_schema = this._schemaFromQueryFields(d.fields); if(d.rows.length < this.options.get('rows_per_page')) { this.lastPage = true; } return d.rows; }, - _schemaFromQueryFields: function(f) { + // given fields array as they come from SQL create a map name -> type + _schemaFromQueryFields: function(fields) { var sc = {}; - for(k in f) { - sc[k] = f[k].type; + for(k in fields) { + sc[k] = fields[k].type; } return sc; }, @@ -231,7 +232,37 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ return this.sqlApiUrl(); }, + /** + * we need to override sync to avoid the sql request to be sent by GET. + * For security reasons, we need them to be send as a PUT request. + * @method sync + * @param method {'save' || 'read' || 'delete' || 'create'} + * @param model {Object} + * @param options {Object} + */ sync: function(method, model, options) { + if(!options) { options = {}; } + options.data = this._createUrlOptions(function(d) { + return d !== 'sql'; + }); + + if (cdb.admin.CartoDBTableMetadata.alterTableData(this.options.get('sql') || '')) { + options.data += "&q=" + encodeURIComponent(this.options.get('sql')); + options.type = 'POST'; + } else { + // when a geometry can be lazy fetched, don't fetch it + var fetchGeometry = 'cartodb_id' in this.query_schema; + options.data += "&q=" + encodeURIComponent(this.wrappedSQL(this.query_schema, [], !fetchGeometry)); + + if (options.data.length > 2048) { + options.type = 'POST'; + } + } + + return Backbone.sync.call(this, method, this, options); + }, + + /*sync: function(method, model, options) { if(!options) { options = {}; }; var query = this._createUrlOptions(); query += "&q=" + encodeURIComponent(this.wrappedSQL(this.table.columnTypes())); @@ -240,7 +271,7 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ } options.data = query; return Backbone.sync.call(this, method, this, options); - }, + },*/ sqlApiUrl: function() { var protocol = cdb.config.get("sql_api_port") == 443 ? 'https': 'http'; @@ -758,8 +789,106 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ self.fetched = true; success && success.apply(self, arguments); } - this.elder('fetch', opts); - } + this._fetch(opts); + }, + + _fetch: function(opts) { + var MAX_GET_LENGTH = 1024; + var self = this; + this.trigger('loading', opts); + + var sql = this.getSQL(); + // if the query changes the database just send it + if (cdb.admin.CartoDBTableMetadata.alterTableData(sql)) { + cdb.ui.common.TableData.prototype.fetch.call(self, opts); + return; + } + + // use get to fetch the schema, probably cached + this._sqlQuery(_.template('select * from (<%= sql %>) __wrapped limit 0')({ sql: sql }), function(data) { + // get schema + self.query_schema = self._schemaFromQueryFields(data.fields); + cdb.ui.common.TableData.prototype.fetch.call(self, opts); + }, function (err) { + self.trigger('error', self, err); + }, sql.length > MAX_GET_LENGTH ? "POST" : "GET"); + }, + + /** + * with the data from the rows fetch create an schema + * if the schema from original table is passed the method + * set the column types according to it + * return an empty list if no data was fetch + */ + schemaFromData: function(originalTableSchema) { + // build schema in format [ [field, type] , ...] + return cdb.admin.CartoDBTableMetadata.sortSchema(_(this.query_schema).map(function(v, k) { + return [k, v]; + })); + }, + + geometryTypeFromGeoJSON: function(geojson) { + try { + var geo = JSON.parse(geojson); + return geo.type + } catch(e) { + } + }, + + geometryTypeFromWKT: function(wkt) { + if(!wkt) return null; + var types = cdb.admin.WKT.types; + wkt = wkt.toUpperCase(); + for(var i = 0; i < types.length; ++i) { + var t = types[i]; + if (wkt.indexOf(t) !== -1) { + return t; + } + } + }, + + geometryTypeFromWKB: function(wkb) { + if(!wkb) return null; + + var typeMap = { + '0001': 'Point', + '0002': 'LineString', + '0003': 'Polygon', + '0004': 'MultiPoint', + '0005': 'MultiLineString', + '0006': 'MultiPolygon' + }; + + var bigendian = wkb[0] === '0' && wkb[1] === '0'; + var type = wkb.substring(2, 6); + if(!bigendian) { + // swap '0100' => '0001' + type = type[2] + type[3] + type[0] + type[1]; + } + return typeMap[type]; + + }, + + + // + // guesses from the first row the geometry types involved + // returns an empty array where there is no rows + // return postgist types, like st_GEOTYPE + // + getGeometryTypes: function() { + var row = null; + var i = this.size(); + while (i-- && !(row && row.get('the_geom'))) { + row = this.at(i); + } + if(!row) return []; + var geom = row.get('the_geom') || row.get('the_geom_webmercator'); + var geoType = this.geometryTypeFromWKB(geom) || this.geometryTypeFromWKT(geom); + if(geoType) { + return ['ST_' + geoType[0].toUpperCase() + geoType.substring(1).toLowerCase()]; + } + return []; + }, }); diff --git a/lib/assets/test/spec/cartodb/models/table.spec.js b/lib/assets/test/spec/cartodb/models/table.spec.js index 6800cdb014aa..80a752cdd5ee 100644 --- a/lib/assets/test/spec/cartodb/models/table.spec.js +++ b/lib/assets/test/spec/cartodb/models/table.spec.js @@ -138,30 +138,6 @@ describe("admin table", function() { expect(s.called).toEqual(true); }); - it("should not override geometry_types and schema when in sqlView", function() { - var sqlView = new cdb.admin.SQLViewData(null, { sql: 'select * from a' }); - table.useSQLView(sqlView); - var parsed = table.parse({ - name: 'test_2', - schema: [ ['a', 'number'] ], - geometry_types: ['test'] - }) - expect(parsed.name).toEqual('test_2'); - expect(parsed.schema).toEqual(undefined); - expect(parsed.geometry_types).toEqual(undefined); - - table.useSQLView(null); - parsed = table.parse({ - name: 'test_2', - schema: [ ['a', 'number'] ], - geometry_types: ['test'] - }) - expect(parsed.name).toEqual('test_2'); - expect(parsed.schema).toEqual([ ['a', 'number'] ]); - expect(parsed.geometry_types).toEqual(['test']); - - }); - it("should copy the types of the original schema" ,function() { var sqlView = new cdb.admin.SQLViewData(null, { sql: 'select * from a' }); @@ -708,12 +684,6 @@ describe("admin table", function() { table.set({ name: 'testTable', id: 'testTable', - schema: [ - ['test', 'string'], - ['test2', 'number'], - ['the_geom', 'geometry'], - ['the_geom_webmercator', 'geometry'] - ], }) var the_geom = ["CASE", "WHEN GeometryType(the_geom) = 'POINT' THEN", @@ -723,20 +693,24 @@ describe("admin table", function() { "ELSE", "GeometryType(the_geom)", "END the_geom"].join(' ') - var sql = "select \"test\",\"test2\","+ the_geom +" from (select * from testTable) __wrapped order by cartodb_id asc"; + var sql = "select \"cartodb_id\",\"test\",\"test2\","+ the_geom +" from (select * from testTable) __wrapped order by cartodb_id asc"; + tableData.query_schema = { + 'cartodb_id': 'integer', + 'test': 'string', + 'test2': 'number', + 'the_geom': 'geometry' + } + tableData.sync(); var postData = Backbone.sync.calls.mostRecent().args[2].data; expect(postData.indexOf(encodeURIComponent(sql))).not.toEqual(-1); expect(postData.indexOf('rows_per_page=40')).not.toEqual(-1); expect(postData.indexOf("the_geom_webmercator")).toEqual(-1); - table.set({name: 'testTable', - schema: [ - ['test', 'string'], - ['test2', 'number'], - ['the_geom_webmercator', 'geometry'] - ] - }); + tableData.query_schema = { + 'test': 'string', + 'test2': 'number', + } tableData.sync(); postData = Backbone.sync.calls.mostRecent().args[2].data; @@ -758,6 +732,12 @@ describe("admin table", function() { it("should add params to the request", function() { spyOn(Backbone, 'sync'); + tableData.query_schema = { + 'cartodb_id': 'integer', + 'test': 'string', + 'test2': 'number', + 'the_geom': 'geometry' + } tableData.sync(); var postData = Backbone.sync.calls.mostRecent().args[2].data; expect(postData.indexOf('rows_per_page=40')).not.toEqual(-1); @@ -782,6 +762,14 @@ describe("admin table", function() { } opts.success({ rows: r, modified: false }) }; + tableData._sqlQuery = function(sql, callback) { + callback({ + 'cartodb_id': 'integer', + 'test': 'string', + 'test2': 'number', + 'the_geom': 'geometry' + }) + } tableData.fetch(); expect(tableData.pages).toEqual([0]); expect(tableData.size()).toEqual(40); @@ -801,24 +789,6 @@ describe("admin table", function() { //expect(tableData.pages).toEqual([0, 1, 2, 3]); expect(tableData.size()).toEqual(4*40); - /*rows_to_return = 30; - tableData.loadPageAtBottom(); - expect(tableData.pages).toEqual([1, 2, 3, 4]); - expect(tableData.size()).toEqual(3*40 + rows_to_return); - expect(tableData.lastPage).toEqual(true); - - // load again - tableData.loadPageAtBottom(); - expect(tableData.pages).toEqual([1, 2, 3, 4]); - expect(tableData.size()).toEqual(3*40 + rows_to_return); - expect(tableData.lastPage).toEqual(true); - - rows_to_return = 40; - tableData.loadPageAtTop(); - expect(tableData.pages).toEqual([0, 1, 2, 3]); - expect(tableData.size()).toEqual(4*40); - expect(tableData.lastPage).toEqual(false); - */ }); @@ -1048,12 +1018,6 @@ describe("admin table", function() { expect(sqlView.size()).toEqual(0); }); - it("should set type for special geometry types", function() { - expect(sqlView._schemaFromQueryFields({ the_geom: { type: 'string'}, the_geom_webmercator: { type: 'string' }, test: {type: 'string'} })).toEqual( - { the_geom:'geometry', the_geom_webmercator: 'geometry' , test: 'string' } - ); - }) - it("fech should use POST when the query is longer than 1024 bytes", function() { spyOn(sqlView, '_sqlQuery') var sql = 'select * from table limit ' diff --git a/lib/assets/test/spec/cartodb/table/tableview.spec.js b/lib/assets/test/spec/cartodb/table/tableview.spec.js index 8b36598601d4..aada30a83cb7 100644 --- a/lib/assets/test/spec/cartodb/table/tableview.spec.js +++ b/lib/assets/test/spec/cartodb/table/tableview.spec.js @@ -120,6 +120,7 @@ describe("tableview", function() { geocoder: new cdb.admin.Geocoding(), globalError: globalErrorDummy }); + model.data().query_schema = model._columnType; model.data().reset([ {'id': 1, 'col1': 1, 'col2': 2, 'col3': 3, 'the_geom': null}, {'id': 2, 'col1': 4, 'col2': 5, 'col3': 6, 'the_geom': null} @@ -217,7 +218,7 @@ describe("tableview", function() { model.set('geometry_types', ["st_point"]); tview.render(); - expect(tview.$('td .cell').eq(5).html()).toEqual(" 45.3211,-33.3312"); + expect(tview.$('td .cell').eq(1).html()).toEqual(" 45.3211,-33.3312"); }) @@ -229,7 +230,7 @@ describe("tableview", function() { model.set('geometry_types', ["st_linestring"]); tview.render(); - expect(tview.$('td .cell').eq(5).html()).toEqual("Line"); + expect(tview.$('td .cell').eq(1).html()).toEqual("Line"); }) @@ -239,7 +240,7 @@ describe("tableview", function() { ]); tview.render(); - expect(tview.$('td .cell').eq(5).html()).toEqual("null"); + expect(tview.$('td .cell').eq(1).html()).toEqual("null"); }) it("should be able to add an empty row", function() { From ea8d7031c347ad7791d17871141544276d4e7c30 Mon Sep 17 00:00:00 2001 From: javi Date: Wed, 11 Feb 2015 12:38:57 +0100 Subject: [PATCH 03/16] fixed spec helper --- lib/assets/test/spec/SpecHelper.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/assets/test/spec/SpecHelper.js b/lib/assets/test/spec/SpecHelper.js index e34071981568..00b6ca1f5788 100644 --- a/lib/assets/test/spec/SpecHelper.js +++ b/lib/assets/test/spec/SpecHelper.js @@ -64,6 +64,8 @@ TestUtil.feedTable = function(table, nElements) { element['id'] = i; elements.push(element) } + // set up query_schema so the table schema will be filled with that data + table.data().query_schema = table._columnType; table.data().reset(elements); } From 2eda249d6c1d0da9e69b200f8fa8436d0bd5cc86 Mon Sep 17 00:00:00 2001 From: javi Date: Wed, 11 Feb 2015 12:39:21 +0100 Subject: [PATCH 04/16] fix table data or sql data depending on if sql is active --- lib/assets/javascripts/cartodb/models/cartodb_layer.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/assets/javascripts/cartodb/models/cartodb_layer.js b/lib/assets/javascripts/cartodb/models/cartodb_layer.js index b665a71b1c64..b43575e29fbd 100644 --- a/lib/assets/javascripts/cartodb/models/cartodb_layer.js +++ b/lib/assets/javascripts/cartodb/models/cartodb_layer.js @@ -339,6 +339,8 @@ cdb.admin.CartoDBLayer = cdb.geo.CartoDBLayer.extend({ var sql = this.get('query'); if (sql) { this.applySQLView(sql, { add_to_history: false }); + } else { + this.table.data().fetch(); } }, From 9a604d9d8598c3c0aec348158d4f1e6572ebf61e Mon Sep 17 00:00:00 2001 From: javi Date: Wed, 11 Feb 2015 15:08:42 +0100 Subject: [PATCH 05/16] table data does not depend on table anymore --- .../cartodb/models/cartodb_layer.js | 2 +- .../javascripts/cartodb/models/table.js | 2 - .../javascripts/cartodb/models/tabledata.js | 56 +------------------ .../javascripts/cartodb/table/actions_menu.js | 2 +- 4 files changed, 3 insertions(+), 59 deletions(-) diff --git a/lib/assets/javascripts/cartodb/models/cartodb_layer.js b/lib/assets/javascripts/cartodb/models/cartodb_layer.js index b43575e29fbd..286285f8ac1a 100644 --- a/lib/assets/javascripts/cartodb/models/cartodb_layer.js +++ b/lib/assets/javascripts/cartodb/models/cartodb_layer.js @@ -62,7 +62,7 @@ cdb.admin.CartoDBLayer = cdb.geo.CartoDBLayer.extend({ }, isTableLoaded: function() { - return this.table.get('id') && this.table.get('schema'); + return this.table.get('id') && this.table.get('privacy'); }, getLayerDef: function() { diff --git a/lib/assets/javascripts/cartodb/models/table.js b/lib/assets/javascripts/cartodb/models/table.js index 84e542b33042..2a3997851ea5 100644 --- a/lib/assets/javascripts/cartodb/models/table.js +++ b/lib/assets/javascripts/cartodb/models/table.js @@ -478,7 +478,6 @@ this.bindData(); if(view) { - this._data.unlinkFromSchema(); view.bind('reset', function() { if(!view.modify_rows) { this.set({ @@ -509,7 +508,6 @@ this.dataModel = this.sqlView; view.table = this; } else { - this._data.linkToSchema(); this.dataModel = this._data; // get the original schema self.set({ diff --git a/lib/assets/javascripts/cartodb/models/tabledata.js b/lib/assets/javascripts/cartodb/models/tabledata.js index bc3776fd23b9..0c9f2c5805c3 100644 --- a/lib/assets/javascripts/cartodb/models/tabledata.js +++ b/lib/assets/javascripts/cartodb/models/tabledata.js @@ -11,9 +11,6 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ this.table = options ? options.table: null; this.model.prototype.idAttribute = 'cartodb_id'; this.initOptions(); - if(this.table) { - this.linkToSchema(); - } this.filter = null; this._fetching = false; this.pages = []; @@ -101,44 +98,6 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ }, this); }, - /** - * when the schema changes the data is not refetched - */ - unlinkFromSchema: function() { - this.table.unbind('change', null, this); - }, - - /** - * when the schema changes the data is fetched again - */ - linkToSchema: function() { - var self = this; - function needsFetch(table) { - if (table.no_data_fetch) return false; - // if the change only affects to geometry_types do not - // refresh - var ca = table.changedAttributes() || {}; - var changed = _.without( _.keys(ca), - 'geometry_types', - 'name', - 'updated_at', - 'table_visualization' - ); - - if (changed.length === 0) { - return false; - } - - return true; - } - - this.table.bind('change', function() { - if (needsFetch(self.table) && self.table.get('name') && self.table.has('schema')) { - self.fetch(); - } - }, this); - }, - parse: function(d) { // when the query modifies the data modified flag is true // TODO: change this when SQL API was able to say if a @@ -290,20 +249,7 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ var self = this; // Unlink schema binding avoiding // double sql requests - this.unlinkFromSchema(); - - this.table.fetch({ - complete: function() { - - self.fetch({ - complete: function() { - // Link schema binding again - self.linkToSchema(); - } - }); - - } - }); + this.fetch(); }, isFetchingPage: function() { diff --git a/lib/assets/javascripts/cartodb/table/actions_menu.js b/lib/assets/javascripts/cartodb/table/actions_menu.js index e40e020a8d78..e5b8cac630ea 100644 --- a/lib/assets/javascripts/cartodb/table/actions_menu.js +++ b/lib/assets/javascripts/cartodb/table/actions_menu.js @@ -77,7 +77,7 @@ this.add_related_model(this.map); this._bindSort(); // Bind sort movement - this._addLayers(this.map.layers); + //this._addLayers(this.map.layers); // Setting several view parameters this.model = new cdb.core.Model({ From 3c7babd048e1a3cf3b138c4cb41d955a6f05a970 Mon Sep 17 00:00:00 2001 From: javi Date: Wed, 11 Feb 2015 15:13:22 +0100 Subject: [PATCH 06/16] fixed tests --- lib/assets/test/spec/cartodb/models/table.spec.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/assets/test/spec/cartodb/models/table.spec.js b/lib/assets/test/spec/cartodb/models/table.spec.js index 80a752cdd5ee..f75cf5bc1181 100644 --- a/lib/assets/test/spec/cartodb/models/table.spec.js +++ b/lib/assets/test/spec/cartodb/models/table.spec.js @@ -610,8 +610,6 @@ describe("admin table", function() { spyOn(table._data, 'fetch'); table.set('geometry_types', ['st_polygon', 'st_multilinestring']); expect(table._data.fetch).not.toHaveBeenCalled() - table.set('schema', [['test', 'number']]); - expect(table._data.fetch).toHaveBeenCalled() }); it("should raise when read only is set", function() { @@ -675,7 +673,6 @@ describe("admin table", function() { }); spyOn(tableData, 'fetch'); tableData.refresh(); - expect(tableData.table.fetch).toHaveBeenCalled(); expect(tableData.fetch).toHaveBeenCalled(); }); From f3d686753942fab8a6ee578b4bc724110cd1eb4f Mon Sep 17 00:00:00 2001 From: javi Date: Wed, 11 Feb 2015 15:41:36 +0100 Subject: [PATCH 07/16] removed dead code, added some comments to clarify decisions --- lib/assets/javascripts/cartodb/models/tabledata.js | 14 -------------- .../javascripts/cartodb/table/actions_menu.js | 3 +++ 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/lib/assets/javascripts/cartodb/models/tabledata.js b/lib/assets/javascripts/cartodb/models/tabledata.js index 0c9f2c5805c3..86bf05b56653 100644 --- a/lib/assets/javascripts/cartodb/models/tabledata.js +++ b/lib/assets/javascripts/cartodb/models/tabledata.js @@ -221,17 +221,6 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ return Backbone.sync.call(this, method, this, options); }, - /*sync: function(method, model, options) { - if(!options) { options = {}; }; - var query = this._createUrlOptions(); - query += "&q=" + encodeURIComponent(this.wrappedSQL(this.table.columnTypes())); - if (query.length > 2048) { - options.type = 'POST'; - } - options.data = query; - return Backbone.sync.call(this, method, this, options); - },*/ - sqlApiUrl: function() { var protocol = cdb.config.get("sql_api_port") == 443 ? 'https': 'http'; var u = protocol + "://" + cdb.config.get('user_name') + "." + cdb.config.get('sql_api_domain'); @@ -246,9 +235,6 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ // Refresh all table data refresh: function() { - var self = this; - // Unlink schema binding avoiding - // double sql requests this.fetch(); }, diff --git a/lib/assets/javascripts/cartodb/table/actions_menu.js b/lib/assets/javascripts/cartodb/table/actions_menu.js index e5b8cac630ea..77279fdbee74 100644 --- a/lib/assets/javascripts/cartodb/table/actions_menu.js +++ b/lib/assets/javascripts/cartodb/table/actions_menu.js @@ -77,6 +77,9 @@ this.add_related_model(this.map); this._bindSort(); // Bind sort movement + // the following call is not needed, 'reset' on map.layers will be raised + // after this is loaded. If the map data was not loaded using AJAX this would + // need to be enabled //this._addLayers(this.map.layers); // Setting several view parameters From 98f01a289e82dcdf638cb6f1f5c3262ac6ae234b Mon Sep 17 00:00:00 2001 From: javi Date: Wed, 11 Feb 2015 16:49:18 +0100 Subject: [PATCH 08/16] fixed column operations --- lib/assets/javascripts/cartodb/models/table.js | 8 ++++---- lib/assets/test/spec/cartodb/models/table.spec.js | 6 ++++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/assets/javascripts/cartodb/models/table.js b/lib/assets/javascripts/cartodb/models/table.js index 2a3997851ea5..6f0fcb3c674b 100644 --- a/lib/assets/javascripts/cartodb/models/table.js +++ b/lib/assets/javascripts/cartodb/models/table.js @@ -307,7 +307,7 @@ success: function() { self.notice(self._TEXTS.columnAdded, 'info'); self.trigger('columnAdd', columnName); - self.fetch(); + self.data().fetch(); callback && callback(); }, error: function(e, resp) { @@ -326,7 +326,7 @@ success: function() { self.trigger('columnDelete', columnName); self.notice(self._TEXTS.columnDeleted, 'info'); - self.fetch(); + self.data().fetch(); }, error: function(e, resp) { self.error('error deleting column', resp); @@ -350,7 +350,7 @@ success: function() { self.notice('Column has been renamed', 'info'); self.trigger('columnRename', newName, oldName); - self.fetch(); + self.data().fetch(); }, error: function(e, resp) { cdb.log.error("can't rename column"); @@ -410,7 +410,7 @@ success: function() { self.notice('Column type has been changed', 'info'); self.trigger('typeChanged', newType); // to make it testable - self.fetch(); + self.data().fetch(); }, error: function(e, resp) { self.trigger('typeChangeFailed', newType, e); // to make it testable diff --git a/lib/assets/test/spec/cartodb/models/table.spec.js b/lib/assets/test/spec/cartodb/models/table.spec.js index f75cf5bc1181..92ed5b11dbc6 100644 --- a/lib/assets/test/spec/cartodb/models/table.spec.js +++ b/lib/assets/test/spec/cartodb/models/table.spec.js @@ -523,9 +523,11 @@ describe("admin table", function() { table.bind('columnDelete', function() { succeded = true; }); + spyOn(table._data, 'fetch'); table.deleteColumn('test'); server.respond(); expect(succeded).toBeTruthy(); + expect(table._data.fetch).toHaveBeenCalled(); }); it("should not remove a column if it's invalid", function() { @@ -552,9 +554,11 @@ describe("admin table", function() { if(oldName == 'test' && newName == 'irrelevant') succeded = true; }); + spyOn(table._data, 'fetch'); table.renameColumn('test', 'irrelevant'); server.respond(); expect(succeded).toBeTruthy(); + expect(table._data.fetch).toHaveBeenCalled(); }) it("should be able to say if a type change is destructive", function() { @@ -573,9 +577,11 @@ describe("admin table", function() { if(newType = 'string') succeded = true; }); + spyOn(table._data, 'fetch'); table.changeColumnType('test2', 'string'); server.respond(); expect(succeded).toBeTruthy(); + expect(table._data.fetch).toHaveBeenCalled(); }) it("should trigger an error when there's any error saving the type change", function() { From c3081d85d4069861c58a00f4f0596e79f7b19ee8 Mon Sep 17 00:00:00 2001 From: javi Date: Wed, 11 Feb 2015 18:26:03 +0100 Subject: [PATCH 09/16] don't use the_geom_webmercator if the_geom is present --- lib/assets/javascripts/cartodb/models/tabledata.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/assets/javascripts/cartodb/models/tabledata.js b/lib/assets/javascripts/cartodb/models/tabledata.js index 86bf05b56653..ef7e44f6ae73 100644 --- a/lib/assets/javascripts/cartodb/models/tabledata.js +++ b/lib/assets/javascripts/cartodb/models/tabledata.js @@ -740,6 +740,9 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ this._sqlQuery(_.template('select * from (<%= sql %>) __wrapped limit 0')({ sql: sql }), function(data) { // get schema self.query_schema = self._schemaFromQueryFields(data.fields); + if ('the_geom' in self.query_schema) { + delete self.query_schema['the_geom_webmercator']; + } cdb.ui.common.TableData.prototype.fetch.call(self, opts); }, function (err) { self.trigger('error', self, err); From cb52006c47ceb2e2bde955a7ffb6ed8de22e43ad Mon Sep 17 00:00:00 2001 From: javi Date: Wed, 11 Feb 2015 18:26:39 +0100 Subject: [PATCH 10/16] fetch the data not the table after removing the sql view --- lib/assets/javascripts/cartodb/models/table.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/assets/javascripts/cartodb/models/table.js b/lib/assets/javascripts/cartodb/models/table.js index 6f0fcb3c674b..c0a7a174e69e 100644 --- a/lib/assets/javascripts/cartodb/models/table.js +++ b/lib/assets/javascripts/cartodb/models/table.js @@ -513,13 +513,7 @@ self.set({ 'schema': self.get('original_schema') });///*, { silent: true }); - this.fetch({ - success: function() { - if(options.force_data_fetch) { - self.data().fetch(); - } - } - }); + self.data().fetch(); } this.trigger('change:dataSource', this.dataModel, this); }, From da7c151c761d124abd41f7bdeb8b6d2e1041673d Mon Sep 17 00:00:00 2001 From: javi Date: Wed, 11 Feb 2015 19:32:34 +0100 Subject: [PATCH 11/16] fixed tests --- lib/assets/test/spec/cartodb/models/table.spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/assets/test/spec/cartodb/models/table.spec.js b/lib/assets/test/spec/cartodb/models/table.spec.js index 92ed5b11dbc6..11f0d0e667d0 100644 --- a/lib/assets/test/spec/cartodb/models/table.spec.js +++ b/lib/assets/test/spec/cartodb/models/table.spec.js @@ -366,10 +366,10 @@ describe("admin table", function() { sqlView.reset([ { a: 1, b:2 } ]); - spyOn(table, 'fetch'); + spyOn(table._data, 'fetch'); expect(sqlView.table).toEqual(table); table.useSQLView(null); - expect(table.fetch).toHaveBeenCalled(); + expect(table._data.fetch).toHaveBeenCalled(); expect(sqlView.table).toEqual(null); }); From f2ffdec51107906bea2f8066cde68fa032f81303 Mon Sep 17 00:00:00 2001 From: javi Date: Thu, 12 Feb 2015 14:30:12 +0100 Subject: [PATCH 12/16] do not fetch the_geom_webmercator when using table view --- lib/assets/javascripts/cartodb/models/tabledata.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/assets/javascripts/cartodb/models/tabledata.js b/lib/assets/javascripts/cartodb/models/tabledata.js index ef7e44f6ae73..50bb97425495 100644 --- a/lib/assets/javascripts/cartodb/models/tabledata.js +++ b/lib/assets/javascripts/cartodb/models/tabledata.js @@ -740,8 +740,10 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ this._sqlQuery(_.template('select * from (<%= sql %>) __wrapped limit 0')({ sql: sql }), function(data) { // get schema self.query_schema = self._schemaFromQueryFields(data.fields); - if ('the_geom' in self.query_schema) { - delete self.query_schema['the_geom_webmercator']; + if (!self.table.isInSQLView()) { + if ('the_geom' in self.query_schema) { + delete self.query_schema['the_geom_webmercator']; + } } cdb.ui.common.TableData.prototype.fetch.call(self, opts); }, function (err) { From e01299ae8954b34b69103ff44d8a5ece2522df32 Mon Sep 17 00:00:00 2001 From: javi Date: Thu, 12 Feb 2015 14:30:44 +0100 Subject: [PATCH 13/16] fixed an unrelated bug. I don't want to create a new branch for this --- lib/assets/javascripts/cartodb/models/carto.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/assets/javascripts/cartodb/models/carto.js b/lib/assets/javascripts/cartodb/models/carto.js index 56a24414521b..3cee2f52a8ce 100644 --- a/lib/assets/javascripts/cartodb/models/carto.js +++ b/lib/assets/javascripts/cartodb/models/carto.js @@ -423,7 +423,7 @@ function normalizeQuartiles(quartiles) { var normalized = []; for(var i = 0; i < quartiles.length; ++i) { var q = quartiles[i]; - if(q > Math.abs(maxNumber)) { + if(q > Math.abs(maxNumber) && String(q).indexOf('.') === -1) { q = q + ".01"; } normalized.push(q); From 7becb4112c3d194208bec41ab3511b43f0f08d79 Mon Sep 17 00:00:00 2001 From: javi Date: Thu, 12 Feb 2015 15:11:33 +0100 Subject: [PATCH 14/16] fixes add layer, #2208 --- lib/assets/javascripts/cartodb/models/table.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/assets/javascripts/cartodb/models/table.js b/lib/assets/javascripts/cartodb/models/table.js index c0a7a174e69e..e85d582580c2 100644 --- a/lib/assets/javascripts/cartodb/models/table.js +++ b/lib/assets/javascripts/cartodb/models/table.js @@ -130,7 +130,9 @@ if(resp.name) { resp.id = resp.name; } - delete resp.geometry_types; + // do not drop geometry_types. It's calculated from sql query + // but in some places the cached value from backend is enough + //delete resp.geometry_types; delete resp.schema; return resp; }, From d5c81c3e45c9ed4b06cb0f04c502be2e2276e9db Mon Sep 17 00:00:00 2001 From: javi Date: Fri, 13 Feb 2015 10:45:30 +0100 Subject: [PATCH 15/16] fixed #2209 --- lib/assets/javascripts/cartodb/models/tabledata.js | 6 +++--- lib/assets/test/spec/cartodb/models/table.spec.js | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/assets/javascripts/cartodb/models/tabledata.js b/lib/assets/javascripts/cartodb/models/tabledata.js index 50bb97425495..71e1bfdad9c1 100644 --- a/lib/assets/javascripts/cartodb/models/tabledata.js +++ b/lib/assets/javascripts/cartodb/models/tabledata.js @@ -323,10 +323,10 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ newRow: function(attrs) { r = new cdb.admin.Row(attrs); r.table = this.table; - r.bind('saved', function() { + r.bind('saved', function _saved() { if(r.table.data().length == 0) { - r.table.fetch(); - r.unbind('saved', null, r.table); + r.table.data().fetch(); + r.unbind('saved', _saved, r.table); } }, r.table); return r; diff --git a/lib/assets/test/spec/cartodb/models/table.spec.js b/lib/assets/test/spec/cartodb/models/table.spec.js index 11f0d0e667d0..f967b44de613 100644 --- a/lib/assets/test/spec/cartodb/models/table.spec.js +++ b/lib/assets/test/spec/cartodb/models/table.spec.js @@ -392,9 +392,9 @@ describe("admin table", function() { it("newRow should fetch the table if table is emtpy", function() { table.data().reset([]); var r = table.data().newRow(); - spyOn(table, 'fetch') + spyOn(table.data(), 'fetch') r.trigger('saved'); - expect(table.fetch).toHaveBeenCalled(); + expect(table.data().fetch).toHaveBeenCalled(); }); it("should be able to link to infowindow", function() { From 81db80188e36a21173061c45dc57ad883d3d3005 Mon Sep 17 00:00:00 2001 From: javi Date: Mon, 16 Feb 2015 09:32:39 +0100 Subject: [PATCH 16/16] updated news and version --- NEWS.md | 1 + config/frontend.yml | 2 +- package.json | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index 71461cca9bfe..1adf41a7a7c2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -6,6 +6,7 @@ * [content guessing] Prioritize ip over country guessing [#2089](https://github.com/CartoDB/cartodb/issues/2089) * Added new import error code (6667) for detecting and reporting statement timeouts. * Fixes creation of a visualization from a table [#2145](https://github.com/CartoDB/cartodb/issues/2145) +* Changes the way geometry types are loaded client side (performance), see PR [#2189](https://github.com/CartoDB/cartodb/pull/218) 3.8.0 (2015-01-30) ------------------ diff --git a/config/frontend.yml b/config/frontend.yml index 0d3f176056b8..4fd1dae3e168 100644 --- a/config/frontend.yml +++ b/config/frontend.yml @@ -1,2 +1,2 @@ #This file defines the version of the frontend code that is going to be loaded. -3.4.7 +3.4.8 diff --git a/package.json b/package.json index 3be6f521ccc6..a9f0b408adc0 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "cartodb-ui", - "version": "3.4.7", + "version": "3.4.8", "description": "CartoDB UI frontend", "repository": { "type": "git",