Sanitize column names

app/controllers/api/json/tables_controller.rb

@@ -143,13 +143,15 @@ class Api::Json::TablesController < ApplicationController
           unless params[:column].blank? || params[:column].empty?
             begin
               if params[:what] == 'add'
-                @table.add_column!(params[:column])
+                resp = @table.add_column!(params[:column])
+                render :json => resp.to_json, :status => 200 and return
               elsif params[:what] == 'drop'
                 @table.drop_column!(params[:column])
+                render :json => ''.to_json, :status => 200 and return
               else
-                @table.modify_column!(params[:column])
+                resp = @table.modify_column!(params[:column])
+                render :json => resp.to_json, :status => 200 and return
               end
-              render :json => ''.to_json, :status => 200
             rescue => e
               render :json => { :errors => [e.message.split("\n").first] }.to_json, :status => 400 and return
             end

app/models/table.rb

@@ -151,8 +151,9 @@ class Table < Sequel::Model(:user_tables)
 
   def add_column!(options)
     owner.in_database do |user_database|
-      user_database.add_column name.to_sym, options[:name].to_s, options[:type]
+      user_database.add_column name.to_sym, options[:name].to_s.sanitize, options[:type]
     end
+    return {:name => options[:name].to_s.sanitize, :type => options[:type]}
   end
 
   def drop_column!(options)
@@ -162,27 +163,32 @@ class Table < Sequel::Model(:user_tables)
   end
 
   def modify_column!(options)
+    new_name = options[:name]
+    new_type = options[:type]
     owner.in_database do |user_database|
       if options[:old_name] && options[:new_name]
-        user_database.rename_column name.to_sym, options[:old_name].to_sym, options[:new_name].to_sym
+        user_database.rename_column name.to_sym, options[:old_name].to_sym, options[:new_name].sanitize.to_sym
+        new_name = options[:new_name].sanitize
       end
       if options[:type]
+        column_name = (options[:new_name] || options[:name]).sanitize
         begin
-          user_database.set_column_type name.to_sym, (options[:new_name] || options[:name]).to_sym, options[:type]
+          user_database.set_column_type name.to_sym, column_name.to_sym, options[:type]
         rescue => e
           message = e.message.split("\n").first
           if message =~ /cannot be cast to type/
             user_database.transaction do
               random_name = "new_column_#{rand(10)*Time.now.to_i}"
               user_database.add_column name.to_sym, random_name, options[:type]
-              user_database["UPDATE #{name} SET #{random_name}=#{(options[:new_name] || options[:name])}::#{options[:type]}"]
-              user_database.drop_column name.to_sym, (options[:new_name] || options[:name]).to_sym
-              user_database.rename_column name.to_sym, random_name, (options[:new_name] || options[:name]).to_sym
+              user_database["UPDATE #{name} SET #{random_name}=#{column_name}::#{options[:type]}"]
+              user_database.drop_column name.to_sym, column_name.to_sym
+              user_database.rename_column name.to_sym, random_name, column_name.to_sym
             end
           end
         end
       end
     end
+    return {:name => new_name, :type => new_type}
   end
 
   def to_json(options = {})

spec/acceptance/api/tables_spec.rb

@@ -153,17 +153,21 @@ feature "Tables JSON API" do
                                                               }
                                                            }
     response.status.should == 200
+    json_response = JSON(response.body)
+    json_response.should == {"name" => "postal_code", "type" => 'integer'}
     table.reload
-    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:"postal code", "integer"]]
+    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:postal_code, "integer"]]
 
     put_json "/api/json/tables/#{table.id}/update_schema", {
                                                               :what => "modify", :column => {
-                                                                  :type => "text", :name => "postal code"
+                                                                  :type => "text", :name => "postal_code"
                                                               }
                                                            }
     response.status.should == 200
+    json_response = JSON(response.body)
+    json_response.should == {"name" => "postal_code", "type" => 'text'}
     table.reload
-    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:"postal code", "text"]]
+    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:postal_code, "text"]]
 
     put_json "/api/json/tables/#{table.id}/update_schema", {
                                                               :what => "add", :column => {
@@ -176,7 +180,7 @@ feature "Tables JSON API" do
 
     put_json "/api/json/tables/#{table.id}/update_schema", {
                                                               :what => "drop", :column => {
-                                                                :name => "postal code"
+                                                                :name => "postal_code"
                                                               }
                                                            }
     response.status.should == 200
@@ -185,17 +189,17 @@ feature "Tables JSON API" do
 
     put_json "/api/json/tables/#{table.id}/update_schema", {
                                                               :what => "drop", :column => {
-                                                                :name => "postal code"
+                                                                :name => "postal_code"
                                                               }
                                                            }
     response.status.should == 400
     table.reload
     json_response = JSON(response.body)
-    json_response['errors'].should == ["PGError: ERROR:  column \"postal code\" of relation \"#{table.name}\" does not exist"]
+    json_response['errors'].should == ["PGError: ERROR:  column \"postal_code\" of relation \"#{table.name}\" does not exist"]
 
     put_json "/api/json/tables/#{table.id}/update_schema", {
                                                               :what => "wadus", :column => {
-                                                                :name => "postal code"
+                                                                :name => "postal_code"
                                                               }
                                                            }
     response.status.should == 400
@@ -218,7 +222,11 @@ feature "Tables JSON API" do
 
     authenticate_api user
 
-    post_json "/api/json/tables/#{table.id}/rows", { :name => "Name 123", :description => "The description", :location => Point.from_x_y(1,1).as_ewkt }
+    post_json "/api/json/tables/#{table.id}/rows", {
+        :name => "Name 123",
+        :description => "The description",
+        :location => Point.from_x_y(1,1).as_ewkt
+    }
     response.status.should == 200
     table.reload
     table.rows_counted.should == 1

spec/models/table_spec.rb

@@ -153,31 +153,35 @@ describe Table do
     }.should raise_error
     table.reload
 
-    table.add_column!(:name => "my new column", :type => "integer")
+    resp = table.add_column!(:name => "my new column", :type => "integer")
+    resp.should == {:name => 'my_new_column', :type => 'integer'}
     table.reload
-    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:"my new column", "integer"]]
+    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:my_new_column, "integer"]]
 
-    table.modify_column!(:old_name => "my new column", :new_name => "my new column new name", :type => "text")
+    resp = table.modify_column!(:old_name => "my_new_column", :new_name => "my new column new name", :type => "text")
+    resp.should == {:name => 'my_new_column_new_name', :type => 'text'}
     table.reload
-    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:"my new column new name", "text"]]
+    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:my_new_column_new_name, "text"]]
 
-    table.modify_column!(:old_name => "my new column new name", :new_name => "my new column")
+    resp = table.modify_column!(:old_name => "my_new_column_new_name", :new_name => "my new column")
+    resp.should == {:name => 'my_new_column', :type => nil}
     table.reload
-    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:"my new column", "text"]]
+    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:my_new_column, "text"]]
 
-    table.modify_column!(:name => "my new column", :type => "text")
+    resp = table.modify_column!(:name => "my_new_column", :type => "text")
+    resp.should == {:name => 'my_new_column', :type => 'text'}
     table.reload
-    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:"my new column", "text"]]
+    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:my_new_column, "text"]]
 
     table.drop_column!(:name => "location")
     table.reload
-    table.schema.should == [[:id, "integer"], [:name, "text"], [:description, "text"], [:"my new column", "text"]]
+    table.schema.should == [[:id, "integer"], [:name, "text"], [:description, "text"], [:my_new_column, "text"]]
 
     lambda {
       table.drop_column!(:name => "location")
     }.should raise_error
     table.reload
-    table.schema.should == [[:id, "integer"], [:name, "text"], [:description, "text"], [:"my new column", "text"]]
+    table.schema.should == [[:id, "integer"], [:name, "text"], [:description, "text"], [:my_new_column, "text"]]
   end
 
   it "should be able to modify it's schema with castings that the DB engine doesn't support" do
@@ -186,11 +190,11 @@ describe Table do
 
     table.add_column!(:name => "my new column", :type => "text")
     table.reload
-    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:"my new column", "text"]]
+    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:my_new_column, "text"]]
 
-    table.modify_column!(:old_name => "my new column", :new_name => "my new column new name", :type => "integer", :force_value => "NULL")
+    table.modify_column!(:old_name => "my_new_column", :new_name => "my new column new name", :type => "integer", :force_value => "NULL")
     table.reload
-    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:"my new column new name", "integer"]]
+    table.schema.should == [[:id, "integer"], [:name, "text"], [:location, "geometry"], [:description, "text"], [:my_new_column_new_name, "integer"]]
   end
 
   it "should be able to insert a new row" do