Merge branch 'master' into 1761-dataview-source

NEWS.md

@@ -379,6 +379,7 @@ More information at [Dropbox migration guide](https://www.dropbox.com/developers
 * Show infowindow when user reaches max layer limit (#12167)
 * Format quota infowindow numbers (#11743)
 * Improved analysis error tooltip (#12250)
+* Rollback failed user/organization imports
 * Enable user migrations across clouds (#12795)
 
 ### Bug fixes

app/models/carto/user_migration_import.rb

@@ -46,7 +46,7 @@ module Carto
       update_attributes(state: STATE_FAILURE)
       false
     ensure
-      package.cleanup
+      package.try(:cleanup)
     end
 
     def enqueue
@@ -64,30 +64,80 @@ module Carto
     end
 
     def import(service, package)
-      if import_metadata?
-        log.append('=== Importing metadata ===')
+      imported = do_import_metadata(package, service) if import_metadata?
+      do_import_data(package, service)
+      import_visualizations(imported, package, service) if import_metadata?
+    end
+
+    def do_import_metadata(package, service)
+      log.append('=== Importing metadata ===')
+      begin
         imported = service.import_from_directory(package.meta_dir)
-        org_import? ? self.organization = imported : self.user = imported
-        update_database_host
-        save!
+      rescue UserAlreadyExists, OrganizationAlreadyExists => e
+        log.append('Organization already exists. Skipping!')
+        raise e
+      rescue => e
+        log.append('=== Error importing metadata. Rollback! ===')
+        service.rollback_import_from_directory(package.meta_dir)
+        raise e
       end
+      org_import? ? self.organization = imported : self.user = imported
+      update_database_host
+      save!
+      imported
+    end
 
+    def do_import_data(package, service)
       log.append('=== Importing data ===')
-      CartoDB::DataMover::ImportJob.new(import_job_arguments(package.data_dir)).run!
+      import_job = CartoDB::DataMover::ImportJob.new(import_job_arguments(package.data_dir))
+      begin
+        import_job.run!
+      rescue => e
+        log.append('=== Error importing data. Rollback! ===')
+        rollback_import_data(package)
+        service.rollback_import_from_directory(package.meta_dir) if import_metadata?
+        raise e
+      ensure
+        import_job.terminate_connections
+      end
+    end
 
-      if import_metadata?
-        log.append('=== Importing visualizations and search tweets ===')
-        service.import_metadata_from_directory(imported, package.meta_dir)
+    def import_visualizations(imported, package, service)
+      log.append('=== Importing visualizations and search tweets ===')
+      begin
+        ActiveRecord::Base.transaction do
+          service.import_metadata_from_directory(imported, package.meta_dir)
+        end
+      rescue => e
+        log.append('=== Error importing visualizations and search tweets. Rollback! ===')
+        rollback_import_data(package)
+        service.rollback_import_from_directory(package.meta_dir)
+        raise e
       end
     end
 
+    def rollback_import_data(package)
+      import_job = CartoDB::DataMover::ImportJob.new(
+        import_job_arguments(package.data_dir).merge(rollback: true,
+                                                     mode: :rollback,
+                                                     drop_database: true,
+                                                     drop_roles: true)
+      )
+
+      import_job.run!
+      import_job.terminate_connections
+    rescue => e
+      log.append('There was an error while rolling back import data:' + e.to_s)
+    end
+
     def update_database_host
       users.each do |user|
         Rollbar.info("Updating database conection for user #{user.username} to #{database_host}")
         user.database_host = database_host
         user.save!
-        ::User[user.id].reload # This is because Sequel models are being cached along request. This forces reload.
-                               # It's being used in visualizations_export_persistence_service.rb#save_import
+        # This is because Sequel models are being cached along request. This forces reload.
+        # It's being used in visualizations_export_persistence_service.rb#save_import
+        ::User[user.id].reload
       end
     end
 

app/models/user.rb

@@ -399,7 +399,7 @@ class User < Sequel::Model
     @force_destroy = true
   end
 
-  def before_destroy
+  def before_destroy(skip_table_drop: false)
     ensure_nonviewer
 
     @org_id_for_org_wipe = nil
@@ -464,19 +464,28 @@ class User < Sequel::Model
     # Invalidate user cache
     invalidate_varnish_cache
 
-    # Delete the DB or the schema
+    drop_database(has_organization) unless skip_table_drop || error_happened
+
+    # Remove metadata from redis last (to avoid cutting off access to SQL API if db deletion fails)
+    unless error_happened
+      $users_metadata.DEL(key)
+      $users_metadata.DEL(timeout_key)
+    end
+
+    feature_flags_user.each(&:delete)
+  end
+
+  def drop_database(has_organization)
     if has_organization
-      unless error_happened
-        db_service.drop_organization_user(
-          organization_id,
-          is_owner: !@org_id_for_org_wipe.nil?,
-          force_destroy: @force_destroy
-        )
-      end
+      db_service.drop_organization_user(
+        organization_id,
+        is_owner: !@org_id_for_org_wipe.nil?,
+        force_destroy: @force_destroy
+      )
     elsif ::User.where(database_name: database_name).count > 1
       raise CartoDB::BaseCartoDBError.new(
         'The user is not supposed to be in a organization but another user has the same database_name. Not dropping it')
-    elsif !error_happened
+    else
       Thread.new {
         conn = in_database(as: :cluster_admin)
         db_service.drop_database_and_user(conn)
@@ -484,14 +493,6 @@ class User < Sequel::Model
       }.join
       db_service.monitor_user_notification
     end
-
-    # Remove metadata from redis last (to avoid cutting off access to SQL API if db deletion fails)
-    unless error_happened
-      $users_metadata.DEL(key)
-      $users_metadata.DEL(timeout_key)
-    end
-
-    feature_flags_user.each(&:delete)
   end
 
   def delete_external_data_imports

app/models/user/db_service.rb

@@ -361,6 +361,7 @@ module CartoDB
           if !retried && e.message =~ /cannot be dropped because some objects depend on it/
             retried = true
             e.message =~ /object[s]? in database (.*)$/
+            e.message =~ /privileges for database (.*)$/ unless $1
             if database_with_conflicts == $1
               raise e
             else

app/services/carto/organization_metadata_export_service.rb

@@ -166,6 +166,7 @@ module Carto
     end
   end
 
+  class OrganizationAlreadyExists < RuntimeError; end
   # Both String and Hash versions are provided because `deep_symbolize_keys` won't symbolize through arrays
   # and having separated methods make handling and testing much easier.
   class OrganizationMetadataExportService
@@ -190,10 +191,10 @@ module Carto
 
     def import_from_directory(meta_path)
       # Import organization
-      organization_file = Dir["#{meta_path}/organization_*.json"].first
-      organization = build_organization_from_json_export(File.read(organization_file))
+      organization = load_organization_from_directory(meta_path)
+      raise OrganizationAlreadyExists.new if ::Carto::Organization.exists?(id: organization.id)
 
-      organization_redis_file = Dir["#{meta_path}/redis_organization_*.json"].first
+      organization_redis_file = redis_filename(meta_path)
       Carto::RedisExportService.new.restore_redis_from_json_export(File.read(organization_redis_file))
 
       # Groups and notifications must be saved after users
@@ -204,7 +205,7 @@ module Carto
 
       save_imported_organization(organization)
 
-      user_list = Dir["#{meta_path}/user_*"]
+      user_list = get_user_list(meta_path)
 
       # In order to get permissions right, we first import all users, then all datasets and finally, all maps
       organization.users = user_list.map do |user_path|
@@ -218,6 +219,37 @@ module Carto
       organization
     end
 
+    def rollback_import_from_directory(meta_path)
+      organization_redis_file = redis_filename(meta_path)
+      Carto::RedisExportService.new.remove_redis_from_json_export(File.read(organization_redis_file))
+      organization = load_organization_from_directory(meta_path)
+
+      user_list = get_user_list(meta_path)
+      user_list.map do |user_path|
+        Carto::UserMetadataExportService.new.rollback_import_from_directory(user_path)
+      end
+      return unless Carto::Organization.exists?(organization.id)
+
+      organization = Carto::Organization.find(organization.id)
+      organization.groups.delete
+      organization.notifications.delete
+      organization.users.delete
+      organization.delete
+    end
+
+    def get_user_list(meta_path)
+      Dir["#{meta_path}/user_*"]
+    end
+
+    def redis_filename(meta_path)
+      Dir["#{meta_path}/redis_organization_*.json"].first
+    end
+
+    def load_organization_from_directory(meta_path)
+      organization_file = Dir["#{meta_path}/organization_*.json"].first
+      build_organization_from_json_export(File.read(organization_file))
+    end
+
     def import_metadata_from_directory(organization, path)
       organization.users.each do |user|
         Carto::UserMetadataExportService.new.import_user_visualizations_from_directory(

app/services/carto/redis_export_service.rb

@@ -18,23 +18,43 @@ module Carto
       restore_redis_from_hash_export(JSON.parse(exported_json_string).deep_symbolize_keys)
     end
 
+    def remove_redis_from_json_export(exported_json_string)
+      remove_redis_from_hash_export(JSON.parse(exported_json_string).deep_symbolize_keys)
+    end
+
     def restore_redis_from_hash_export(exported_hash)
       raise 'Wrong export version' unless compatible_version?(exported_hash[:version])
 
       restore_redis(exported_hash[:redis])
     end
 
+    def remove_redis_from_hash_export(exported_hash)
+      raise 'Wrong export version' unless compatible_version?(exported_hash[:version])
+
+      remove_redis(exported_hash[:redis])
+    end
+
     private
 
     def restore_redis(redis_export)
       restore_keys($users_metadata, redis_export[:users_metadata])
     end
 
+    def remove_redis(redis_export)
+      remove_keys($users_metadata, redis_export[:users_metadata])
+    end
+
     def restore_keys(redis_db, redis_keys)
       redis_keys.each do |key, value|
         redis_db.restore(key, value[:ttl], Base64.decode64(value[:value]))
       end
     end
+
+    def remove_keys(redis_db, redis_keys)
+      redis_keys.each do |key|
+        redis_db.del(key)
+      end
+    end
   end
 
   module RedisExportServiceExporter

app/services/carto/user_metadata_export_service.rb

@@ -176,6 +176,8 @@ module Carto
     end
   end
 
+  class UserAlreadyExists < RuntimeError; end
+
   # Both String and Hash versions are provided because `deep_symbolize_keys` won't symbolize through arrays
   # and having separated methods make handling and testing much easier.
   class UserMetadataExportService
@@ -198,16 +200,30 @@ module Carto
     end
 
     def import_from_directory(path)
-      user_file = Dir["#{path}/user_*.json"].first
-      user = build_user_from_json_export(File.read(user_file))
-
+      user = user_from_file(path)
+      raise UserAlreadyExists.new if ::Carto::User.exists?(id: user.id)
       save_imported_user(user)
 
-      Carto::RedisExportService.new.restore_redis_from_json_export(File.read(Dir["#{path}/redis_user_*.json"].first))
+      Carto::RedisExportService.new.restore_redis_from_json_export(redis_user_file(path))
 
       user
     end
 
+    def rollback_import_from_directory(path)
+      user = user_from_file(path)
+      return unless user
+
+      user = ::User[user.id]
+      return unless user
+
+      Carto::User.find(user.id).destroy
+      user.before_destroy(skip_table_drop: true)
+
+      Carto::RedisExportService.new.remove_redis_from_json_export(redis_user_file(path))
+    rescue ActiveRecord::RecordNotFound
+      # User was not created so not found and no redis removal needed
+    end
+
     def import_user_visualizations_from_directory(user, type, meta_path)
       with_non_viewer_user(user) do
         Dir["#{meta_path}/#{type}_*#{Carto::VisualizationExporter::EXPORT_EXTENSION}"].each do |fname|
@@ -229,13 +245,25 @@ module Carto
     end
 
     def import_search_tweets_from_directory(user, meta_path)
-      user_file = Dir["#{meta_path}/user_*.json"].first
+      user_file = user_file_dir(meta_path)
       search_tweets = build_search_tweets_from_json_export(File.read(user_file))
       search_tweets.each { |st| save_imported_search_tweet(st, user) }
     end
 
     private
 
+    def user_from_file(path)
+      build_user_from_json_export(File.read(user_file_dir(path)))
+    end
+
+    def user_file_dir(path)
+      Dir["#{path}/user_*.json"].first
+    end
+
+    def redis_user_file(path)
+      File.read(Dir["#{path}/redis_user_*.json"].first)
+    end
+
     def export_user_visualizations_to_directory(user, type, path)
       root_dir = Pathname.new(path)
       user.visualizations.where(type: type).each do |visualization|

app/services/carto/visualizations_export_persistence_service.rb

@@ -107,7 +107,6 @@ module Carto
       end
 
       visualization.save!
-
       visualization
     end
 

services/user-mover/import_user.rb

@@ -61,6 +61,15 @@ module CartoDB
         end
       end
 
+      def rollback!
+        close_all_database_connections
+        if @pack_config['organization']
+          rollback_org
+        else
+          rollback_user
+        end
+      end
+
       def organization_import?
         @pack_config['organization'] != nil
       end
@@ -266,7 +275,7 @@ module CartoDB
       end
 
       def drop_role(role)
-        superuser_pg_conn.query("DROP ROLE \"#{role}\"")
+        superuser_pg_conn.query("DROP ROLE IF EXISTS \"#{role}\"")
       end
 
       def get_org_info(orgname)
@@ -289,9 +298,22 @@ module CartoDB
         superuser_pg_conn.query("ALTER DATABASE #{superuser_pg_conn.quote_ident(db)} SET statement_timeout = #{timeout}")
       end
 
+      def close_all_database_connections(database_name = @target_dbname)
+        superuser_pg_conn.query("SELECT pg_terminate_backend(pg_stat_activity.pid)
+                                  FROM pg_stat_activity
+                                WHERE pg_stat_activity.datname = '#{database_name}'
+                                AND pid <> pg_backend_pid();")
+        terminate_connections
+      end
+
       def terminate_connections
+        @user_conn && @user_conn.close
         @user_conn = nil
+
+        @superuser_user_conn && @superuser_user_conn.close
         @superuser_user_conn = nil
+
+        @superuser_conn && @superuser_conn.close
         @superuser_conn = nil
       end
 
@@ -320,7 +342,8 @@ module CartoDB
       end
 
       def drop_database(db_name)
-        superuser_pg_conn.query("DROP DATABASE \"#{db_name}\"")
+        close_all_database_connections(db_name)
+        superuser_pg_conn.query("DROP DATABASE IF EXISTS \"#{db_name}\"")
       end
 
       def clean_oids(user_id, user_schema)

spec/models/carto/user_migration_import_spec.rb

@@ -44,6 +44,9 @@ describe Carto::UserMigrationImport do
     end
 
     def setup_mocks
+      @organization_mock = Carto::Organization.new
+      @import.stubs(:assert_organization_does_not_exist)
+      @import.stubs(:assert_user_does_not_exist)
       @user_migration_package_mock = Object.new
       Carto::UserMigrationPackage.stubs(:for_import).returns @user_migration_package_mock
       @user_migration_package_mock.stubs(:download).with(:irrelevant_file)
@@ -52,11 +55,11 @@ describe Carto::UserMigrationImport do
       @user_mock = Carto::User.new
       @export_job_mock = Object.new
       @export_job_mock.expects(:run!).once
+      @export_job_mock.expects(:terminate_connections).once
       CartoDB::DataMover::ImportJob.stubs(:new).returns @export_job_mock
       @user_migration_package_mock.stubs(:cleanup)
       @import.expects(:save!).once.returns @import
 
-      @organization_mock = Carto::Organization.new
     end
 
     def expected_job_arguments

spec/models/carto/user_migration_spec.rb

@@ -74,26 +74,190 @@ describe 'UserMigration' do
   it_should_behave_like 'migrating metadata', true
   it_should_behave_like 'migrating metadata', false
 
+  describe 'failing user imports should rollback' do
+    before :each do
+      @user = create_user_with_visualizations
+      @carto_user = Carto::User.find(@user.id)
+      @user_attributes = @carto_user.attributes
+
+      @export = Carto::UserMigrationExport.create(
+        user: @carto_user,
+        export_metadata: true
+      )
+      @export.run_export
+      destroy_user
+    end
+
+    after :each do
+      @carto_user.destroy
+    end
+
+    it 'import failing in import_metadata should rollback' do
+      Carto::RedisExportService.any_instance.stubs(:restore_redis_from_hash_export).raises('Some exception')
+
+      imp = import
+      imp.run_import.should eq false
+      imp.state.should eq 'failure'
+
+      Carto::RedisExportService.any_instance.unstub(:restore_redis_from_hash_export)
+
+      import.run_import.should eq true
+    end
+
+    it 'import failing in JobImport#run!' do
+      CartoDB::DataMover::ImportJob.any_instance.stubs(:grant_user_role).raises('Some exception')
+
+      imp = import
+      imp.run_import.should eq false
+      imp.state.should eq 'failure'
+
+      CartoDB::DataMover::ImportJob.any_instance.unstub(:grant_user_role)
+
+      import.run_import.should eq true
+    end
+
+    it 'import failing creating user database and roles' do
+      CartoDB::DataMover::ImportJob.any_instance.stubs(:import_pgdump).raises('Some exception')
+
+      imp = import
+      imp.run_import.should eq false
+      imp.state.should eq 'failure'
+
+      CartoDB::DataMover::ImportJob.any_instance.unstub(:import_pgdump)
+
+      import.run_import.should eq true
+    end
+
+    it 'import failing importing visualizations' do
+      Carto::UserMetadataExportService.any_instance.stubs(:import_search_tweets_from_directory).raises('Some exception')
+
+      imp = import
+      imp.run_import.should eq false
+      imp.state.should eq 'failure'
+
+      Carto::UserMetadataExportService.any_instance.unstub(:import_search_tweets_from_directory)
+
+      import.run_import.should eq true
+    end
+
+    it 'fails importing an already existing user' do
+      import.run_import.should eq true
+      import.run_import.should eq false
+    end
+
+    it 'should continue with rollback if data import rollback fails' do
+      CartoDB::DataMover::ImportJob.any_instance.stubs(:grant_user_role).raises('Some exception')
+      CartoDB::DataMover::ImportJob.any_instance.stubs(:rollback_user).raises('Some exception')
+      import.run_import.should eq false
+      CartoDB::DataMover::ImportJob.any_instance.unstub(:grant_user_role)
+      CartoDB::DataMover::ImportJob.any_instance.unstub(:rollback_user)
+      import.run_import.should eq true
+    end
+
+    it 'should not remove user if already exists while importing' do
+      import.run_import.should eq true
+      import.run_import.should eq false
+      Carto::User.exists?(@user.id).should eq true
+    end
+  end
+
+  describe 'failing organization organizations should rollback' do
+    include_context 'organization with users helper'
+    before :all do
+      owner = @carto_organization.owner
+      filepath = "#{Rails.root}/services/importer/spec/fixtures/visualization_export_with_two_tables.carto"
+      data_import = DataImport.create(
+        user_id: owner.id,
+        data_source: filepath,
+        updated_at: Time.now.utc,
+        append: false,
+        create_visualization: true
+      )
+      data_import.values[:data_source] = filepath
+
+      data_import.run_import!
+      data_import.success.should eq true
+
+      @export = Carto::UserMigrationExport.create(
+        organization: @carto_organization,
+        export_metadata: true
+      )
+      @export.run_export
+      @organization.destroy_cascade
+    end
+
+    after :each do
+      begin
+        @organization.reload
+        @organization.destroy_cascade
+      rescue
+      end
+    end
+
+    it 'import failing in import_metadata should rollback' do
+      Carto::RedisExportService.any_instance.stubs(:restore_redis_from_hash_export).raises('Some exception')
+
+      imp = org_import
+      imp.run_import.should eq false
+      imp.state.should eq 'failure'
+
+      Carto::RedisExportService.any_instance.unstub(:restore_redis_from_hash_export)
+
+      org_import.run_import.should eq true
+    end
+
+    it 'import failing in JobImport#run!' do
+      CartoDB::DataMover::ImportJob.any_instance.stubs(:grant_user_role).raises('Some exception')
+
+      imp = org_import
+      imp.run_import.should eq false
+      imp.state.should eq 'failure'
+
+      CartoDB::DataMover::ImportJob.any_instance.unstub(:grant_user_role)
+
+      org_import.run_import.should eq true
+    end
+
+    it 'import failing creating user database and roles' do
+      CartoDB::DataMover::ImportJob.any_instance.stubs(:import_pgdump).raises('Some exception')
+
+      imp = org_import
+      imp.run_import.should eq false
+      imp.state.should eq 'failure'
+
+      CartoDB::DataMover::ImportJob.any_instance.unstub(:import_pgdump)
+
+      org_import.run_import.should eq true
+    end
+
+    it 'import failing importing visualizations' do
+      Carto::UserMetadataExportService.any_instance.stubs(:import_search_tweets_from_directory).raises('Some exception')
+
+      imp = org_import
+      imp.run_import.should eq false
+      imp.state.should eq 'failure'
+
+      Carto::UserMetadataExportService.any_instance.unstub(:import_search_tweets_from_directory)
+
+      org_import.run_import.should eq true
+    end
+
+    it 'should fail if importing an already existing organization with metadata' do
+      org_import.run_import.should eq true
+      imp = org_import
+      imp.run_import.should eq false
+      imp.state.should eq 'failure'
+    end
+  end
+
   it 'exports and imports a user with a data import with two tables' do
     CartoDB::UserModule::DBService.any_instance.stubs(:enable_remote_db_user).returns(true)
 
-    user = FactoryGirl.build(:valid_user).save
+    user = create_user_with_visualizations
+
     carto_user = Carto::User.find(user.id)
     user_attributes = carto_user.attributes
 
-    filepath = "#{Rails.root}/services/importer/spec/fixtures/visualization_export_with_two_tables.carto"
-    data_import = DataImport.create(
-      user_id: user.id,
-      data_source: filepath,
-      updated_at: Time.now.utc,
-      append: false,
-      create_visualization: true
-    )
-    data_import.values[:data_source] = filepath
-
-    data_import.run_import!
-    data_import.success.should eq true
-
     source_visualizations = carto_user.visualizations.map(&:name).sort
 
     export = Carto::UserMigrationExport.create(
@@ -115,6 +279,8 @@ describe 'UserMigration' do
       json_file: export.json_file,
       import_metadata: true
     )
+    import.stubs(:assert_organization_does_not_exist)
+    import.stubs(:assert_user_does_not_exist)
     import.run_import
 
     puts import.log.entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE
@@ -211,6 +377,8 @@ describe 'UserMigration' do
           json_file: export.json_file,
           import_metadata: migrate_metadata
         )
+        import.stubs(:assert_organization_does_not_exist)
+        import.stubs(:assert_user_does_not_exist)
         import.run_import
 
         puts import.log.entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE
@@ -258,4 +426,56 @@ describe 'UserMigration' do
   def attributes_to_test(user_attributes)
     user_attributes.keys - %w(created_at updated_at period_end_date)
   end
+
+  private
+
+  def create_user_with_visualizations
+    user = FactoryGirl.build(:valid_user).save
+
+    filepath = "#{Rails.root}/services/importer/spec/fixtures/visualization_export_with_two_tables.carto"
+    data_import = DataImport.create(
+      user_id: user.id,
+      data_source: filepath,
+      updated_at: Time.now.utc,
+      append: false,
+      create_visualization: true
+    )
+    data_import.values[:data_source] = filepath
+
+    data_import.run_import!
+    data_import.success.should eq true
+    user
+  end
+
+  def org_import
+    imp = Carto::UserMigrationImport.create(
+      exported_file: @export.exported_file,
+      database_host: @carto_organization.owner.attributes['database_host'],
+      org_import: true,
+      json_file: @export.json_file,
+      import_metadata: true
+    )
+
+    imp.stubs(:assert_organization_does_not_exist)
+    imp.stubs(:assert_user_does_not_exist)
+    imp
+  end
+
+  def import
+    imp = Carto::UserMigrationImport.create(
+      exported_file: @export.exported_file,
+      database_host: @user_attributes['database_host'],
+      org_import: false,
+      json_file: @export.json_file,
+      import_metadata: true
+    )
+    imp.stubs(:assert_organization_does_not_exist)
+    imp.stubs(:assert_user_does_not_exist)
+    imp
+  end
+
+  def destroy_user
+    @carto_user.client_applications.each(&:destroy)
+    @user.destroy
+  end
 end