Rails ignoring validations when persisting

Question

I have an issue where my City model is creating several duplicates and somehow ignoring my validations.

My controller is instantiating a Family record and assigning a city as follows:

f = Family.new 
f.assign_city('Seattle', 'Washington') # put in literals as examples

Then, in my Family model, I have the assign_city method:

def assign_city(city_name, state_name)
  raise(GeoException, 'Both city and state names must be present') 
  unless city_name.present? && state_name.present?
  existing_city = City.query_by_name_and_state(city_name, 
    state_name).first
  if existing_city
    self.city = existing_city
  else
    self.city = City.create! name: city_name, state: state_name, 
      country: 'USA', description: ''
  end
end 

My City model has the following entry:

validates :name, presence: true, uniqueness: {scope: :state}
validates :state, presence: true, numericality: false

scope :query_by_name_and_state, (->(city, state) {
  if city.present? && state.present?
    where('LOWER(name) LIKE ? AND LOWER(state) LIKE ?', city.downcase, 
     state.downcase)
  else
    where '' # to prevent exception to be raised if city, state are nil
  end
})

I'm perplexed for two reasons:

1. The program should not have even entered the ELSE flow to create a new city, because there was already a city with the same name and state.
2. After entering the ELSE flow, it should not have succeeded in creating a record, as this is NOT allowed from the validations, as you can see.

That said, I somehow have 17 duplicate cities with the same name and state!

PS- If it also helps, I did a City.where('Seattle', 'Washington').map(&:is_valid?) and get an array of [false, false, false,...]

Any advice would be appreciated. Thanks!

Answer 1

You don't need to do this conditional logic, you can use find_or_create_by

def assign_city(city_name, state_name)
  raise(GeoException, 'Both city and state names must be present') unless city_name.present? && state_name.present?

  self.city = City.find_or_create_by name: city_name, state: state_name, country: 'USA', description: ''
end 

It's hard to know why you're not getting model validation errors. I suspect it's because you're operating on an unpersisted instance of Family. But if you wanted to be safe, you would do city.valid? in your conditional logic. But the best practice is to include db level validation if you want to insure data integrity as there are ways to bypass / override model validations in rails. Your migration might look like this:

class ChangeCity < ActiveRecord::Migration
  def change
     add_index :cities, [:city_name, :state_name],  unique: true
  end
end

Having this will raise db level errors which don't rely on model validation. You'll also need to remove dupes first or this migration will fail.

UPDATE: I tested your City scope method and found it may not be very reliable as you can see here if we pass empty strings for city and stage look what happens:

simple_soundcloud_app(main)> existing_city = City.query_by_name_and_state('', '').first
  City Load (0.1ms)  SELECT  "cities".* FROM "cities" ORDER BY "cities"."id" ASC LIMIT ?  [["LIMIT", 1]]
=> #<City:0x007f84790c2308
 id: 1,
 name: "New York",
 state: "NY",
 created_at: Sun, 01 Apr 2018 22:58:35 UTC +00:00,
 updated_at: Sun, 01 Apr 2018 22:58:35 UTC +00:00>

This doesn't answer the question as to why your validation didn't fire as expected However we can't be sure of the input data you used or how the duplicates got created in the first place. I would recommend writing unit tests for all your methods and test all edge cases.

UPDATE 2, let's fix your scope method as there are so many things wrong with it, especially the hack as illustrated above. Your scope expects arguments so don't call it without the as that is anti-pattern. You should probably be using a gem like https://github.com/loureirorg/city-state or something. But if you're going to manage the data, normalize it to downcase everything. Then this will work:

class City < ApplicationRecord
  has_many :families
  validates :name, presence: true, uniqueness: {scope: :state}
  validates :state, presence: true, numericality: false

  scope :query_by_name_and_state, -> (city, state) {
      where(name: city.downcase, state: state.downcase)
  }

  before_save :normalize_data

  def normalize_data
    self.name.downcase!
    self.state.downcase!
  end
end


simple_soundcloud_app(main)> existing_city = City.query_by_name_and_state('Boston', 'MA').first
  City Load (0.1ms)  SELECT  "cities".* FROM "cities" WHERE "cities"."name" = ? AND "cities"."state" = ? ORDER BY "cities"."id" ASC LIMIT ?  [["name", "boston"], ["state", "ma"], ["LIMIT", 1]]
=> #<City:0x007fce6a34e5c8
 id: 3,
 name: "boston",
 state: "ma",
 created_at: Sun, 08 Apr 2018 01:29:43 UTC +00:00,
 updated_at: Sun, 08 Apr 2018 01:29:43 UTC +00:00>
 simple_soundcloud_app(main)> existing_city = City.query_by_name_and_state('', '').first
  City Load (0.1ms)  SELECT  "cities".* FROM "cities" WHERE "cities"."name" = ? AND "cities"."state" = ? ORDER BY "cities"."id" ASC LIMIT ?  [["name", ""], ["state", ""], ["LIMIT", 1]]
=> nil