A Supposedly Terminating Loop I'll Never Write Again

| 2 Comments

I haven't written an infinite loop bug in a while. I suppose I was due. (The Greek gods always punish hubris.)

I have a database table full of users. Let's call that table user. A user has one or more roles (from the role) table. A user can be an unverified user, a free user, a subscriber, or an administrator. (Administrators obviously are also subscribers, but you get the point.)

I use DBIx::Class for the database access layer. My User result class has a couple of methods which let other model actions query whether the user can perform specific operations. They're synthetic attributes:

has [qw( is_admin is_subscriber )], is => 'ro', lazy_build => 1;

sub _build_is_admin      { shift->find_role_by_name( 'admin'      ) }
sub _build_is_subscriber { shift->find_role_by_name( 'subscriber' ) }

Obviously the implementation of find_role_by_name() is supremely important. Here's how I originally wrote the code (no sense blaming anyone else for this, because I wrote all of the access control code myself.) See if you can catch the bug:

sub find_role_by_name
{
    my ($self, $name) = @_;

    while (my $role = $self->user_roles->next)
    {
        return 1 if $role->role->name eq $name;
    }

    return 0;
}

As a hint, the user_roles method represents the relationship between the user table and the role table. That method returns a DBIC resultset which represents the entries in the table with a foreign key to the user table.

See the bug yet? It took me a while. The problem is that every invocation of the loop fetches a new resultset. Every invocation of the loop fetches only the first result from that resultset.

My tests (yeah, I wrote the tests too, before I wrote this code in fact, just like you're supposed to do) didn't catch that because the rest of the code only ever checked for one user role, the administrator role, anywhere else. Only when I added code for the subscriber role did the subscription tests trigger this infinite loop. (Now how abhorred in my imagination is that most excellent jest!)

I wish I could say that I diagnosed the cause immediately. I didn't. I stared at the code for far too long, until I had the presence of mind to put a debugging statement in the loop body to print the name of the role. (Stat officium pristina nomine.)

Here's the corrected code, with a single expression hoisted into a scalar variable where it may endure properly:

sub find_role_by_name
{
    my ($self, $name) = @_;
    my $user_roles_rs = $self->user_roles;

    while (my $role = $user_roles_rs->next)
    {
        return 1 if $role->role->name eq $name;
    }

    return 0;
}

I will say this: expecting your test suite to pass in under 30 seconds makes this sort of thing much easier to catch. If I'd had to wait for a continuous integration server to get around to checking this commit, I wouldn't have fixed it as quickly as I did.

Then again, it's easier to debug bugs you never write in the first place.

Oh, and if you catch the literary allusion in the title, here's your prize: congratulations! You might be overeducated. (If you caught the other three allusions, have you ever noticed how much Pynchon's V influenced Stephenson's Cryptonomicon?)

2 Comments

Well, if you didn't have a loop in the first place, then there would be no risk of going into an infinite loop :-)

Apparently one could check user roles through a single request to the database, something like

my $has_role = $self->user_roles(-where => {name => $name});
return 1 if @$has_role.

(sorry, this is DBIx::DataModel syntax, I don't remember exactly how to write it in DBIC syntax, but you get the point).

Or am I missing something ?

I considered but didn't write that code for some reason I don't remember at the moment. It may not have been a good reason.

With two types of role checks (admin and subscriber), it's even less of a good reason.

Modern Perl: The Book

cover image for Modern Perl: the book

The best Perl Programmers read Modern Perl: The Book.

sponsored by the How to Make a Smoothie guide

Categories

Pages

About this Entry

This page contains a single entry by chromatic published on September 7, 2012 6:00 AM.

The Advantages of Declarative Exporting was the previous entry in this blog.

Features Perl 5 Needs in 2012 is the next entry in this blog.

Find recent content on the main index or look in the archives to find all content.


Powered by the Perl programming language

what is programming?