Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #10479 - prevent validation error on host creation when domain … #2374

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/domain.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class Domain < ActiveRecord::Base

accepts_nested_attributes_for :domain_parameters, :allow_destroy => true
include ParameterValidators
validates :name, :presence => true, :uniqueness => true
validates :name, :presence => true, :uniqueness => { :case_sensitive => false }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be enforced with a DB index of some sort too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a case-insensitive unique index for this.
I will also attach to this PR another commit making host names case insensitive, feel free to return this PR to waiting on contributor.

validates :fullname, :uniqueness => true, :allow_blank => true, :allow_nil => true
validates :dns, :proxy_features => { :feature => "DNS", :message => N_('does not have the DNS feature') }

Expand Down
2 changes: 1 addition & 1 deletion app/models/nic/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ def name=(*args)
end

def shortname
domain.nil? ? name : name.to_s.chomp("." + domain.name)
domain.nil? ? name : name.to_s.sub(/\.#{Regexp.escape domain.name}\Z/i, '')
end

def validated?
Expand Down
8 changes: 4 additions & 4 deletions app/models/nic/interface.rb
Original file line number Diff line number Diff line change
Expand Up @@ -93,19 +93,19 @@ def normalize_name
return if name.empty?
if domain.nil? && name.include?('.') && !changed_attributes['domain_id'].present?
# try to assign the domain automatically based on our existing domains from the host FQDN
self.domain = Domain.find_by(:name => name.partition('.')[2])
self.domain = Domain.where('LOWER(name) = ?', name.partition('.')[2].downcase).first
elsif persisted? && changed_attributes['domain_id'].present?
# if we've just updated the domain name, strip off the old one
old_domain = Domain.find(changed_attributes["domain_id"])
old_domain = Domain.where(:id => changed_attributes["domain_id"]).pluck(:name).first
# Remove the old domain, until fqdn will be set as the full name
self.name.chomp!("." + old_domain.to_s)
self.name.sub!(/\.#{Regexp.escape old_domain}/i, '')
end
# name should be fqdn
self.name = fqdn
# A managed host we should know the domain for; and the shortname shouldn't include a period
# This only applies for unattended=true, as otherwise the name field includes the domain
errors.add(:name, _("must not include periods")) if ( host && host.managed? && managed? && shortname.include?(".") && SETTINGS[:unattended] )
self.name = Net::Validations.normalize_hostname(name) if self.name.present?
self.name = Net::Validations.normalize_hostname(name)
end
end
end
Expand Down
16 changes: 16 additions & 0 deletions db/migrate/20150921090740_add_index_to_domain_name.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class AddIndexToDomainName < ActiveRecord::Migration
def up
case ActiveRecord::Base.connection.instance_values["config"][:adapter]
when "postgresql"
execute 'CREATE UNIQUE INDEX domain_name_index ON domains (lower(name))'
when "sqlite3"
execute 'CREATE UNIQUE INDEX domain_name_index ON domains (name collate nocase)'
else
add_index :domains, :name, :name => 'domain_name_index', :unique => true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this relies on MySQL unique indexes being case sensitive by default, please leave a comment

end
end

def down
remove_index! :domains, 'domain_name_index'
end
end
8 changes: 8 additions & 0 deletions test/unit/domain_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ def setup
should validate_uniqueness_of(:fullname).allow_nil
should validate_uniqueness_of(:fullname).allow_blank

test "should have a unique case-insensitive name" do
other_domain = Domain.new(:name => @domain.name.upcase)
refute other_domain.save
assert_raises ActiveRecord::RecordNotUnique, ActiveRecord::StatementInvalid do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the index is only enforcing this on MySQL, Postgres and sqlite won't raise this exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The weirdest part is that if I remove the migration and add the index manually running CREATE UNIQUE INDEX domain_name_index ON domains (name collate nocase), this test pass. The migration is certainly running for pg and sqlite as you can see on Jenkins console.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failure is actually because rake test runs rake db:test:prepare which loads the test db schema from schema.rb. However, the SQL commands I created for sqlite and pgsql aren't preserved in schema.rb as they aren't adapter agnostic.
If you run only this test after running migrations on the test db it passes.
I'm actually a bit stumped as to what would be the best approach here:

  • remove this test knowing that it works and the problem is in the way rake loads the test DB schema.
  • remove the case-insensitive enforcement from the DB altogether and rely on validations only
  • create a duplicate column that saves the lowercase (similar to what we have for user names), causing this to work but introducing data duplication and increasing complexity for something the DB should do on its own.
  • create something like foreigner that allows case-insensitive indices in schema.rb and migrations (most likely the most complex yet most reusable solution)

I'd be happy to get your input on what would be the best approach, as I will need to do the same for the next commit addressing the host names as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dLobatog ping, trying to revive this old PR. Could I get you input about the previous comment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the case-insensitive enforcement from the database isn't the way to go as you can encounter this situation when 2 people submit a domain to be created or updated.

1. USER1 - Query the database to see if this domain already exists.
2. USER2 - Query the database to see if this domain already exists.
3. USER1 - OK, doesn't exist, let's insert the record.
4. USER2 - OK, doesn't exist, let's insert the record.

Removing the test isn't good either IMO. We want to verify this behavior. I'm not sure right now how to move forward, I'm checking why exactly this happens, even when not loading the test via rake (ruby -I'test' test/unit/domain_test.rb)

other_domain.save(:validate => false) #make sure the DB enforces this
end
end

test "when cast to string should return the name" do
s = @domain.to_s
assert_equal @domain.name, s
Expand Down
8 changes: 8 additions & 0 deletions test/unit/nics/managed_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@ class ManagedTest < ActiveSupport::TestCase
assert_nil nic.domain
end

test "#normalize_hostname accepts domain even if it contains capital letters" do
domain = FactoryGirl.create(:domain, :name => "CAPITAL.com")
nic = setup_primary_nic_with_name(" Host.#{domain.name}", :domain => nil)
nic.send(:normalize_name)
assert_equal "host.#{domain.name.downcase}", nic.name
assert_equal domain, nic.domain
end

test "#normalize_hostname keeps domain nil if it can't find such domain based on name" do
nic = setup_primary_nic_with_name(" Host", :domain => nil)
nic.send(:normalize_name)
Expand Down