- Fixes #2658 - CSV-Import doesn't lookup records by "name" field.

- Fixes #2659 - CSV-Import doesn't (always) handle database errors correctly.

Attention: This introduces a behavior change: Before (other) data was still imported on errors, now data only will get imported when no error occurs.
This commit is contained in:
Ryan Lue 2019-10-21 12:44:52 +02:00 committed by Thorsten Eckel
parent 1d0d227575
commit dbe13c2ed6
17 changed files with 240 additions and 260 deletions

View file

@ -47,11 +47,12 @@ returns
private
def find_and_save_to_cache_by(attr)
if !Rails.application.config.db_case_sensitive && string_key?(attr.keys.first)
where(attr).find { |r| r[attr.keys.first] == attr.values.first }
else
find_by(attr)
end.tap { |r| cache_set(attr.values.first, r) }
record = find_by(attr)
return nil if string_key?(attr.keys.first) && (record&.send(attr.keys.first) != attr.values.first) # enforce case-sensitivity on MySQL
return record if ActiveRecord::Base.connection.transaction_open? # rollbacks can invalidate cache entries
cache_set(attr.values.first, record)
record
end
def string_key?(key)

View file

@ -180,41 +180,48 @@ returns
csv_object_ids_ignored = @csv_object_ids_ignored || []
records = []
line_count = 0
payload.each do |attributes|
line_count += 1
record = nil
lookup_keys.each do |lookup_by|
next if attributes[lookup_by].blank?
params = {}
params[lookup_by] = if %i[email login].include?(lookup_by)
attributes[lookup_by].downcase
else
attributes[lookup_by]
end
record = lookup(params)
break if record
end
Transaction.execute(disable_notification: true, bulk: true) do
payload.each do |attributes|
line_count += 1
record = nil
lookup_keys.each do |lookup_by|
next if attributes[lookup_by].blank?
if attributes[:id].present? && !record
errors.push "Line #{line_count}: unknown record with id '#{attributes[:id]}' for #{new.class}."
next
end
record = if lookup_by.in?(%i[name])
find_by("LOWER(#{lookup_by}) = ?", attributes[lookup_by].downcase)
elsif lookup_by.in?(%i[email login])
lookup(attributes.slice(lookup_by).transform_values!(&:downcase))
else
lookup(attributes.slice(lookup_by))
end
if record && csv_object_ids_ignored.include?(record.id)
errors.push "Line #{line_count}: unable to update record with id '#{attributes[:id]}' for #{new.class}."
next
end
break if record
end
begin
clean_params = association_name_to_id_convert(attributes)
rescue => e
errors.push "Line #{line_count}: #{e.message}"
next
end
if record.in?(records)
errors.push "Line #{line_count}: duplicate record found."
next
end
# create object
Transaction.execute(disable_notification: true, reset_user_id: true, bulk: true) do
if attributes[:id].present? && !record
errors.push "Line #{line_count}: unknown record with id '#{attributes[:id]}' for #{new.class}."
next
end
if record && csv_object_ids_ignored.include?(record.id)
errors.push "Line #{line_count}: unable to update record with id '#{attributes[:id]}' for #{new.class}."
next
end
begin
clean_params = association_name_to_id_convert(attributes)
rescue => e
errors.push "Line #{line_count}: #{e.message}"
next
end
# create object
UserInfo.current_user_id = clean_params[:updated_by_id] || clean_params[:created_by_id]
if !record || delete == true
stats[:created] += 1
@ -227,8 +234,6 @@ returns
clean_params[:updated_by_id] = 1
end
record = new(clean_params)
next if try == true
record.associations_from_param(attributes)
record.save!
rescue => e
@ -237,8 +242,6 @@ returns
end
else
stats[:updated] += 1
next if try == true
begin
csv_verify_attributes(clean_params)
clean_params = param_cleanup(clean_params)
@ -261,14 +264,11 @@ returns
next
end
end
records.push record if record
end
records.push record
end
result = 'success'
if errors.present?
result = 'failed'
ensure
raise ActiveRecord::Rollback if try || errors.any?
end
{
@ -276,7 +276,7 @@ returns
records: records,
errors: errors,
try: try,
result: result,
result: errors.empty? ? 'success' : 'failed',
}
end

View file

@ -0,0 +1,109 @@
RSpec.shared_examples 'ApplicationModel::CanLookup' do
describe '.lookup_keys' do
it 'returns a subset of: id, name, login, email, number' do
expect(described_class.lookup_keys)
.to all(be_in(%i[id name login email number]))
end
it 'only includes attributes present on the model' do
expect(described_class.lookup_keys)
.to all(be_in(described_class.attribute_names.map(&:to_sym)))
end
end
describe '.lookup' do
around do |example|
Rails.cache.clear
example.run
Rails.cache.clear
end
let!(:instance) { create(described_class.name.underscore) }
let(:valid_lookup_key) { (described_class.lookup_keys - [:id]).sample }
let(:invalid_lookup_key) { (described_class.attribute_names.map(&:to_sym) - described_class.lookup_keys).sample }
it 'accepts exactly one attribute-value pair' do
expect { described_class.lookup(id: instance.id, valid_lookup_key => 'foo') }
.to raise_error(ArgumentError)
end
it 'only accepts attributes from .lookup_keys' do
expect { described_class.lookup(invalid_lookup_key => 'foo') }
.to raise_error(ArgumentError)
end
shared_examples 'per-attribute examples' do |attribute|
it "finds records by #{attribute} (like .find_by)" do
expect(described_class.lookup(attribute => instance.send(attribute))).to eq(instance)
end
describe "cache storage by #{attribute}" do
context 'inside a DB transaction' do # provided by default RSpec config
it 'leaves the cache untouched' do
expect { described_class.lookup(attribute => instance.send(attribute)) }
.not_to change { described_class.cache_get(instance.send(attribute)) }
end
end
context 'outside a DB transaction' do
before do
allow(ActiveRecord::Base.connection)
.to receive(:transaction_open?).and_return(false)
end
context 'when called for the first time' do
it 'saves the value to the cache' do
expect(Rails.cache)
.to receive(:write)
.with("#{described_class}::#{instance.send(attribute)}", instance, { expires_in: 7.days })
.and_call_original
expect { described_class.lookup(attribute => instance.send(attribute)) }
.to change { described_class.cache_get(instance.send(attribute)) }
.to(instance)
end
end
context 'when called a second time' do
before { described_class.lookup(attribute => instance.send(attribute)) }
it 'retrieves results from cache' do
expect(Rails.cache)
.to receive(:read)
.with("#{described_class}::#{instance.send(attribute)}")
described_class.lookup(attribute => instance.send(attribute))
end
if attribute != :id
context 'after it has been updated' do
let!(:old_attribute_val) { instance.send(attribute) }
let!(:new_attribute_val) { instance.send(attribute).next }
it 'moves the record to a new cache key' do
expect { instance.update(attribute => new_attribute_val) }
.to change { described_class.cache_get(old_attribute_val) }.to(nil)
expect { described_class.lookup({ attribute => instance.send(attribute) }) }
.to change { described_class.cache_get(new_attribute_val) }.to(instance)
end
end
end
context 'after it has been destroyed' do
it 'returns nil' do
expect { instance.destroy }
.to change { described_class.cache_get(instance.send(attribute)) }
.to(nil)
end
end
end
end
end
end
described_class.lookup_keys.each do |key|
include_examples 'per-attribute examples', key
end
end
end

View file

@ -1,11 +1,13 @@
require 'models/application_model/can_assets_examples'
require 'models/application_model/can_associations_examples'
require 'models/application_model/can_latest_change_examples'
require 'models/application_model/can_lookup_examples'
require 'models/application_model/checks_import_examples'
RSpec.shared_examples 'ApplicationModel' do |options = {}|
include_examples 'ApplicationModel::CanAssets', options[:can_assets]
include_examples 'ApplicationModel::CanAssociations'
include_examples 'ApplicationModel::CanLatestChange'
include_examples 'ApplicationModel::CanLookup'
include_examples 'ApplicationModel::ChecksImport'
end

View file

@ -0,0 +1,52 @@
RSpec.shared_examples 'CanCsvImport' do |unique_attributes: []|
describe '.csv_import' do
let!(:params) { { string: <<~CSV, parse_params: { col_sep: ',' } } }
#{described_class.attribute_names.join(',')}
#{build(factory).attributes.values.join(',')}
CSV
let(:factory) { described_class.name.underscore }
context 'with duplicate entries for unique attributes' do
shared_examples 'case-insensitive duplicates' do |attribute|
let!(:params) { { string: <<~CSV, parse_params: { col_sep: ',' } } }
#{described_class.attribute_names.join(',')}
#{build(factory, attribute => 'Foo').attributes.values.join(',')}
#{build(factory, attribute => 'FOO').attributes.values.join(',')}
CSV
it 'cancels import' do
expect { described_class.csv_import(**params) }
.not_to change(described_class, :count)
end
it 'reports the duplicate' do
expect(described_class.csv_import(**params))
.to include(result: 'failed')
end
end
Array(unique_attributes).each do |attribute|
include_examples 'case-insensitive duplicates', attribute
end
end
context 'when record creation unexpectedly fails' do
around do |example|
described_class.validate { errors.add(:base, 'Unexpected failure!') }
example.run
described_class._validate_callbacks.send(:chain).pop
described_class.send(:set_callbacks, :validate, described_class._validate_callbacks.dup)
end
context 'during a dry-run' do
before { params.merge!(try: 'true') }
it 'reports the failure' do
expect(described_class.csv_import(**params))
.to include(result: 'failed')
end
end
end
end
end

View file

@ -1,78 +0,0 @@
require 'rails_helper'
RSpec.shared_examples 'CanLookup' do
describe '::lookup' do
let(:subject) { described_class }
let(:ensure_instance) { create(subject.to_s.downcase) if subject.none? }
let(:string_attributes) { subject.lookup_keys }
let(:non_attributes) { (subject.attribute_names.map(&:to_sym) - subject.lookup_keys) }
let(:lookup_id) { 1 }
let(:cache_key) { "#{subject}::#{lookup_id}" }
let(:cache_expiry_param) { { expires_in: 7.days } }
it 'finds records by id (like ::find_by)' do
ensure_instance
expect(subject.lookup(id: lookup_id)).to eql(subject.find_by(id: lookup_id))
end
it 'finds records by other attributes (like ::find_by)' do
ensure_instance
# don't run this example on models with no valid string attributes
if string_attributes.select! { |a| subject.pluck(a).any? }
attribute_name = string_attributes.sample
attribute_value = subject.pluck(target_attribute).sample
expect(subject.lookup(attribute_name => attribute_value))
.to eql(subject.find_by(attribute_name => attribute_value))
end
end
it 'only accepts attributes that uniquely identify the record' do
expect { subject.lookup(created_by_id: 1) }
.to raise_error(ArgumentError)
end
it 'accepts exactly one attribute-value pair' do
expect { subject.lookup(id: lookup_id, some_attribute: 'foo') }
.to raise_error(ArgumentError)
end
it 'does not accept attributes not present in model' do
expect { subject.lookup(non_attributes.sample => 'foo') }
.to raise_error(ArgumentError)
end
it 'saves results to cache' do
ensure_instance
allow(Rails.cache)
.to receive(:write)
.and_call_original
subject.lookup(id: lookup_id)
expect(Rails.cache)
.to have_received(:write)
.with(cache_key, subject.first, cache_expiry_param)
end
it 'retrieves results from cache, if stored' do
ensure_instance
allow(Rails.cache)
.to receive(:read)
.and_call_original
subject.lookup(id: lookup_id)
expect(Rails.cache)
.to have_received(:read)
.with(cache_key)
end
it 'verify lookup keys' do
expect(subject.lookup_keys).to match_array((%i[id name login email number] & subject.attribute_names.map(&:to_sym)))
end
end
end

View file

@ -1,6 +1,6 @@
require 'rails_helper'
require 'models/application_model_examples'
require 'models/concerns/can_lookup_examples'
require 'models/concerns/can_csv_import_examples'
require 'models/concerns/has_history_examples'
require 'models/concerns/has_search_index_backend_examples'
require 'models/concerns/has_xss_sanitized_note_examples'
@ -8,7 +8,7 @@ require 'models/concerns/has_object_manager_attributes_validation_examples'
RSpec.describe Organization, type: :model do
it_behaves_like 'ApplicationModel', can_assets: { associations: :members }
it_behaves_like 'CanLookup'
it_behaves_like 'CanCsvImport', unique_attributes: 'name'
it_behaves_like 'HasHistory'
it_behaves_like 'HasSearchIndexBackend', indexed_factory: :organization
it_behaves_like 'HasXssSanitizedNote', model_factory: :organization

View file

@ -1,12 +1,14 @@
require 'rails_helper'
require 'models/application_model_examples'
require 'models/concerns/can_be_imported_examples'
require 'models/concerns/can_csv_import_examples'
require 'models/concerns/has_history_examples'
require 'models/concerns/has_object_manager_attributes_validation_examples'
RSpec.describe Ticket::Article, type: :model do
it_behaves_like 'ApplicationModel'
it_behaves_like 'CanBeImported'
it_behaves_like 'CanCsvImport'
it_behaves_like 'HasHistory'
it_behaves_like 'HasObjectManagerAttributesValidation'

View file

@ -1,7 +1,7 @@
require 'rails_helper'
require 'models/application_model_examples'
require 'models/concerns/can_be_imported_examples'
require 'models/concerns/can_lookup_examples'
require 'models/concerns/can_csv_import_examples'
require 'models/concerns/has_history_examples'
require 'models/concerns/has_tags_examples'
require 'models/concerns/has_xss_sanitized_note_examples'
@ -10,7 +10,7 @@ require 'models/concerns/has_object_manager_attributes_validation_examples'
RSpec.describe Ticket, type: :model do
it_behaves_like 'ApplicationModel'
it_behaves_like 'CanBeImported'
it_behaves_like 'CanLookup'
it_behaves_like 'CanCsvImport'
it_behaves_like 'HasHistory', history_relation_object: 'Ticket::Article'
it_behaves_like 'HasTags'
it_behaves_like 'HasXssSanitizedNote', model_factory: :ticket

View file

@ -6,7 +6,6 @@ require 'models/concerns/has_roles_examples'
require 'models/concerns/has_groups_permissions_examples'
require 'models/concerns/has_xss_sanitized_note_examples'
require 'models/concerns/can_be_imported_examples'
require 'models/concerns/can_lookup_examples'
require 'models/concerns/has_object_manager_attributes_validation_examples'
RSpec.describe User, type: :model do
@ -23,7 +22,6 @@ RSpec.describe User, type: :model do
it_behaves_like 'HasXssSanitizedNote', model_factory: :user
it_behaves_like 'HasGroups and Permissions', group_access_no_permission_factory: :user
it_behaves_like 'CanBeImported'
it_behaves_like 'CanLookup'
it_behaves_like 'HasObjectManagerAttributesValidation'
describe 'Class methods:' do

View file

@ -524,7 +524,7 @@ RSpec.describe 'Organization', type: :request, searchindex: true do
expect(json_response).to be_a_kind_of(Hash)
expect(json_response['try']).to eq(true)
expect(json_response['records'].count).to eq(2)
expect(json_response['records']).to be_empty
expect(json_response['result']).to eq('failed')
expect(json_response['errors'].count).to eq(2)
expect(json_response['errors'][0]).to eq("Line 1: Unable to create record - unknown attribute 'name2' for Organization.")

View file

@ -55,7 +55,7 @@ RSpec.describe 'Text Module', type: :request do
expect(json_response).to be_a_kind_of(Hash)
expect(json_response['try']).to be_truthy
expect(json_response['records'].count).to eq(2)
expect(json_response['records']).to be_empty
expect(json_response['result']).to eq('failed')
expect(json_response['errors'].count).to eq(2)
expect(json_response['errors'][0]).to eq("Line 1: Unable to create record - unknown attribute 'keywords2' for TextModule.")

View file

@ -879,7 +879,7 @@ RSpec.describe 'User', type: :request, searchindex: true do
expect(json_response).to be_a_kind_of(Hash)
expect(json_response['try']).to eq(true)
expect(json_response['records'].count).to eq(2)
expect(json_response['records']).to be_empty
expect(json_response['result']).to eq('failed')
expect(json_response['errors'].count).to eq(2)
expect(json_response['errors'][0]).to eq("Line 1: Unable to create record - unknown attribute 'firstname2' for User.")

View file

@ -101,85 +101,4 @@ class ObjectCacheTest < ActiveSupport::TestCase
assert_equal(user1.organization_ids.sort, assets[:User][user1.id]['organization_ids'].sort)
end
test 'group cache' do
name = "object cache test #{rand(9_999_999)}"
group = Group.create!(
name: name,
updated_by_id: 1,
created_by_id: 1,
)
group_new = Group.where(name: name).first
assert_equal(name, group_new[:name], 'verify by where')
# lookup by name
cache_key = group_new.name.to_s
assert_nil(Group.cache_get(cache_key))
group_lookup_name = Group.lookup(name: group_new.name)
assert_equal(group_new.name, group_lookup_name[:name], 'verify by lookup.name')
assert(Group.cache_get(cache_key))
# lookup by id
cache_key = group_new.id.to_s
assert_nil(Group.cache_get(cache_key))
group_lookup_id = Group.lookup(id: group.id)
assert_equal(group_new.name, group_lookup_id[:name], 'verify by lookup.id')
assert(Group.cache_get(cache_key))
# update / check if old name caches are deleted
name_new = name + ' next'
group.name = name_new
group.save
# lookup by name
cache_key = group.name.to_s
assert_nil(Group.cache_get(cache_key))
group_lookup = Group.where(name: group_new.name).first
assert_nil(group_lookup, 'verify by where name_old')
assert_nil(Group.cache_get(cache_key))
group_lookup = Group.where(name: group.name).first
assert_equal(name_new, group_lookup[:name], 'verify by where name_new')
assert_nil(Group.cache_get(cache_key))
group_lookup_name = Group.lookup(name: group_new.name)
assert_nil(group_lookup_name, 'verify by lookup.name name_old')
assert_nil(Group.cache_get(cache_key))
group_lookup_name = Group.lookup(name: group.name)
assert_equal(name_new, group_lookup_name[:name], 'verify by lookup.name name_new')
assert(Group.cache_get(cache_key))
# lookup by id
cache_key = group_new.id.to_s
assert_nil(Group.cache_get(cache_key))
group_lookup_id = Group.lookup(id: group.id)
assert_equal(name_new, group_lookup_id[:name], 'verify by lookup.id')
assert(Group.cache_get(cache_key))
group.destroy
# lookup by name
group_lookup = Group.where(name: group_new.name).first
assert_nil(group_lookup, 'verify by where name_old')
group_lookup = Group.where(name: group.name).first
assert_nil(group_lookup, 'verify by where name_new')
group_lookup_name = Group.lookup(name: group_new.name)
assert_nil(group_lookup_name, 'verify by lookup.name name_old')
group_lookup_name = Group.lookup(name: group.name)
assert_nil(group_lookup_name, 'verify by lookup.name name_new')
# lookup by id
group_lookup_id = Group.lookup(id: group.id)
assert_nil(group_lookup_id, 'verify by lookup.id')
end
end

View file

@ -139,12 +139,8 @@ class OrganizationCsvImportTest < ActiveSupport::TestCase
assert_nil(Organization.find_by(name: 'organization-simple-invalid_id-import1'))
organization2 = Organization.find_by(name: 'organization-simple-invalid_id-import2')
assert(organization2)
assert_equal(organization2.name, 'organization-simple-invalid_id-import2')
assert_equal(organization2.active, true)
organization2.destroy!
# any single failure will cause the entire import to be aborted
assert_nil(Organization.find_by(name: 'organization-simple-invalid_id-import2'))
end
test 'simple import with members' do

View file

@ -146,10 +146,8 @@ class TicketCsvImportTest < ActiveSupport::TestCase
assert_nil(Ticket.find_by(number: '123456'))
ticket2 = Ticket.find_by(number: '123457')
assert(ticket2)
assert_equal(ticket2.title, 'some title2')
assert_equal(ticket2.note, 'some note2')
# any single failure will cause the entire import to be aborted
assert_nil(Ticket.find_by(number: '123457'))
csv_string = "id;number;title;state;priority;owner;customer;group;note\n999999999;123456;some title1;new;2 normal;-;nicole.braun@zammad.org;Users;some note1\n;123457;some title22;closed;1 low;admin@example.com;nicole.braun@zammad.org;Users;some note22\n"
@ -166,12 +164,8 @@ class TicketCsvImportTest < ActiveSupport::TestCase
assert_nil(Ticket.find_by(number: '123456'))
ticket2 = Ticket.find_by(number: '123457')
assert(ticket2)
assert_equal(ticket2.title, 'some title22')
assert_equal(ticket2.note, 'some note22')
ticket2.destroy!
# any single failure will cause the entire import to be aborted
assert_nil(Ticket.find_by(number: '123457'))
end
test 'invalid attributes' do

View file

@ -213,15 +213,8 @@ class UserCsvImportTest < ActiveSupport::TestCase
assert_nil(User.find_by(login: 'user-simple-invalid_id-import1'))
user2 = User.find_by(login: 'user-simple-invalid_id-import2')
assert(user2)
assert_equal(user2.login, 'user-simple-invalid_id-import2')
assert_equal(user2.firstname, 'firstname-simple-import2')
assert_equal(user2.lastname, 'lastname-simple-import2')
assert_equal(user2.email, 'user-simple-invalid_id-import2@example.com')
assert_equal(user2.active, false)
user2.destroy!
# any single failure will cause the entire import to be aborted
assert_nil(User.find_by(login: 'user-simple-invalid_id-import2'))
end
test 'simple import with read only id' do
@ -255,15 +248,8 @@ class UserCsvImportTest < ActiveSupport::TestCase
assert_nil(User.find_by(login: 'user-simple-readonly_id-import1'))
user2 = User.find_by(login: 'user-simple-readonly_id-import2')
assert(user2)
assert_equal(user2.login, 'user-simple-readonly_id-import2')
assert_equal(user2.firstname, 'firstname-simple-import2')
assert_equal(user2.lastname, 'lastname-simple-import2')
assert_equal(user2.email, 'user-simple-readonly_id-import2@example.com')
assert_equal(user2.active, false)
user2.destroy!
# any single failure will cause the entire import to be aborted
assert_nil(User.find_by(login: 'user-simple-readonly_id-import2'))
end
test 'simple import with roles' do
@ -392,8 +378,8 @@ class UserCsvImportTest < ActiveSupport::TestCase
try: true,
)
assert_equal(true, result[:try])
assert_equal(3, result[:records].count)
assert_equal('success', result[:result])
assert_equal(2, result[:records].count)
assert_equal('failed', result[:result])
assert_nil(User.find_by(login: 'user-duplicate-import1'))
assert_nil(User.find_by(login: 'user-duplicate-import2'))
@ -407,15 +393,12 @@ class UserCsvImportTest < ActiveSupport::TestCase
try: false,
)
assert_equal(false, result[:try])
assert_equal(3, result[:records].count)
assert_equal('success', result[:result])
assert_equal(2, result[:records].count)
assert_equal('failed', result[:result])
assert(User.find_by(login: 'user-duplicate-import1'))
assert(User.find_by(login: 'user-duplicate-import2'))
assert_nil(User.find_by(login: 'user-duplicate-import1'))
assert_nil(User.find_by(login: 'user-duplicate-import2'))
assert_nil(User.find_by(login: 'user-duplicate-import3'))
User.find_by(login: 'user-duplicate-import1').destroy!
User.find_by(login: 'user-duplicate-import2').destroy!
end
test 'invalid attributes' do
@ -486,7 +469,7 @@ class UserCsvImportTest < ActiveSupport::TestCase
assert_nil(User.find_by(login: 'user-reference-import1'))
assert_nil(User.find_by(login: 'user-reference-import2'))
assert(User.find_by(login: 'user-reference-import3'))
assert_nil(User.find_by(login: 'user-reference-import3'))
assert_equal("Line 1: No lookup value found for 'organization': \"organization-reference-import1\"", result[:errors][0])
assert_equal("Line 2: No lookup value found for 'organization': \"organization-reference-import2\"", result[:errors][1])
@ -506,7 +489,9 @@ class UserCsvImportTest < ActiveSupport::TestCase
assert_equal('success', result[:result])
assert_nil(User.find_by(login: 'user-reference-import1'))
assert_nil(User.find_by(login: 'user-reference-import2'))
assert(User.find_by(login: 'user-reference-import3'))
# any single failure will cause the entire import to be aborted
assert_nil(User.find_by(login: 'user-reference-import3'))
result = User.csv_import(
string: csv_string,