frog and code

Any developer who's had to deal with Date/Time calculations knows there are myriad issues to navigate, particularly around time zone and Daylight Saving handling. Today (Mon 2008.11.03), I ran our tests and stumbled upon the following code (simplified for this post) that was breaking:

# relapse_test.rb
def test_middle_time
  t = Time.now
  r = Relapse.new(:start_time => t-5.days, :end_time => t-1.day)
  middle = t-3.days
  assert_equal middle, r.middle_time # Breaking on Mon 2008.11.03
end

# Relapse class in relapse.rb
def middle_time
  return start_time + (end_time - start_time)/2
end

Results of the test:

<Fri Oct 31 11:27:18 -0400 2008> expected but was
<Fri Oct 31 11:57:18 -0400 2008>.

The purpose of middle_time() is to return the time halfway between the start and end times. The code seemed straightforward, but for some reason, the expected and returned values differed by 30 minutes.

As I stared at the code, there was another nagging question - why was this happening now when it wasn't prior, before the weekend. Hmmm ... a Halloween trick? To get more insight, I added some print statements to echo the times used by the middle_time calculation and got the following:

Start: Wed Oct 29 11:27:18 -0400 2008
  End: Sun Nov 02 11:27:18 -0500 2008

Bingo. Check out the time zone offset: -0400 for start time and -0500 for end. What happened over the weekend? Halloween in Salem was a good guess but the culprit was the switch from Daylight Saving back to Standard Time. Because of the time zone disparity, there is an extra hour between the start time, taking place in Daylight Saving time, and the end time in Standard Time. So (end_time - start time) in middle_time() adds an extra hour (4 days plus one hour) and, when divided by 2, produces the 30 minute discrepancy between expected and actual time.

Proper handling depends on your situation. In ours, we were interested in the middle DATE rather than the time, so the implementation was changed to use dates v. times. For situations where time does matter, the 30 minute discrepancy may, in fact, be desired as it factors in the number of hours that have elapsed. But if you want the calculation to be time zone-consistent (or agnostic), you can use the UTC (GMT) time zone; Time.now.utc returns the current UTC time. Calculations can be performed in UTC, which does not shift for Daylight Saving (or any other reason), and converted back to a zone-specific time as needed.

The popular Rails search plugin acts_as_ferret has had, until recently, a nasty little bug affecting boolean attributes. Indexed attributes returning a value of false are indexed as *nil*, not false as expected. (True values are indexed correctly.) Thus searches that test whether an attribute is false omit matching records and, conversely and often more frighteningly, include records that should NOT match.

The bug resides in the following code in instance_methods.rb (from version 4.3, the latest stable release at this time):

def content_for_field_name(field, dynamic_boost = nil)
  ar_field = aaf_configuration[:ferret_fields][field][:via]
  field_data = self.send(ar_field) || self.instance_variable_get("@#{ar_field}")
  if (dynamic_boost && boost_value = self.send(dynamic_boost))
    field_data = Ferret::Field.new(field_data)
    field_data.boost = boost_value.to_i
  end
  field_data
end

On line three, the first half of the conditional expression

self.send(ar_field)

returns false if the indexed field is a method returning false. But instead assigning false to field_data (which is subsequently indexed), the second half of the condition

self.instance_variable_get("@#{ar_field}")

is run and checks for an instance variable with that field name. If none exists (typically the case for Rails attributes derived from database columns), the expression returns nil. So field_data is assigned nil, not false, and indexed as such.

Why is this a big deal? Boolean attributes such as 'deleted', 'activated', 'public' and 'suspended' are often used by applications to determine whether and when a user may participate and be visible. If these fields are indexed improperly, legitimate results can be dropped from searches and illegitimate users shown. (And if application logic relies on the attributes returned, further security issues can arise.) For applications that maintain sensitive user data, exposing a user as public who has asked to be private is a scary proposition.

About a month ago, I submitted a bug report http://projects.jkraemer.net/acts_as_ferret/ticket/220 with the following patch for line 3 above:

# Issue was that field_data was being set to nil for fields with value=false.
begin
  field_data = self.send(ar_field)
rescue
  field_data = self.instance_variable_get("@#{ar_field}")
end

Was happy to see that a fix has been recently committed on the trunk http://projects.jkraemer.net/acts_as_ferret/changeset/350 to address the issue. The following code was used to replace line 3:

field_data = (respond_to?(via) ? send(via) : instance_variable_get("@#{via}")).to_s

The respond_to? call tests for the presence of a method and skips the instance_variable_get if one is found. This is definitely slicker and more idiomatic ruby than the begin/rescue approach in my code (I'll conveniently blame years of Java for the exceptional approach). I'm happy to see this fix - thank you, Jens! - and encourage you to upgrade acts_as_ferret or patch your apps as soon as possible. You'll also need to re-index models with boolean fields if you've already been burned by the bug.

One of boons of using Ruby is its expressiveness and readability. A small detail that struck me when I started writing Ruby code was the ability to use words like and and or as keywords in conditional expressions (in lieu of && and ||). A nice touch that allows code to read more like English.

However, it turns out that the English forms of these conditional operators are not quite equivalent to their symbol counterparts. Consider the following exercise on the console:

>> username = 'frog'
>> password = nil
>> login_valid = !username.blank? && !password.blank?
>> puts login_valid
false

Login is valid if both username and password are not blank. Since password is nil, the value of login_valid is false as we would expect. Now let's try the same code, replacing && with and to represent the AND conditional operator.

>> username = 'frog'
>> password = nil
>> login_valid = !username.blank? and !password.blank?
>> puts login_valid
true

Whoa!? The login_valid variable now evaluates to true. What happened? Well, and is not simply an alias for &&, it's a different operator with a different level of precedence. Programming Ruby by Dave Thomas contains a table of operators in order of precedence and notes that "the word forms of these operators (and, or, and not) have a lower precedence than the corresponding symbol forms (&&, ||, and !)." More specifically, the word forms are lower in precedence than assignment operators (= in the example above), while the symbol forms are higher in precedence.

So in the second example, the variable login_valid is assigned the value of !username.blank? before the and condition !password.blank? is evaluated. Crazy, and a little scary - I'd expect both forms to work the same way, or at least both to have precedence over assignment operators. The solution in this case is either to use && as in the first example or wrap the full condition in parentheses to specify precedence explicitly:

>> username = 'frog'
>> password = nil
>> login_valid = (!username.blank? and !password.blank?)
>> puts login_valid
false

Rails 2.1 was released last week at RailsConf with a lot fanfare and sexy features like timestamped migrations, named scopes and better timezone handling. There was also a minor fix that I was happy to see: String#titleize now works with acronyms. Prior to 2.1:

>> "ALS community".titleize
=> "Als Community"

Not terribly desirable that "ALS" suddenly has lowercase letters because it's used in a title. But now in 2.1:

>> "ALS community".titleize
=> "ALS Community"

Ah, much better. I had patched String in my projects to correct this. Now the band-aid can come off. Interestingly, it looks like this bug was not reported (shame on me); instead the fix was made in response to an issue where the character following an apostrophe was being incorrectly capitalized:

>> "frog's code".titleize
=> "Frog'S Code"

The change to address this case also solves the acronym problem. Details of the fix can be found here:

http://dev.rubyonrails.org/changeset/8533