From 99edf8d72f973f7a29a0baef8d695ad10ca6c063 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Fri, 6 Feb 2015 14:11:46 +0100 Subject: [PATCH 1/4] Notifying instead of failing fixes #2111 --- app/models/table.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/table.rb b/app/models/table.rb index 4a39ac565769..6da11ab91ac2 100644 --- a/app/models/table.rb +++ b/app/models/table.rb @@ -1425,10 +1425,10 @@ def update_name_changes # update metadata records $tables_metadata.rename(old_key, new_key) rescue StandardError => exception - exception_to_raise = CartoDB::BaseCartoDBError.new( - "Table update_name_changes(): '#{@name_changed_from}','#{key}' renaming metadata", exception) - CartoDB::notify_exception(exception_to_raise, user: owner) - errored = true + # we don't want to fail because of not existing keys + exception_to_notify = CartoDB::BaseCartoDBError.new( + "Continuing after exception: Table update_name_changes(): '#{@name_changed_from}','#{key}' renaming metadata", exception) + CartoDB::notify_exception(exception_to_notify, user: owner) end unless register_table_only From 5fd8328cf2a1d87e1779a9bfc60a758edfeade79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 9 Feb 2015 14:51:44 +0100 Subject: [PATCH 2/4] Removed Redis table metadata usage fixing #2111 --- app/models/table.rb | 87 ++++++------------- app/models/table/privacy_manager.rb | 6 +- app/models/visualization/member.rb | 2 +- lib/cartodb/generic_migrator.rb | 9 +- lib/cartodb/migrator20.rb | 12 ++- lib/cartodb/migrator21.rb | 6 +- spec/models/table_spec.rb | 79 ++++------------- .../organization_visualization_spec.rb | 2 +- spec/requests/admin/visualizations_spec.rb | 2 +- 9 files changed, 67 insertions(+), 138 deletions(-) diff --git a/app/models/table.rb b/app/models/table.rb index 6da11ab91ac2..efd7a56a9ecd 100644 --- a/app/models/table.rb +++ b/app/models/table.rb @@ -97,16 +97,28 @@ def default_privacy_values def geometry_types if schema.select { |key, value| key == :the_geom }.length > 0 - owner.in_database[ %Q{ + query_geometry_types + else + [] + end + end + + def query_geometry_types + owner.in_database[ %Q{ SELECT DISTINCT ST_GeometryType(the_geom) FROM ( SELECT the_geom FROM "#{self.name}" WHERE (the_geom is not null) LIMIT 10 ) as foo }].all.map {|r| r[:st_geometrytype] } - else - [] - end + end + + def calculate_the_geom_type + return self.the_geom_type if self.the_geom_type.present? + + calculated = query_geometry_types.first + calculated.present? ? calculated.downcase.sub('st_', '') : DEFAULT_THE_GEOM_TYPE + self.the_geom_type = calculated end def is_raster? @@ -144,7 +156,7 @@ def is_raster? :import_from_query, :import_from_table_copy, :importing_encoding, - :temporal_the_geom_type, + :the_geom_type_value, :migrate_existing_table, # this flag is used to register table changes only without doing operations on in the database # for example when the table is renamed or created. For remove see keep_user_database_table @@ -527,10 +539,6 @@ def before_create else create_table_in_database! set_table_id - unless self.temporal_the_geom_type.blank? - self.the_geom_type = self.temporal_the_geom_type - self.temporal_the_geom_type = nil - end set_the_geom_column!(self.the_geom_type) end end @@ -547,7 +555,6 @@ def after_create set_default_table_privacy @force_schema = nil - $tables_metadata.hset key, 'user_id', user_id self.new_table = true # finally, close off the data import @@ -580,7 +587,7 @@ def after_save manager.set_from_table_privacy(privacy) manager.propagate_to(table_visualization) if privacy_changed? - manager.propagate_to_redis_and_varnish + manager.propagate_to_varnish update_cdb_tablemetadata end @@ -610,7 +617,6 @@ def handle_creation_error(e) # Remove the table, except if it already exists unless self.name.blank? || e.message =~ /relation .* already exists/ @data_import.log.append ("Import ERROR: Dropping table #{qualified_table_name}") if @data_import - $tables_metadata.del key self.remove_table_from_user_database unless keep_user_database_table end @@ -668,7 +674,6 @@ def after_destroy super # Delete visualization BEFORE deleting metadata, or named map won't be destroyed properly @table_visualization.delete(from_table_deletion=true) if @table_visualization - $tables_metadata.del key Tag.filter(:user_id => user_id, :table_id => id).delete remove_table_from_stats invalidate_varnish_cache @@ -835,18 +840,6 @@ def privacy_changed? previous_changes.keys.include?(:privacy) end #privacy_changed? - def key - Table.key(owner.database_name, "#{owner.database_schema}.#{name}") - rescue - nil - end - - # @param db_name String - # @param table_name String Must come fully qualified - def self.key(db_name, table_name) - "rails:#{db_name}:#{table_name}" - end - def sequel owner.in_database.from(sequel_qualified_table_name) end @@ -886,6 +879,9 @@ def schema(options = {}) last_columns = [] owner.in_database.schema(name, options.slice(:reload).merge(schema: owner.database_schema)).each do |column| next if column[0] == THE_GEOM_WEBMERCATOR + + calculate_the_geom_type if column[0] == :the_geom + col_db_type = column[1][:db_type].starts_with?('geometry') ? 'geometry' : column[1][:db_type] col = [ column[0], @@ -1200,41 +1196,24 @@ def georeference_from!(options = {}) end end - def the_geom_type - $tables_metadata.hget(key,'the_geom_type') || DEFAULT_THE_GEOM_TYPE - rescue => e - # FIXME: patch for Redis timeout errors, should be removed when the problem is solved - CartoDB::notify_error("Redis timeout error patched", error_info: "Table: #{self.name}. Connection: #{self.redis_connection_info}. Will retry.\n #{e.message} #{e.backtrace.join}") - begin - $tables_metadata.hget(key,'the_geom_type') || DEFAULT_THE_GEOM_TYPE - rescue => e - CartoDB::notify_error("Redis timeout error patched retry", error_info: "Table: #{self.name}. Connection: #{self.redis_connection_info}. Second error, won't retry.\n #{e.message} #{e.backtrace.join}") - raise e - end - end - - def redis_connection_info - "#{$tables_metadata.client.host}:#{$tables_metadata.client.port}, db: #{$tables_metadata.client.db}, timeout: #{$tables_metadata.client.timeout}" + self.the_geom_type_value end def the_geom_type=(value) - the_geom_type_value = case value.downcase + self.the_geom_type_value = case value.downcase when 'geometry' 'geometry' when 'point' 'point' when 'line' 'multilinestring' + when 'multipoint' + 'point' else value !~ /^multi/ ? "multi#{value.downcase}" : value.downcase end - raise CartoDB::InvalidGeomType unless CartoDB::VALID_GEOMETRY_TYPES.include?(the_geom_type_value) - if owner.in_database.table_exists?(name) - $tables_metadata.hset(key, 'the_geom_type', the_geom_type_value) - else - self.temporal_the_geom_type = the_geom_type_value - end + raise CartoDB::InvalidGeomType.new(self.the_geom_type_value) unless CartoDB::VALID_GEOMETRY_TYPES.include?(self.the_geom_type_value) end # if the table is already renamed, we just need to update the name attribute @@ -1418,19 +1397,6 @@ def update_name_changes if @name_changed_from.present? && @name_changed_from != name reload - old_key = Table.key(owner.database_name,"#{owner.database_schema}.#{@name_changed_from}") - new_key = key - - begin - # update metadata records - $tables_metadata.rename(old_key, new_key) - rescue StandardError => exception - # we don't want to fail because of not existing keys - exception_to_notify = CartoDB::BaseCartoDBError.new( - "Continuing after exception: Table update_name_changes(): '#{@name_changed_from}','#{key}' renaming metadata", exception) - CartoDB::notify_exception(exception_to_notify, user: owner) - end - unless register_table_only begin owner.in_database.rename_table(@name_changed_from, name) unless errored @@ -1455,7 +1421,6 @@ def update_name_changes end if errored - $tables_metadata.rename(new_key, old_key) raise exception_to_raise end end diff --git a/app/models/table/privacy_manager.rb b/app/models/table/privacy_manager.rb index 5e2e4427ba8e..f0de08b1e318 100644 --- a/app/models/table/privacy_manager.rb +++ b/app/models/table/privacy_manager.rb @@ -56,10 +56,9 @@ def privacy_for_redis end end - def propagate_to_redis_and_varnish + def propagate_to_varnish raise 'table privacy cannot be nil' unless privacy # TODO: Improve this, hack because tiler checks it - $tables_metadata.hset redis_key, 'privacy', privacy_for_redis invalidate_varnish_cache self end @@ -108,9 +107,6 @@ def varnish_key end end - def redis_key - "rails:#{table.owner.database_name}:#{owner.database_schema}.#{table.name}" - end end end diff --git a/app/models/visualization/member.rb b/app/models/visualization/member.rb index ebe8c30027fa..d390922d98c7 100644 --- a/app/models/visualization/member.rb +++ b/app/models/visualization/member.rb @@ -689,7 +689,7 @@ def propagate_privacy_to(table) if type == TYPE_CANONICAL CartoDB::TablePrivacyManager.new(table) .set_from(self) - .propagate_to_redis_and_varnish + .propagate_to_varnish end self end diff --git a/lib/cartodb/generic_migrator.rb b/lib/cartodb/generic_migrator.rb index 419c50f38873..ab3c6c72cbd9 100644 --- a/lib/cartodb/generic_migrator.rb +++ b/lib/cartodb/generic_migrator.rb @@ -16,16 +16,21 @@ def initialize(version) end #initialize def already_migrated?(table) - $tables_metadata.hget(table.key, "migrated_to_#{@version}").to_s == "true" + $tables_metadata.hget(key(table), "migrated_to_#{@version}").to_s == "true" end def migrated!(table) @stats[:tables_migrated] += 1 - $tables_metadata.hset(table.key, "migrated_to_#{@version}", true) + $tables_metadata.hset(key(table), "migrated_to_#{@version}", true) end def log msg @logger.debug(msg) end + + private + def key(table) + "rails:#{table.owner.database_name}:#{table.owner.database_schema}.#{table.name}") + end end end diff --git a/lib/cartodb/migrator20.rb b/lib/cartodb/migrator20.rb index 2b1e7af558b4..f0447020582c 100644 --- a/lib/cartodb/migrator20.rb +++ b/lib/cartodb/migrator20.rb @@ -55,7 +55,7 @@ def add_table_id(table) end def migrate_table_map(table) - map_metadata = JSON.parse($tables_metadata.hget(table.key, 'map_metadata')) rescue {} + map_metadata = JSON.parse($tables_metadata.hget(key(table), 'map_metadata')) rescue {} map = table.map # All previous maps were based on google maps @@ -73,8 +73,8 @@ def migrate_table_map(table) end def migrate_table_layers(table) - map_metadata = JSON.parse($tables_metadata.hget(table.key, 'map_metadata')) rescue {} - infowindow_metadata = JSON.parse($tables_metadata.hget(table.key, 'infowindow')) rescue {} + map_metadata = JSON.parse($tables_metadata.hget(key(table), 'map_metadata')) rescue {} + infowindow_metadata = JSON.parse($tables_metadata.hget(key(table), 'infowindow')) rescue {} # Data layer setup @@ -145,4 +145,10 @@ def migrate_table_layers(table) base_layer.save end end + + private + def key(table) + "rails:#{table.owner.database_name}:#{table.owner.database_schema}.#{table.name}") + end + end diff --git a/lib/cartodb/migrator21.rb b/lib/cartodb/migrator21.rb index f1fff35198bc..a5975781477b 100644 --- a/lib/cartodb/migrator21.rb +++ b/lib/cartodb/migrator21.rb @@ -51,9 +51,13 @@ def rollback! ::Table.db['delete from layers_user_tables'] # Remove redis keys @tables_to_migrate.all.each do |table| - $tables_metadata.hdel(table.key, "migrated_to_#{@version}") + $tables_metadata.hdel(key(table), "migrated_to_#{@version}") end end + private + def key(table) + "rails:#{table.owner.database_name}:#{table.owner.database_schema}.#{table.name}") + end end end # CartoDB diff --git a/spec/models/table_spec.rb b/spec/models/table_spec.rb index 32ddd895bf17..c9e2a7a7200d 100644 --- a/spec/models/table_spec.rb +++ b/spec/models/table_spec.rb @@ -183,7 +183,6 @@ def @data_import.data_source=(filepath) it "should have a privacy associated and it should be private by default" do table = create_table :user_id => @user.id table.should be_private - $tables_metadata.hget(table.key,'privacy').to_i.should == Table::PRIVACY_PRIVATE end it 'changes to and from public-with-link privacy' do @@ -441,7 +440,6 @@ def @data_import.data_source=(filepath) table = create_table(:user_id => @user.id) table.should be_private - $tables_metadata.hget(table.key,"privacy").to_i.should == Table::PRIVACY_PRIVATE table.privacy = Table::PRIVACY_PUBLIC table.save @@ -449,8 +447,6 @@ def @data_import.data_source=(filepath) expect { @user.in_database(:as => :public_user).run("select * from #{table.name}") }.to_not raise_error - - $tables_metadata.hget(table.key,"privacy").to_i.should == Table::PRIVACY_PUBLIC end it "should be associated to a database table" do @@ -532,11 +528,6 @@ def @data_import.data_source=(filepath) table.save end - it "should store the identifier of its owner when created" do - table = create_table(:user_id => @user.id) - $tables_metadata.hget(table.key,"user_id").should == table.user_id.to_s - end - it "should rename the pk sequence when renaming the table" do table1 = new_table :name => 'table 1', :user_id => @user.id table1.save.reload @@ -591,56 +582,22 @@ def @data_import.data_source=(filepath) end end - context "redis syncs" do - it "should have a unique key to be identified in Redis" do - table = create_table(:user_id => @user.id) - table.key.should == "rails:#{table.owner.database_name}:#{table.owner.database_schema}.#{table.name}" - end - - it "should rename the entries in Redis when the table has been renamed" do - table = create_table(:user_id => @user.id) - original_name = table.name - original_the_geom_type = table.the_geom_type - - table.name = "brand_new_name" - table.save_changes - table.reload - - table.key.should == "rails:#{table.owner.database_name}:#{table.owner.database_schema}.#{table.name}" - $tables_metadata.exists(table.key).should be_true - $tables_metadata.exists(original_name).should be_false - $tables_metadata.hget(table.key, "privacy").should be_present - $tables_metadata.hget(table.key, "user_id").should be_present - $tables_metadata.hget(table.key,"the_geom_type").should == original_the_geom_type - end - - it "should store the_geom_type in Redis" do - table = create_table(:user_id => @user.id) - - table.the_geom_type.should == "geometry" - $tables_metadata.hget(table.key,"the_geom_type").should == "geometry" - - table.the_geom_type = "multipolygon" - $tables_metadata.hget(table.key,"the_geom_type").should == "multipolygon" - end - - it "should remove varnish cache when updating the table privacy" do - CartoDB::NamedMapsWrapper::NamedMaps.any_instance.stubs(:get).returns(nil) - @user.private_tables_enabled = true - @user.save - table = create_table(user_id: @user.id, name: "varnish_privacy", privacy: Table::PRIVACY_PRIVATE) - - id = table.table_visualization.id - CartoDB::Varnish.any_instance.expects(:purge) - .times(2) - .with(".*#{id}:vizjson") - .returns(true) - - CartoDB::TablePrivacyManager.any_instance - .expects(:propagate_to_redis_and_varnish) - table.privacy = Table::PRIVACY_PUBLIC - table.save - end + it "should remove varnish cache when updating the table privacy" do + CartoDB::NamedMapsWrapper::NamedMaps.any_instance.stubs(:get).returns(nil) + @user.private_tables_enabled = true + @user.save + table = create_table(user_id: @user.id, name: "varnish_privacy", privacy: Table::PRIVACY_PRIVATE) + + id = table.table_visualization.id + CartoDB::Varnish.any_instance.expects(:purge) + .times(2) + .with(".*#{id}:vizjson") + .returns(true) + + CartoDB::TablePrivacyManager.any_instance + .expects(:propagate_to_varnish) + table.privacy = Table::PRIVACY_PUBLIC + table.save end context "when removing the table" do @@ -673,10 +630,6 @@ def @data_import.data_source=(filepath) @user.in_database["select * from #{table.name}"].all.should == [] end - it "should remove the table metadata from Redis" do - $tables_metadata.exists(@doomed_table.key).should be_false - end - it "should update denormalized counters" do @user.reload @user.tables_count.should == 0 diff --git a/spec/models/visualization/organization_visualization_spec.rb b/spec/models/visualization/organization_visualization_spec.rb index dac5b86c90a7..c04e3fcad8f2 100644 --- a/spec/models/visualization/organization_visualization_spec.rb +++ b/spec/models/visualization/organization_visualization_spec.rb @@ -29,7 +29,7 @@ UserOrganization.any_instance.stubs(:move_user_tables_to_schema).returns(nil) CartoDB::TablePrivacyManager.any_instance.stubs( :set_from_table_privacy => nil, - :propagate_to_redis_and_varnish => nil + :propagate_to_varnish => nil ) User.any_instance.stubs( diff --git a/spec/requests/admin/visualizations_spec.rb b/spec/requests/admin/visualizations_spec.rb index 23f90d3529f0..0a3420c9d432 100644 --- a/spec/requests/admin/visualizations_spec.rb +++ b/spec/requests/admin/visualizations_spec.rb @@ -222,7 +222,7 @@ def app CartoDB::UserOrganization.any_instance.stubs(:move_user_tables_to_schema).returns(nil) CartoDB::TablePrivacyManager.any_instance.stubs( :set_from_table_privacy => nil, - :propagate_to_redis_and_varnish => nil + :propagate_to_varnish => nil ) User.any_instance.stubs( From 3175b8b0c75dbdbd98cf7cbbd4ca2f4f725e79b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 9 Feb 2015 16:54:15 +0100 Subject: [PATCH 3/4] Fixed calculated geometry type --- app/models/table.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/table.rb b/app/models/table.rb index efd7a56a9ecd..9d2734b9cdfc 100644 --- a/app/models/table.rb +++ b/app/models/table.rb @@ -117,7 +117,7 @@ def calculate_the_geom_type return self.the_geom_type if self.the_geom_type.present? calculated = query_geometry_types.first - calculated.present? ? calculated.downcase.sub('st_', '') : DEFAULT_THE_GEOM_TYPE + calculated = calculated.present? ? calculated.downcase.sub('st_', '') : DEFAULT_THE_GEOM_TYPE self.the_geom_type = calculated end From 6ab785f592111cdc4559ff329562f3f2cab20dd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Wed, 11 Feb 2015 16:08:09 +0100 Subject: [PATCH 4/4] Removed privacy_for_redis --- app/models/table/privacy_manager.rb | 9 --------- lib/varnish/lib/cartodb-varnish.rb | 1 - 2 files changed, 10 deletions(-) diff --git a/app/models/table/privacy_manager.rb b/app/models/table/privacy_manager.rb index f0de08b1e318..8d94319c2c6c 100644 --- a/app/models/table/privacy_manager.rb +++ b/app/models/table/privacy_manager.rb @@ -47,15 +47,6 @@ def propagate_to(visualization) self end - def privacy_for_redis - case @table.privacy - when ::Table::PRIVACY_PUBLIC, ::Table::PRIVACY_LINK - ::Table::PRIVACY_PUBLIC - else - ::Table::PRIVACY_PRIVATE - end - end - def propagate_to_varnish raise 'table privacy cannot be nil' unless privacy # TODO: Improve this, hack because tiler checks it diff --git a/lib/varnish/lib/cartodb-varnish.rb b/lib/varnish/lib/cartodb-varnish.rb index 6a1abee6d4f1..8a8e92bcfe69 100644 --- a/lib/varnish/lib/cartodb-varnish.rb +++ b/lib/varnish/lib/cartodb-varnish.rb @@ -4,7 +4,6 @@ module CartoDB class Varnish def purge(what) ActiveSupport::Notifications.instrument('purge.varnish', what: what) do |payload| - conf = Cartodb::config[:varnish_management] if conf['http_port'] request = Typhoeus::Request.new(