From 0d84a704df53df8d85aa39ed2dd7f89199d2cd5d Mon Sep 17 00:00:00 2001
From: Sandro Santilli <strk@keybit.net>
Date: Tue, 1 Oct 2013 17:19:44 +0200
Subject: [PATCH] Fix missing .prj file in shapefile format

Finds srid, when needed, with an additional query.
Closes #110.
Includes testcases.
---
 NEWS.md                             |  1 +
 app/models/formats/ogr.js           | 79 ++++++++++++++++++++---------
 app/models/formats/shp.js           |  1 +
 app/models/psql.js                  | 29 ++++++++++-
 test/acceptance/export/shapefile.js | 25 +++++----
 test/test.sql                       |  4 +-
 6 files changed, 102 insertions(+), 37 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 4bed88fd..c8cd0412 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,5 +1,6 @@
 1.6.0
 -----
+* Fix missing .prj in shapefile export (#110)
 * Improve recognition of non-standard field types names by db lookup (#112)
 
 1.5.4 - 2013-10-01
diff --git a/app/models/formats/ogr.js b/app/models/formats/ogr.js
index 31c0803e..3d26f9a6 100644
--- a/app/models/formats/ogr.js
+++ b/app/models/formats/ogr.js
@@ -65,7 +65,11 @@ ogr.prototype.toOGR = function(dbname, user_id, gcol, sql, skipfields, out_forma
   var dbuser = userid_to_dbuser(user_id);
   var dbpass = ''; // turn into a parameter..
 
+  var that = this;
+
   var columns = [];
+  var geocol;
+  var pg;
 
   // Drop ending semicolon (ogr doens't like it)
   sql = sql.replace(/;\s*$/, ''); 
@@ -73,32 +77,60 @@ ogr.prototype.toOGR = function(dbname, user_id, gcol, sql, skipfields, out_forma
   Step (
 
     function fetchColumns() {
-      var colsql = 'SELECT * FROM (' + sql + ') as _cartodbsqlapi LIMIT 1';
-      var pg = new PSQL(user_id, dbname, 1, 0);
+      var colsql = 'SELECT * FROM (' + sql + ') as _cartodbsqlapi LIMIT 0';
+      pg = new PSQL(user_id, dbname, 1, 0);
       pg.query(colsql, this);
     },
-    function spawnDumper(err, result) {
+    function findSRS(err, result) {
       if (err) throw err;
 
       //if ( ! result.rows.length ) throw new Error("Query returns no rows");
 
-      // Skip system columns
-      if ( result.rows.length ) {
-        for (var k in result.rows[0]) {
-          if ( skipfields.indexOf(k) != -1 ) continue;
-          if ( out_format != 'CSV' && k == "the_geom_webmercator" ) continue; // TODO: drop ?
-          if ( out_format == 'CSV' ) columns.push('"' + k + '"::text');
-          else columns.push('"' + k + '"');
+      var needSRS = that._needSRS;
+
+      // Skip system columns, find geom column
+      for (var i=0; i<result.fields.length; ++i) {
+        var field = result.fields[i];
+        var k = field.name;
+        if ( skipfields.indexOf(k) != -1 ) continue;
+        if ( out_format != 'CSV' && k == "the_geom_webmercator" ) continue; // TODO: drop ?
+        if ( out_format == 'CSV' ) columns.push(pg.quoteIdentifier(k)+'::text');
+        else columns.push(pg.quoteIdentifier(k));
+
+        if ( needSRS ) {
+          if ( ! geocol && pg.typeName(field.dataTypeID) == 'geometry' ) {
+            geocol = k
+          }
         }
-      } else columns.push('*');
+      }
       //console.log(columns.join(','));
 
+      if ( ! needSRS || ! geocol ) return null;
+
       var next = this;
 
-      sql = 'SELECT ' + columns.join(',')
+      var sridsql = 'SELECT ST_Srid(' + pg.quoteIdentifier(geocol) +
+                   ') as srid FROM (' + sql + ') as _cartodbsqlapi WHERE ' +
+                   pg.quoteIdentifier(geocol) + ' is not null limit 1';
+
+      pg.query(sridsql, function(err, result) {
+        if ( err ) { next(err); return; }
+        if ( result.rows.length ) {
+          var srid = result.rows[0].srid;
+          next(null, srid);
+        }
+      });
+
+    },
+    function spawnDumper(err, srid) {
+      if (err) throw err;
+
+      var next = this;
+
+      var ogrsql = 'SELECT ' + columns.join(',')
           + ' FROM (' + sql + ') as _cartodbsqlapi';
 
-      var child = spawn(ogr2ogr, [
+      var ogrargs = [
         '-f', out_format,
         '-lco', 'ENCODING=UTF-8',
         '-lco', 'LINEFORMAT=CRLF',
@@ -111,20 +143,17 @@ ogr.prototype.toOGR = function(dbname, user_id, gcol, sql, skipfields, out_forma
                           // in turn breaks knowing SRID with gdal-0.10.1:
                           // http://github.com/CartoDB/CartoDB-SQL-API/issues/110
          + "",
-        '-sql', sql
-      ]);
+        '-sql', ogrsql
+      ];
+
+      if ( srid ) {
+        ogrargs.push('-a_srs', 'EPSG:'+srid);
+      }
+
+      var child = spawn(ogr2ogr, ogrargs);
 
 /*
-console.log(['ogr2ogr',
-        '-f', '"'+out_format+'"',
-        out_filename,
-        "'PG:host=" + dbhost
-         + " user=" + dbuser
-         + " dbname=" + dbname
-         + " password=" + dbpass
-         + " tables=fake" // trick to skip query to geometry_columns
-         + "'",
-        "-sql '", sql, "'"].join(' '));
+console.log('ogr2ogr' + ogrargs);
 */
 
       var stdout = '';
diff --git a/app/models/formats/shp.js b/app/models/formats/shp.js
index 8e81621d..dac69f8b 100644
--- a/app/models/formats/shp.js
+++ b/app/models/formats/shp.js
@@ -14,6 +14,7 @@ var p = shp.prototype;
 
 p._contentType = "application/zip; charset=utf-8";
 p._fileExtension = "zip";
+p._needSRS = true;
 
 p.generate = function(options, callback) {
   var o = options;
diff --git a/app/models/psql.js b/app/models/psql.js
index 1dafc3c9..4a1ddfee 100644
--- a/app/models/psql.js
+++ b/app/models/psql.js
@@ -174,7 +174,34 @@ var PSQL = function(user_id, db) {
                 callback(err, query)
             }
         );
-    },
+    };
+
+    // Ported from PostgreSQL 9.2.4 source code in src/interfaces/libpq/fe-exec.c
+    me.quoteIdentifier = function(str) {
+
+      // TODO: delegate to node-postgresl (needs node-pg 2.2.0+!)
+
+      var escaped = '"';
+
+      for(var i = 0; i < str.length; i++) {
+        var c = str[i];
+        if(c === '"') {
+          escaped += c + c;
+        } else {
+          escaped += c;
+        }
+      }
+
+      escaped += '"';
+
+      return escaped;
+    };
+
+/*
+    me.escapeLiteral = function(s) {
+      return this.client.escapeLiteral(s);
+    };
+*/
 
     me.query = function(sql, callback){
         var that = this;
diff --git a/test/acceptance/export/shapefile.js b/test/acceptance/export/shapefile.js
index 60bd9eae..4fa41446 100644
--- a/test/acceptance/export/shapefile.js
+++ b/test/acceptance/export/shapefile.js
@@ -36,8 +36,9 @@ test('SHP format, unauthenticated', function(done){
         assert.ok(_.contains(zf.names, 'cartodb-query.shp'), 'SHP zipfile does not contain .shp: ' + zf.names);
         assert.ok(_.contains(zf.names, 'cartodb-query.shx'), 'SHP zipfile does not contain .shx: ' + zf.names);
         assert.ok(_.contains(zf.names, 'cartodb-query.dbf'), 'SHP zipfile does not contain .dbf: ' + zf.names);
-        // missing SRID, so no PRJ (TODO: add ?)
-        //assert.ok(_.contains(zf.names, 'cartodb-query.prj'), 'SHP zipfile does not contain .prj: ' + zf.names);
+        // This will fail with < GDAL-0.10.2
+        // https://github.com/CartoDB/CartoDB-SQL-API/issues/110
+        assert.ok(_.contains(zf.names, 'cartodb-query.prj'), 'SHP zipfile does not contain .prj: ' + zf.names);
         // TODO: check DBF contents
         fs.unlinkSync(tmpfile);
         done();
@@ -96,8 +97,9 @@ test('SHP format, unauthenticated, with custom filename', function(done){
         assert.ok(_.contains(zf.names, 'myshape.shp'), 'SHP zipfile does not contain .shp: ' + zf.names);
         assert.ok(_.contains(zf.names, 'myshape.shx'), 'SHP zipfile does not contain .shx: ' + zf.names);
         assert.ok(_.contains(zf.names, 'myshape.dbf'), 'SHP zipfile does not contain .dbf: ' + zf.names);
-        // missing SRID, so no PRJ (TODO: add ?)
-        //assert.ok(_.contains(zf.names, 'myshape.prj'), 'SHP zipfile does not contain .prj: ' + zf.names);
+        // This will fail with < GDAL-0.10.2
+        // https://github.com/CartoDB/CartoDB-SQL-API/issues/110
+        assert.ok(_.contains(zf.names, 'myshape.prj'), 'SHP zipfile does not contain .prj: ' + zf.names);
         fs.unlinkSync(tmpfile);
         done();
     });
@@ -122,8 +124,9 @@ test('SHP format, unauthenticated, with custom, dangerous filename', function(do
         assert.ok(_.contains(zf.names, fname + '.shp'), 'SHP zipfile does not contain .shp: ' + zf.names);
         assert.ok(_.contains(zf.names, fname + '.shx'), 'SHP zipfile does not contain .shx: ' + zf.names);
         assert.ok(_.contains(zf.names, fname + '.dbf'), 'SHP zipfile does not contain .dbf: ' + zf.names);
-        // missing SRID, so no PRJ (TODO: add ?)
-        //assert.ok(_.contains(zf.names, fname+ '.prj'), 'SHP zipfile does not contain .prj: ' + zf.names);
+        // This will fail with < GDAL-0.10.2
+        // https://github.com/CartoDB/CartoDB-SQL-API/issues/110
+        assert.ok(_.contains(zf.names, fname+ '.prj'), 'SHP zipfile does not contain .prj: ' + zf.names);
         fs.unlinkSync(tmpfile);
         done();
     });
@@ -146,8 +149,9 @@ test('SHP format, authenticated', function(done){
         assert.ok(_.contains(zf.names, 'cartodb-query.shp'), 'SHP zipfile does not contain .shp: ' + zf.names);
         assert.ok(_.contains(zf.names, 'cartodb-query.shx'), 'SHP zipfile does not contain .shx: ' + zf.names);
         assert.ok(_.contains(zf.names, 'cartodb-query.dbf'), 'SHP zipfile does not contain .dbf: ' + zf.names);
-        // missing SRID, so no PRJ (TODO: add ?)
-        //assert.ok(_.contains(zf.names, 'cartodb-query.prj'), 'SHP zipfile does not contain .prj: ' + zf.names);
+        // This will fail with < GDAL-0.10.2
+        // https://github.com/CartoDB/CartoDB-SQL-API/issues/110
+        assert.ok(_.contains(zf.names, 'cartodb-query.prj'), 'SHP zipfile does not contain .prj: ' + zf.names);
         // TODO: check contents of the DBF
         fs.unlinkSync(tmpfile);
         done();
@@ -276,8 +280,9 @@ test('SHP format, concurrently', function(done){
           assert.ok(_.contains(zf.names, 'cartodb-query.shp'), 'SHP zipfile does not contain .shp: ' + zf.names);
           assert.ok(_.contains(zf.names, 'cartodb-query.shx'), 'SHP zipfile does not contain .shx: ' + zf.names);
           assert.ok(_.contains(zf.names, 'cartodb-query.dbf'), 'SHP zipfile does not contain .dbf: ' + zf.names);
-          // missing SRID, so no PRJ (TODO: add ?)
-          //assert.ok(_.contains(zf.names, 'cartodb-query.prj'), 'SHP zipfile does not contain .prj: ' + zf.names);
+          // This will fail with < GDAL-0.10.2
+          // https://github.com/CartoDB/CartoDB-SQL-API/issues/110
+          assert.ok(_.contains(zf.names, 'cartodb-query.prj'), 'SHP zipfile does not contain .prj: ' + zf.names);
           // TODO: check DBF contents
           fs.unlinkSync(tmpfile);
           if ( ! --waiting ) done();
diff --git a/test/test.sql b/test/test.sql
index e929cf2c..d3a82470 100644
--- a/test/test.sql
+++ b/test/test.sql
@@ -113,4 +113,6 @@ CREATE USER test_cartodb_user_1 WITH PASSWORD '';
 GRANT ALL ON TABLE untitle_table_4 TO test_cartodb_user_1;
 GRANT SELECT ON TABLE untitle_table_4 TO publicuser;
 GRANT ALL ON TABLE private_table TO test_cartodb_user_1;
-GRANT ALL ON SEQUENCE test_table_cartodb_id_seq_p TO test_cartodb_user_1
+GRANT ALL ON SEQUENCE test_table_cartodb_id_seq_p TO test_cartodb_user_1;
+
+GRANT ALL ON TABLE spatial_ref_sys TO test_cartodb_user_1, publicuser;