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

tbrisker · 2015-05-12T08:30:37Z

…contains capital letters

domcleal · 2015-05-12T08:35:03Z

This needs unit tests.

domcleal · 2015-05-20T14:04:04Z

There are a lot of bugs filed against our handling of mixed case hostnames, particularly that we downcase them unnecessarily, which causes more issues. I don't believe increasing the amount of downcasing is a very good idea here, but rather we should perhaps fix the underlying bug which is probably that we're not treating the domain name as case insensitive?

domcleal · 2015-05-29T15:05:36Z

I'm not sure I understand why this level of support was chosen in the latest update, could you comment please?

The expectation from the test, and when creating hosts is that the name is downcased.. I guess that meets the very basic requirement that there's no validation error, but if you've specified an upper case domain, then should it be stored and used that way? It seems incomplete to allow someone to enter an upper case domain, then ignore it.

The alternative is to block entering upper case domains, but as I mentioned earlier, I don't think decreasing support for mixed or upper case domains is the right direction.

http://projects.theforeman.org/issues/8859 and http://projects.theforeman.org/issues/8754 are relevant.

tbrisker · 2015-06-03T11:52:43Z

@domcleal The idea behind the change is to make domains case-insensitive while keeping them case-aware, so that applications which are case-sensitive will be able to get the correct domain name (as in the related issues you mentioned). Note that the domain name maintains it's case, only the host name ignores it and downcases everything.
I am not sure why we force downcase host names at all. I would suggest we also do this for host names, solving those issues, and downcase only if a certain use-case requires the name in lower case (or better, make any comparisons case-insensitive if possible). However that is outside the scope of this PR.

ohadlevy · 2015-06-09T18:39:03Z

@tbrisker @domcleal ping?

domcleal · 2015-07-24T12:08:30Z

so that applications which are case-sensitive will be able to get the correct domain name

This is the right direction, but I don't see much use for it, since 98% of domain name consumption is via the host name.

Note that the domain name maintains it's case, only the host name ignores it and downcases everything.
I am not sure why we force downcase host names at all. I would suggest we also do this for host names, solving those issues, and downcase only if a certain use-case requires the name in lower case (or better, make any comparisons case-insensitive if possible). However that is outside the scope of this PR.

Right, this is what I'm getting at. Perhaps you could consider fixing those issues as further commits to this PR?

I don't get the impression that they will be fixed otherwise, which leaves us with a commit that doesn't really fix the issues that matter... just closes a trivial ticket.

domcleal · 2015-07-24T12:08:44Z

app/models/domain.rb

@@ -26,7 +26,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 }


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

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.

dLobatog · 2015-09-24T11:45:08Z

test/unit/domain_test.rb

+  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


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

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.

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.

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

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)

dLobatog · 2015-09-24T12:03:49Z

@tbrisker 👍 with me, except for the failure on pg and sqlite, if you can fix that that'd be great. Seems like the migration is not running properly or something. Thanks!

dLobatog · 2016-04-21T11:12:47Z

db/migrate/20150921090740_add_index_to_domain_name.rb

+    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


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

dLobatog · 2016-06-30T14:25:12Z

@tbrisker This was nearly ready, mind to rebase & update the branch? Thanks!

tbrisker · 2016-09-25T07:51:27Z

Closing this for now. may reopen in the future.

theforeman-bot added Not yet reviewed Needs testing labels May 12, 2015

domcleal added Waiting on contributor and removed Not yet reviewed labels May 12, 2015

tbrisker force-pushed the 10479 branch from ca09d91 to 9aeb64f Compare May 19, 2015 13:36

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels May 19, 2015

dLobatog added Waiting on contributor and removed Needs re-review Needs testing labels May 20, 2015

tbrisker force-pushed the 10479 branch from 9aeb64f to dda16f2 Compare May 21, 2015 13:58

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels May 21, 2015

tbrisker force-pushed the 10479 branch 3 times, most recently from 7cd42dd to b817a9e Compare May 21, 2015 14:48

domcleal added Waiting on contributor and removed Needs re-review labels May 29, 2015

domcleal added Needs re-review and removed Waiting on contributor labels Jul 22, 2015

domcleal reviewed Jul 24, 2015
View reviewed changes

domcleal added the Waiting on contributor label Jul 24, 2015

domcleal removed the Needs re-review label Jul 24, 2015

tbrisker force-pushed the 10479 branch from b817a9e to 311db83 Compare September 21, 2015 17:34

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Sep 21, 2015

domcleal added Waiting on contributor and removed Needs re-review labels Sep 22, 2015

tbrisker force-pushed the 10479 branch from 311db83 to aeb05fb Compare September 22, 2015 07:58

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Sep 22, 2015

tbrisker force-pushed the 10479 branch from aeb05fb to e54d772 Compare September 22, 2015 10:41

domcleal added Waiting on contributor and removed Needs re-review labels Sep 23, 2015

dLobatog reviewed Sep 24, 2015
View reviewed changes

dLobatog removed the Needs testing label Sep 24, 2015

Fixes #10479 - make domain names case-insensitive

9d7fa12

tbrisker force-pushed the 10479 branch from e54d772 to 9d7fa12 Compare March 1, 2016 09:03

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Mar 1, 2016

dLobatog reviewed Apr 21, 2016
View reviewed changes

dLobatog added Waiting on contributor and removed Needs re-review Needs testing labels Jun 30, 2016

tbrisker closed this Sep 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tbrisker commented May 12, 2015

domcleal commented May 12, 2015

domcleal commented May 20, 2015

domcleal commented May 29, 2015

tbrisker commented Jun 3, 2015

ohadlevy commented Jun 9, 2015

domcleal commented Jul 24, 2015

domcleal Jul 24, 2015

tbrisker Sep 21, 2015

dLobatog Sep 24, 2015

dLobatog Sep 24, 2015

tbrisker Sep 24, 2015

tbrisker Apr 20, 2016

dLobatog Apr 21, 2016

dLobatog commented Sep 24, 2015

dLobatog Apr 21, 2016

dLobatog commented Jun 30, 2016

tbrisker commented Sep 25, 2016

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

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

Conversation

tbrisker commented May 12, 2015

domcleal commented May 12, 2015

domcleal commented May 20, 2015

domcleal commented May 29, 2015

tbrisker commented Jun 3, 2015

ohadlevy commented Jun 9, 2015

domcleal commented Jul 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dLobatog commented Sep 24, 2015

Choose a reason for hiding this comment

dLobatog commented Jun 30, 2016

tbrisker commented Sep 25, 2016