organized flames

Search

Refactoring a simple function in BIND 9: zone_settimer()

Posted on June 04, 2010 by Michael

Just like a garage, code needs an occasional cleanup. Without spending some time going back over existing code to make it better, shorter, and better tested, it will be filled with spiders, too. I mean bugs, my garage has the spiders.

As a practical example, I’ve taken a function in BIND 9 and rewritten it incrementally to be more testable, and I think easier to understand. This function, as we will learn later, sets a timer inside the zone to expire at the time of the next zone event. What the various events mean is beyond the scope of this example, but it’s unimportant also.

While the changes are incremental, I will include the full listing in each iteration. This will make this page unbelievably large, but it will make it easier to see the steps as we follow along.

Here’s the code as it exists in BIND 9.7.0’s version of lib/dns/zone.c:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121

static void
zone_settimer(dns_zone_t *zone, isc_time_t *now) {
  const char me[] = "zone_settimer";
  isc_time_t next;
  isc_result_t result;

  ENTER;
  REQUIRE(DNS_ZONE_VALID(zone));
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
    return;

  isc_time_settoepoch(&next);

  switch (zone->type) {
  case dns_zone_master:
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
      next = zone->notifytime;
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->dumptime, &next) < 0)
        next = zone->dumptime;
    }
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING) &&
        !isc_time_isepoch(&zone->refreshkeytime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->refreshkeytime, &next) < 0)
        next = zone->refreshkeytime;
    }
    if (!isc_time_isepoch(&zone->resigntime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->resigntime, &next) < 0)
        next = zone->resigntime;
    }
    if (!isc_time_isepoch(&zone->keywarntime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->keywarntime, &next) < 0)
        next = zone->keywarntime;
    }
    if (!isc_time_isepoch(&zone->signingtime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->signingtime, &next) < 0)
        next = zone->signingtime;
    }
    if (!isc_time_isepoch(&zone->nsec3chaintime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->nsec3chaintime, &next) < 0)
        next = zone->nsec3chaintime;
    }
    break;

  case dns_zone_slave:
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
      next = zone->notifytime;
    /*FALLTHROUGH*/

  case dns_zone_stub:
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING)) {
      INSIST(!isc_time_isepoch(&zone->refreshtime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->refreshtime, &next) < 0)
        next = zone->refreshtime;
    }
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
      INSIST(!isc_time_isepoch(&zone->expiretime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->expiretime, &next) < 0)
        next = zone->expiretime;
    }
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->dumptime, &next) < 0)
        next = zone->dumptime;
    }
    break;

  case dns_zone_key:
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->dumptime, &next) < 0)
        next = zone->dumptime;
    }
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING)) {
      if (isc_time_isepoch(&next) ||
          (!isc_time_isepoch(&zone->refreshkeytime) &&
          isc_time_compare(&zone->refreshkeytime, &next) < 0))
        next = zone->refreshkeytime;
    }
    break;

  default:
    break;
  }

  if (isc_time_isepoch(&next)) {
    zone_debuglog(zone, me, 10, "settimer inactive");
    result = isc_timer_reset(zone->timer, isc_timertype_inactive,
            NULL, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not deactivate zone timer: %s",
             isc_result_totext(result));
  } else {
    if (isc_time_compare(&next, now) <= 0)
      next = *now;
    result = isc_timer_reset(zone->timer, isc_timertype_once,
           &next, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not reset zone timer: %s",
             isc_result_totext(result));
  }
}

Document

At a first glance, I notice two things. One, there is no description of this function, so I have to read the code to really get what it does. Two, there is a lot of duplication. Let’s fix the documentation part first. I’m something of a C expert and I know well what things like DNSZONEFLGNEEDDUMP means, but if I did not, I would spend quite a bit of time here.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157

/*
 * A zone has one timer, but has events that occur at various times.
 * Go through the various events as needed for this zone type, and find
 * the one that will trigger next.  Set the zone's timer to this event's
 * time.
 *
 * If no events are scheduled to trigger, stop the timer.
 */
static void
zone_settimer(dns_zone_t *zone, isc_time_t *now) {
  const char me[] = "zone_settimer";
  isc_time_t next;
  isc_result_t result;

  ENTER;
  REQUIRE(DNS_ZONE_VALID(zone));
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
    return;

  isc_time_settoepoch(&next);

  switch (zone->type) {
  case dns_zone_master:
    /*
     * If we need to send a notify, set the timer.
     */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
      next = zone->notifytime;

    /* Do we need to dump to disk, and are not doing so? */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->dumptime, &next) < 0)
        next = zone->dumptime;
    }

    /* Do we need to refresh the signatures? */
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING) &&
        !isc_time_isepoch(&zone->refreshkeytime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->refreshkeytime, &next) < 0)
        next = zone->refreshkeytime;
    }
    
    /* Do we need to re-sign any data? */
    if (!isc_time_isepoch(&zone->resigntime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->resigntime, &next) < 0)
        next = zone->resigntime;
    }
    
    /* Do we need to warn about expiring keys? */
    if (!isc_time_isepoch(&zone->keywarntime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->keywarntime, &next) < 0)
        next = zone->keywarntime;
    }
    
    /* Do we need to sign the zone? */
    if (!isc_time_isepoch(&zone->signingtime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->signingtime, &next) < 0)
        next = zone->signingtime;
    }
    
    /* Do we need build or rebuild nsec3 chains? */
    if (!isc_time_isepoch(&zone->nsec3chaintime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->nsec3chaintime, &next) < 0)
        next = zone->nsec3chaintime;
    }
    break;

  case dns_zone_slave:
    /* Do we need to send notifies? */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
      next = zone->notifytime;
    /*FALLTHROUGH*/

  case dns_zone_stub:
    /*
     * If we are not refreshing and can, we have masters,
     * and we are not loading then check if we should do
     * do that next.
     */
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING)) {
      INSIST(!isc_time_isepoch(&zone->refreshtime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->refreshtime, &next) < 0)
        next = zone->refreshtime;
    }
    
    /* Check expire time if we are loaded */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
      INSIST(!isc_time_isepoch(&zone->expiretime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->expiretime, &next) < 0)
        next = zone->expiretime;
    }
    
    /* Dump if we need to, unless we already are */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->dumptime, &next) < 0)
        next = zone->dumptime;
    }
    break;

  case dns_zone_key:
    /* Dump if we need to, unless we already are */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->dumptime, &next) < 0)
        next = zone->dumptime;
    }
    
    /* Refresh our keys if needed */
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING)) {
      if (isc_time_isepoch(&next) ||
          (!isc_time_isepoch(&zone->refreshkeytime) &&
          isc_time_compare(&zone->refreshkeytime, &next) < 0))
        next = zone->refreshkeytime;
    }
    break;

  default:
    break;
  }

  if (isc_time_isepoch(&next)) {  /* No timers are scheduled to trigger */
    zone_debuglog(zone, me, 10, "settimer inactive");
    result = isc_timer_reset(zone->timer, isc_timertype_inactive,
            NULL, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not deactivate zone timer: %s",
             isc_result_totext(result));
  } else { /* Set the timer to be the soonest event we need to look at */
    if (isc_time_compare(&next, now) <= 0)
      next = *now;
    result = isc_timer_reset(zone->timer, isc_timertype_once,
           &next, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not reset zone timer: %s",
             isc_result_totext(result));
  }
}

Well, things did not get smaller, but larger! That’s OK though since readability and documentation improved. Just in case, do a compile to ensure we did not break things. If changing comments broke things, get more caffeine. Let’s get to actual code changes!

Factor out some duplicated code

The goal in this refactoring is to end up with a bit of code that could be more easily tested, is more readable, and is, well, less. Less duplicated code means less to test, less possible locations for bugs, and less code to drag along into the future. It is specifically not to change the behavior of the code, just its structure.

One thing that should immediately pop out is the repeated pattern:

1
2
3
4
5

    if (!isc_time_isepoch(&zone->resigntime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->resigntime, &next) < 0)
        next = zone->resigntime;
    }

This seems like a first place to start on our cleanup.

This particular code is tested by existing system-level tests. What a good test-driven coder would do here is write the tests for the code they wish they had. For this cleanup, I wish I had a function that would update an accumulator-type variable with a timer if it occurred sooner than what it already held. Go be a good TDD programmer and write that.

Good, got it written? Send it to ISC, please, in the form of a unit test! I’m just going to fumble along and rely on the system-level tests to check my work for this example.

Factoring out some of that repeated code, I came up with this helper function, and refactored the code to use it:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140

/*
 * Set "previous_time" to "new_time" if "new_time" is not the epoch
 * and "new_time" will happen sooner than "previous_time", or if
 * "previous_time" is the epoch.  In this case, epoch means
 * that timer is not set.
 */
static void
set_time_if_sooner(isc_time_t *previous_time, isc_time_t *new_time) {
  if (!isc_time_isepoch(new_time)) {
    if (isc_time_isepoch(previous_time) ||
        isc_time_compare(new_time, previous_time) < 0)
      *previous_time = *new_time;
  }
}

/*
 * A zone has one timer, but has events that occur at various times.
 * Go through the various events as needed for this zone type, and find
 * the one that will trigger next.  Set the zone's timer to this event's
 * time.
 *
 * If no events are scheduled to trigger, stop the timer.
 */
static void
zone_settimer(dns_zone_t *zone, isc_time_t *now) {
  const char me[] = "zone_settimer";
  isc_time_t next;
  isc_result_t result;

  ENTER;
  REQUIRE(DNS_ZONE_VALID(zone));
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
    return;

  isc_time_settoepoch(&next);

  switch (zone->type) {
  case dns_zone_master:
    /*
     * If we need to send a notify, set the timer.
     */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
      next = zone->notifytime;

    /* Do we need to dump to disk, and are not doing so? */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      set_time_if_sooner(&next, &zone->dumptime);
    }

    /* Do we need to refresh the signatures? */
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
      set_time_if_sooner(&next, &zone->refreshkeytime);
    
    /* Do we need to re-sign any data? */
    set_time_if_sooner(&next, &zone->resigntime);
    
    /* Do we need to warn about expiring keys? */
    set_time_if_sooner(&next, &zone->keywarntime);
    
    /* Do we need to sign the zone? */
    set_time_if_sooner(&next, &zone->signingtime);
    
    /* Do we need build or rebuild nsec3 chains? */
    set_time_if_sooner(&next, &zone->nsec3chaintime);

    break;

  case dns_zone_slave:
    /* Do we need to send notifies? */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
      next = zone->notifytime;
    /*FALLTHROUGH*/

  case dns_zone_stub:
    /*
     * If we are not refreshing and can, we have masters,
     * and we are not loading then check if we should do
     * do that next.
     */
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING))
      set_time_if_sooner(&next, &zone->refreshtime);
    
    /* Check expire time if we are loaded */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
      INSIST(!isc_time_isepoch(&zone->expiretime));
      set_time_if_sooner(&next, &zone->expiretime);
    }
    
    /* Dump if we need to, unless we already are */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      set_time_if_sooner(&next, &zone->dumptime);
    }

    break;

  case dns_zone_key:
    /* Dump if we need to, unless we already are */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      set_time_if_sooner(&next, &zone->dumptime);
    }
    
    /* Refresh our keys if needed */
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
      set_time_if_sooner(&next, &zone->refreshkeytime);

    break;

  default:

    break;
  }

  if (isc_time_isepoch(&next)) {  /* No timers are scheduled to trigger */
    zone_debuglog(zone, me, 10, "settimer inactive");
    result = isc_timer_reset(zone->timer, isc_timertype_inactive,
            NULL, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not deactivate zone timer: %s",
             isc_result_totext(result));
  } else { /* Set the timer to be the soonest event we need to look at */
    if (isc_time_compare(&next, now) <= 0)
      next = *now;
    result = isc_timer_reset(zone->timer, isc_timertype_once,
           &next, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not reset zone timer: %s",
             isc_result_totext(result));
  }
}

What we have now is a small helper function that we can test, and when it has proven itself we don’t need to worry about cut and paste typos nearly as much. We have less code to test, because we removed a lot of duplication.

Zone-type specific helper functions

Is there more we can do, you ask? There certainly is! I immediately notice that we have a switch statement, and each of its cases are rather long. Let’s make some more helper functions for each of these cases.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161

/*
 * Set "previous_time" to "new_time" if "new_time" is not the epoch
 * and "new_time" will happen sooner than "previous_time", or if
 * "previous_time" is the epoch.  In this case, epoch means
 * that timer is not set.
 */
static void
set_time_if_sooner(isc_time_t *previous_time, isc_time_t *new_time) {
  if (!isc_time_isepoch(new_time)) {
    if (isc_time_isepoch(previous_time) ||
        isc_time_compare(new_time, previous_time) < 0)
      *previous_time = *new_time;
  }
}

/* find next event for a master zone */
static void
check_master_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /*
   * If we need to send a notify, set the timer.
   */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
    *next = zone->notifytime;

  /* Do we need to dump to disk, and are not doing so? */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
    INSIST(!isc_time_isepoch(&zone->dumptime));
    set_time_if_sooner(next, &zone->dumptime);
  }

  /* Do we need to refresh the signatures? */
  if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
    set_time_if_sooner(next, &zone->refreshkeytime);
  
  /* Do we need to re-sign any data? */
  set_time_if_sooner(next, &zone->resigntime);
  
  /* Do we need to warn about expiring keys? */
  set_time_if_sooner(next, &zone->keywarntime);
  
  /* Do we need to sign the zone? */
  set_time_if_sooner(next, &zone->signingtime);
  
  /* Do we need build or rebuild nsec3 chains? */
  set_time_if_sooner(next, &zone->nsec3chaintime);
}

/* find next event for a slave zone */
static void
check_slave_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /* Do we need to send notifies? */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
    *next = zone->notifytime;
}

/* find next event for a stub (or slave) zone */
static void
check_stub_or_slave_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /*
   * If we are not refreshing and can, we have masters,
   * and we are not loading then check if we should do
   * do that next.
   */
  if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING))
    set_time_if_sooner(next, &zone->refreshtime);
  
  /* Check expire time if we are loaded */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
    INSIST(!isc_time_isepoch(&zone->expiretime));
    set_time_if_sooner(next, &zone->expiretime);
  }
  
  /* Dump if we need to, unless we already are */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
    INSIST(!isc_time_isepoch(&zone->dumptime));
    set_time_if_sooner(next, &zone->dumptime);
  }
}

/* find next event for a key zone */
static void
check_key_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /* Dump if we need to, unless we already are */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
    INSIST(!isc_time_isepoch(&zone->dumptime));
    set_time_if_sooner(next, &zone->dumptime);
  }
  
  /* Refresh our keys if needed */
  if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
    set_time_if_sooner(next, &zone->refreshkeytime);
}

/*
 * A zone has one timer, but has events that occur at various times.
 * Go through the various events as needed for this zone type, and find
 * the one that will trigger next.  Set the zone's timer to this event's
 * time.
 *
 * If no events are scheduled to trigger, stop the timer.
 */
static void
zone_settimer(dns_zone_t *zone, isc_time_t *now) {
  const char me[] = "zone_settimer";
  isc_time_t next;
  isc_result_t result;

  ENTER;
  REQUIRE(DNS_ZONE_VALID(zone));
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
    return;

  isc_time_settoepoch(&next);

  switch (zone->type) {
  case dns_zone_master:
    check_master_zone_timers(&next, zone);
    break;

  case dns_zone_slave:
    check_slave_zone_timers(&next, zone);
    /*FALLTHROUGH*/

  case dns_zone_stub:
    check_stub_or_slave_zone_timers(&next, zone);
    break;

  case dns_zone_key:
    check_key_zone_timers(&next, zone);
    break;

  default:

    break;
  }

  if (isc_time_isepoch(&next)) {  /* No timers are scheduled to trigger */
    zone_debuglog(zone, me, 10, "settimer inactive");
    result = isc_timer_reset(zone->timer, isc_timertype_inactive,
            NULL, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not deactivate zone timer: %s",
             isc_result_totext(result));
  } else { /* Set the timer to be the soonest event we need to look at */
    if (isc_time_compare(&next, now) <= 0)
      next = *now;
    result = isc_timer_reset(zone->timer, isc_timertype_once,
           &next, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not reset zone timer: %s",
             isc_result_totext(result));
  }
}

Now we have even more bite-sized, testable functions which we can target and verify independently with unit tests. As before, a good TDD coder would have written unit tests for each of these functions before factoring them out.

Switching

Is there anything else that we might want to do? The switch statement itself should go into a helper function.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167

/*
 * Set "previous_time" to "new_time" if "new_time" is not the epoch
 * and "new_time" will happen sooner than "previous_time", or if
 * "previous_time" is the epoch.  In this case, epoch means
 * that timer is not set.
 */
static void
set_time_if_sooner(isc_time_t *previous_time, isc_time_t *new_time) {
  if (!isc_time_isepoch(new_time)) {
    if (isc_time_isepoch(previous_time) ||
        isc_time_compare(new_time, previous_time) < 0)
      *previous_time = *new_time;
  }
}

/* find next event for a master zone */
static void
check_master_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /*
   * If we need to send a notify, set the timer.
   */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
    *next = zone->notifytime;

  /* Do we need to dump to disk, and are not doing so? */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
    INSIST(!isc_time_isepoch(&zone->dumptime));
    set_time_if_sooner(next, &zone->dumptime);
  }

  /* Do we need to refresh the signatures? */
  if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
    set_time_if_sooner(next, &zone->refreshkeytime);
  
  /* Do we need to re-sign any data? */
  set_time_if_sooner(next, &zone->resigntime);
  
  /* Do we need to warn about expiring keys? */
  set_time_if_sooner(next, &zone->keywarntime);
  
  /* Do we need to sign the zone? */
  set_time_if_sooner(next, &zone->signingtime);
  
  /* Do we need build or rebuild nsec3 chains? */
  set_time_if_sooner(next, &zone->nsec3chaintime);
}

/* find next event for a slave zone */
static void
check_slave_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /* Do we need to send notifies? */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
    *next = zone->notifytime;
}

/* find next event for a stub (or slave) zone */
static void
check_stub_or_slave_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /*
   * If we are not refreshing and can, we have masters,
   * and we are not loading then check if we should do
   * do that next.
   */
  if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING))
    set_time_if_sooner(next, &zone->refreshtime);
  
  /* Check expire time if we are loaded */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
    INSIST(!isc_time_isepoch(&zone->expiretime));
    set_time_if_sooner(next, &zone->expiretime);
  }
  
  /* Dump if we need to, unless we already are */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
    INSIST(!isc_time_isepoch(&zone->dumptime));
    set_time_if_sooner(next, &zone->dumptime);
  }
}

/* find next event for a key zone */
static void
check_key_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /* Dump if we need to, unless we already are */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
    INSIST(!isc_time_isepoch(&zone->dumptime));
    set_time_if_sooner(next, &zone->dumptime);
  }
  
  /* Refresh our keys if needed */
  if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
    set_time_if_sooner(next, &zone->refreshkeytime);
}

/* Find the next event based on zone type. */
static void
check_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  switch (zone->type) {
  case dns_zone_master:
    check_master_zone_timers(next, zone);
    break;

  case dns_zone_slave:
    check_slave_zone_timers(next, zone);
    /*FALLTHROUGH*/

  case dns_zone_stub:
    check_stub_or_slave_zone_timers(next, zone);
    break;

  case dns_zone_key:
    check_key_zone_timers(next, zone);
    break;

  default:

    break;
  }
}

/*
 * A zone has one timer, but has events that occur at various times.
 * Go through the various events as needed for this zone type, and find
 * the one that will trigger next.  Set the zone's timer to this event's
 * time.
 *
 * If no events are scheduled to trigger, stop the timer.
 */
static void
zone_settimer(dns_zone_t *zone, isc_time_t *now) {
  const char me[] = "zone_settimer";
  isc_time_t next;
  isc_result_t result;

  ENTER;
  REQUIRE(DNS_ZONE_VALID(zone));
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
    return;

  isc_time_settoepoch(&next);

  check_zone_timers(&next, zone);

  if (isc_time_isepoch(&next)) {  /* No timers are scheduled to trigger */
    zone_debuglog(zone, me, 10, "settimer inactive");
    result = isc_timer_reset(zone->timer, isc_timertype_inactive,
            NULL, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not deactivate zone timer: %s",
             isc_result_totext(result));
  } else { /* Set the timer to be the soonest event we need to look at */
    if (isc_time_compare(&next, now) <= 0)
      next = *now;
    result = isc_timer_reset(zone->timer, isc_timertype_once,
           &next, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not reset zone timer: %s",
             isc_result_totext(result));
  }
}

Now we have a simpler function that is easier to read. It is something of a personal preference if we should factor out the case statement. I can see both sides of this. Is the final function easier or harder to read? This is also something of a personal preference, but I believe most people would say it is easier.

Knowing when to stop

Whew, lots of changes so far. Anything else? I would say no, at this point. We could factor the timer setting / canceling code into another helper function, especially if that pattern repeats a lot elsewhere. However, for this code, I think it’s fine to leave it as it stands. There is still some duplication, such as checking for dumptime in multiple functions, but I think that’s fine. The check for the result of timer operations is duplicated as well, but that too seems simple enough as it stands.

Knowing when to stop refactoring is just as critical as identifying the need to refactor. This comes with experience, time, and patience. When the code becomes longer and harder to read by refactoring, it’s a mistake; when the code becomes longer but is more readable, it’s OK. Most of the length increase we added was not code but comments, whitespace, and function declarations. That’s quite OK.

Filed under: Programming | Tagged with:      | 0 comments

How I test Ruby on Rails with RSpec and Cucumber

Posted on November 19, 2009 by Michael

In any non-trivial application, I end up with several things in common.

  • Generic pages which anyone can see. (I usually make this the About controller)
  • Users
  • Things (DNS Zones, pictures, etc)
  • Access restrictions to those users and things
  • Tests to ensure access control to those users and things

When to use RSpec, and when to use Cucumber

I use RSpec for unit tests, and some low-level controller tests. I specifically do not use RSpec for “user experience” or multiple-step testing, so no views are tested using RSpec.

Cucumber is used for all things “user experience.” It will test that a user cannot access other’s pages, etc. but it doesn’t do anything that can’t easily be done by filling in forms or changing the URL.

Types of users

In my world, there are three kinds of users which appear over and over again.

  • Guests. These guys can look at anything in the About controller.
  • Logged-in users. These can look at anything they own and some things that they do not. They can edit anything they own (with security restrictions on some fields.)
  • Administrators. These are the ones who can look at and modify anything. I usually have an admin rights on/off toggle, and try hard to make what they see close to what users would see.

User tests

  • Tests to ensure that the public cannot do things.
  • Tests to ensure that a logged-in user cannot poke around in someone else’s business using usual page contents.
  • Tests to ensure the the public or a logged in user cannot use specially crafted requests to break things.
  • Tests to ensure that a user can change their own stuff.
  • Tests to ensure that an admin can do pretty much everything.

Most of these tests are a combination of RSpec and Cucumber. The “hack” type tests (like directly fiddling around with assignments which my normal forms may limit to only items the logged in user has access to) are best done with RSpec.

Examples

No blog post can possibly be useful without examples. They use Factory Girl to generate a user, which has an email, password, and an admin flag.

spec/spec_helper.rb snippit

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18

def login_user(options = {})
  @logged_in_user = Factory.create(:user, options)
  @controller.stub!(:current_user).and_return(@logged_in_user)
  @logged_in_user
end

def login_admin(options = {})
  options[:admin] = true
  @logged_in_user = Factory.create(:user, options)
  @controller.stub!(:current_user).and_return(@logged_in_user)
  @logged_in_user
end

def logout_user
  @logged_in_user = nil
  @controller.stub!(:current_user).and_return(@logged_in_user)
  @logged_in_user
end

My complete spec/controllers/userscontrollerspec.rb file

Sorry for the length of this part.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270

require 'spec_helper'

describe UsersController do
  setup :activate_authlogic

  before(:each) do
    logout_user
    @other_user = Factory.create(:user)
  end

  def mock_user(stubs={})
    @mock_user ||= mock_model(User, stubs)
  end

  def make_users
    users = [ @other_user ]
    users << @logged_in_user if @logged_in_user
    4.times do
      users << Factory.create(:user)
    end
    users  
  end

  describe "GET index" do
    it "assigns all users as @users (admin)" do
      login_admin
      users = make_users
      get :index
      assigns[:users].sort.should == users.sort
    end

    it "tells me to bugger off (not admin)" do
      login_user
      users = make_users
      get :index
      flash[:error].should match "You must be an administrator to access this page."
      response.should redirect_to(root_path)
    end

    it "tells me to bugger off (not logged in)" do
      users = make_users
      get :index
      flash[:error].should match "You must be an administrator to access this page."
      response.should redirect_to(root_path)
    end
  end

  describe "GET show" do
    it "assigns myself as @user (admin)" do
      login_admin
      get :show, :id => @logged_in_user.id
      assigns[:user].should == @logged_in_user
    end

    it "assigns me as @user (my data)" do
      login_user
      get :show, :id => @logged_in_user.id
      assigns[:user].should == @logged_in_user
    end

    it "assigns the requested user as @user (admin)" do
      login_admin
      get :show, :id => @other_user.id
      assigns[:user].should == @other_user
    end

    it "tells me to bugger off (not admin)" do
      login_user
      get :show, :id => @other_user.id
      flash[:error].should match "You must be an administrator to access this page."
    end

    it "redirects to root (not admin)" do
      login_user
      get :show, :id => @other_user.id
      response.should redirect_to(root_path)
    end

    it "tells me to log in (not logged in)" do
      get :show, :id => @other_user.id
      flash[:error].should match "Please log in to access this page."
    end

    it "redirects to root (not logged in)" do
      get :show, :id => @other_user.id
      response.should redirect_to(login_path)
    end
  end

  describe "GET new" do
    it "assigns a new user as @user (not logged in)" do
      User.stub!(:new).and_return(mock_user)
      get :new
      assigns[:user].should equal(mock_user)
    end
  end

  describe "GET edit" do
    it "assigns me as @user (admin)" do
      login_admin
      get :edit, :id => @logged_in_user.id
      assigns[:user].should == @logged_in_user
    end

    it "assigns the requested user as @user (admin)" do
      login_admin
      get :edit, :id => @logged_in_user.id
      assigns[:user].should == @logged_in_user
    end

    it "assigns me as @user (my data)" do
      login_user
      get :edit, :id => @logged_in_user.id
      assigns[:user].should == @logged_in_user
    end

    it "tells me to bugger off (not admin)" do
      login_user
      get :edit, :id => @other_user.id
      flash[:error].should match "You must be an administrator to access this page."
      end

    it "redirects to root (not admin)" do
      login_user
      get :edit, :id => @other_user.id
      response.should redirect_to(root_path)
    end

    it "tells me to log in (not logged in)" do
      get :edit, :id => @other_user.id
      flash[:error].should match "Please log in to access this page."
    end

    it "redirects to login (not logged in)" do
      get :edit, :id => @other_user.id
      response.should redirect_to(login_path)
    end
  end

  describe "PUT update" do
    describe "with valid params" do
      it "assigns me as @user (my data)" do
        login_user
        put :update, :id => @logged_in_user.id
        assigns[:user].should == @logged_in_user
      end

      it "tells me to bugger off (not admin)" do
        login_user
        put :update, :id => @other_user.id
        flash[:error].should match "You must be an administrator to access this page."
      end

      it "tells me to log in (not logged in)" do
        get :edit, :id => @other_user.id
        flash[:error].should match "Please log in to access this page."
      end

      it "assigns the requested user as @user (admin)" do
        login_admin
        put :update, :id => @logged_in_user.id
        assigns[:user].should == @logged_in_user
      end

      it "redirects to the user (admin)" do
        login_admin
        put :update, :id => @logged_in_user.id
        response.should redirect_to(user_url(@logged_in_user))
      end

      it "redirects to me (my data)" do
        login_user
        put :update, :id => @logged_in_user.id
        response.should redirect_to(user_url(@logged_in_user))
      end

      it "redirects to root (not admin)" do
        login_user
        put :update, :id => @other_user.id
        response.should redirect_to(root_path)
      end

      it "redirects to login (not logged in)" do
        put :update, :id => @other_user.id
        response.should redirect_to(login_path)
      end

      it "can edit anyone's data (admin)" do
        login_admin
        put :update, :id => @other_user.id, :user => { :email => "new_email@example.com"}
        response.should redirect_to(user_url(@other_user))
        assigns[:user].email.should == "new_email@example.com"
      end

      it "can edit my own data (not admin)" do
        login_user
        put :update, :id => @logged_in_user.id, :user => { :email => "new_email@example.com"}
        response.should redirect_to(user_url(@logged_in_user))
        assigns[:user].email.should == "new_email@example.com"
      end
    end

    describe "with invalid params" do
      it "updates the requested user (admin)" do
        login_admin
        User.should_receive(:find).with("37").and_return(mock_user)
        mock_user.should_receive(:update_attributes).with({'these' => 'params'})
        put :update, :id => "37", :user => {:these => 'params'}
      end

      it "assigns the user as @user (admin)" do
        login_admin
        User.stub!(:find).and_return(mock_user(:update_attributes => false))
        put :update, :id => "1"
        assigns[:user].should equal(mock_user)
      end

      it "re-renders the 'edit' template (admin)" do
        login_admin
        User.stub!(:find).and_return(mock_user(:update_attributes => false))
        put :update, :id => "1"
        response.should render_template('edit')
      end
    end

  end

  describe "DELETE destroy" do
    it "destroys the requested user (admin)" do
      login_admin
      User.should_receive(:find).with("37").and_return(mock_user)
      mock_user.should_receive(:destroy)
      delete :destroy, :id => "37"
    end

    it "redirects to the users list (admin)" do
      login_admin
      User.stub!(:find).and_return(mock_user(:destroy => true))
      delete :destroy, :id => "1"
      response.should redirect_to(users_url)
    end

    it "destroys me (my data)" do
      login_user
      delete :destroy, :id => @logged_in_user.id
    end

    it "tells me to bugger off (not admin)" do
      login_user
      delete :destroy, :id => @other_user.id
      flash[:error].should match "You must be an administrator to access this page."
    end

    it "tells me to log in (not logged in)" do
      delete :destroy, :id => @other_user.id
      flash[:error].should match "Please log in to access this page."
    end

    it "redirects to root (not admin)" do
      login_user
      delete :destroy, :id => @other_user.id
      response.should redirect_to(root_path)
    end

    it "redirects to login (not logged in)" do
      delete :destroy, :id => @other_user.id
      response.should redirect_to(login_path)
    end
  end
end

Filed under: Programming | Tagged with:     | 0 comments

attr_accessible vs accepts_nested_attributes_for

Posted on November 10, 2009 by Michael

First, I just have to say that I never thought I’d be in Japan, let along blogging from here.

Today I ran into a little gotcha with restricting access to models when trying out the Rails 2.3 method of nested models. In my model, I have:

1
2
3
4
5
6

  has_many :dnskeys, :dependent => true
  accepts_nested_attributes_for :dnskeys,
    :allow_destroy => true,
    :reject_if => proc { |attrs| attrs.all? { |k, v| v.blank? }}

  attr_accessible :name

I was surprised, although I should not have been, that this prevented me from posting a zone and its dnskeys in the same action. Adding the proper attr_accessible line made the magic happen again:



  attr_accessible :dnskeys_attributes

Filed under: Programming | Tagged with:    | 1 comment

Experiments with HAML

Posted on November 05, 2009 by Michael

I decided to try HAML today, rather than the standard ERB templates. The jury is still out on if it is better or worse, but like many things Rails, it is better to just dive in and try it.

I have a little project (not yet released) which I’m converting over to use HAML. Since I also use rspec for testing, along with Cucumber and Webrat, I thought I had a long day ahead of me.

It turns out most of it can be made close to automatic, with just a little cleanup afterwards.

Using the rake task below, you can issue these two commands to convert your existing *.html.erb views into *.html.haml. The second target will convert the rspec view tests to expect haml. Renaming the spec tests is not required, but I like sanity.

1
2

  $ rake haml:convert:html
  $ rake haml:convert:spec

If you use a SCM (I still like Subversion best) you’ll want to remove the old files and add the new ones. For Subversion, I do:

1
2

  $ svn rm app/views/*/*.html.erb spec/views/*/*.html.erb_spec.rb
  $ svn add app/views/*/*.html.haml spec/views/*/*.html.haml_spec.rb

The main problems were that not all of the templates were properly indented, and “- end” lines appeared in the HAML templates. Fixing this was fast, as I really don’t have much in my templates yet as this is a new project. This step too could probably be done in an automated way. However, the indentation is probably going to require a bit of manual work.

While there, I also fixed up multi-line constructs generated with a much nicer compact format:

1
2
3
4
5
6
7
8

  %h1
    Title
  %tr
    %th
      Heading
    %th
      Heading
...

Compare with this:

1
2
3
4
5
6

  %h1 Title

  %tr
    %th Heading
    %th Heading
...

And now for the rake task:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33

# RAILS_ROOT/lib/tasks/haml_convert.rake

require 'haml'
require 'haml/exec'
 
namespace :haml do
  namespace :convert do
    desc "Convert all *.html.erb files to *.html.haml"
    task :html do
      Dir.glob("app/views/**/*.html.erb").each do |html|
        puts html + "..."
        Haml::Exec::HTML2Haml.new([html, html.gsub(".html.erb", ".html.haml")]).process_result
        File.delete(html)
      end
    end

    desc "Convert all *.html.erb_spec.rb files to *.html.haml_spec.rb"
    task :spec do
      Dir.glob("spec/views/**/*.html.erb_spec.rb").each do |oldname|
        puts oldname + "..."
        newname = oldname.gsub(".html.erb", ".html.haml")
        nf = File.open(newname, "w")
        File.open(oldname) do |of|
          of.each_line do |line|
            nf.puts line.gsub(".html.erb", ".html.haml")
          end
        end
        nf.close
        #File.delete(oldname)
      end
    end
  end
end

Filed under: Programming | Tagged with:    | 0 comments

ActiveResource and Shallow Nested Routes

Posted on November 04, 2009 by Michael

I recently tried to dive into ActiveResource, which allows a Ruby (usually Ruby on Rails) application to connect to a remote RESTful API and treat it almost as if it were local. There were some problems with my API from ActiveResource’s point of view.

Firstly, I use nested routes. This isn’t really a problem as it is possible to specify a prefix to add to the path, and even to substitution on this path. However, Rails added the concept of a “shallow nested route” a while back.

A shallow nested route is a short-hand notation which allows one to appear to scope out collections (that is, a list of things) from the actual thing itself. For example, in my application (https://dlv.isc.org/) I have:

/users/123 <– specific userid

/users/123/zones <— list of all zones for user 123

/zones/456 <— specific zone

This allows me to look at a specific user’s zones (or them, their own zones) without having to do some sort of special back-end processing which relies on something not in the path, such as the @current_user instance variable. After all, how would an admin list a user’s zones if /zones returned the current user’s zones only? Through an admin interface probably, but that seems messy. Shallow routes are seemingly more clean.

However, they break ActiveResource’s concept of the world.

With a little trickery, however, I managed to get shallow nested routes to work without having to do much additional work. I did have to pass in an additional argument to the collection list(s) however.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26

class Zone < ActiveResource::Base
  self.site = SITE
  self.user = USERNAME
  self.password = PASSWORD
  
  def self.collection_path(prefix_options = {}, query_options = nil)
    prefix_options, query_options = split_options(prefix_options) if query_options.nil?
    "/users/#{USERID}#{prefix(prefix_options)}#{collection_name}.#{format.extension}#{query_string(query_options)}"
  end
end

class Dnskey < ActiveResource::Base
  self.site = SITE
  self.user = USERNAME
  self.password = PASSWORD
  
  def self.collection_path(prefix_options = {}, query_options = nil)
    prefix_options, query_options = split_options(prefix_options) if query_options.nil?
    z = ''
    if query_options.has_key?(:zone_id)
      z = "/zones/#{query_options[:zone_id]}"
      query_options.delete(:zone_id)
    end
    "#{z}#{prefix(prefix_options)}#{collection_name}.#{format.extension}#{query_string(query_options)}"
  end
end

In my case, I used a constant USERID to the Zone’s collection, but left the specific item (aka “elementpath”) alone. For Dnskeys, where I needed to pass in different zoneid values, I had to do a small trick.

I call this as:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

all_zones = Zones.find :all # returns a list of all zones for the USERID
z = Zone.find(1) # returns only Zone with the id of 1
z.destroy # destroys the zone, uses the element_path()

z = Zone.find(2)
keys = Dnskey.find(:all, { :zone_id => z.id }) # disgusting, but works

#
# This is a hack.  I want to use Dnskey.create here, but it won't work since I cannot
# pass the zone_id along, and there are no association hints so I can't use
# zone.dnskeys.create() like I can with ActiveRecord.
#
d = Dnskey.new(:flags => key.flags, :algorithm => key.algorithm.to_i, ...)
d.prefix_options[:zone_id] = zone.id
d.save

Filed under: Programming | Tagged with:     | 0 comments

String#bitruncate

Posted on February 21, 2009 by Michael

And that’s ‘“bi-truncate” not “bit-uncate”.

What’s it do?

1
2

"This is a test".bitruncate(:length => 6) ==> "Thi...est"
"This is a test".bitruncate(:elength => 6) ==> "...a test"

The default options are { :length => 30 } which will produce 15 characters from the front and 15 from the end, putting … marks in the middle.

For rails, I put this in my lib/core_extensions.rb file.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74

#
# Copyright (c) 2009 Michael Graff.  All rights reserved.
#
# Redistribution and use in source and binary forms, with or
# without modification, are permitted provided that the following
# conditions are met:
# 1. Redistributions of source code must retain the above copyright
#    notice, this list of conditions and the following disclaimer.
# 2. Redistributions in binary form must reproduce the above
#    copyright notice, this list of conditions and the following
#    disclaimer in the documentation and/or other materials provided
#    with the distribution.
# 3. The name of Michael Graff may not be used to endorse or promote
#    products derived from this software without specific prior
#    written permission.
#
# THIS SOFTWARE IS PROVIDED BY Michael Graff ``AS IS'' AND ANY
# EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
# PARTICULAR PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL Micahel Graff
# BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED
# TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY
# OF SUCH DAMAGE.
#

class String
  #
  # Truncate from both ends of a string.  The :length parameter, which defaults
  # to 30, will return the first 15 and the last 15 characters from a string
  # if it is longer than 30 characters.  If it is shorter, the entire string
  # is returned.
  #
  # Another way to specify the front and back portions are with :flength and
  # :elength.  If you specify one of these but not the other then you will
  # not get the missing part.  e.g., :flength => 10 alone will return only
  # the first 10 charcters of the string.  This is the same as the standard
  # truncate(s, :length => 10) helper.
  #
  # If a :length parameter is provided it will override any other lengths
  # specified.
  #
  def bitruncate(options = {})
    maxlength = options[:length] || 0
    flength = options[:flength] || 0
    elength = options[:elength] || 0
    omission = options[:omission] || '...'

    if maxlength == 0 && flength == 0 && elength == 0
      maxlength = 30
    end
    if maxlength != 0
      flength = maxlength / 2
      elength = maxlength / 2
    end

    return self if length <= (flength + elength)

    front = ''
    back = ''
    if flength > 0
      front = self[0..(flength - 1)]
    end
    if elength > 0
      back = self[(length - elength)..(length)]
    end

    front + omission + back
  end
end

Filed under: Programming | Tagged with:    | 0 comments

Checking Credit Card Numbers in Ruby

Posted on March 24, 2008 by Michael

This is not meant to be an exhaustive list of all possible numbers, nor the only or best method to verify that they pass the “checksum” test, but here’s what I came up with.

I wrote this mostly to link a Ruby version of the code to Wikipedia’s article on Luhn checksum validation, since nearly every other language in use was listed, but Ruby was sadly missing.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101

#!/usr/bin/env ruby

#
# Copyright (c) 2008 Michael Graff.  All rights reserved.
#
# Redistribution and use in source and binary forms, with or
# without modification, are permitted provided that the following
# conditions are met:
# 1. Redistributions of source code must retain the above copyright
#    notice, this list of conditions and the following disclaimer.
# 2. Redistributions in binary form must reproduce the above
#    copyright notice, this list of conditions and the following
#    disclaimer in the documentation and/or other materials provided
#    with the distribution.
# 3. The name of Michael Graff may not be used to endorse or promote
#    products derived from this software without specific prior
#    written permission.
#
# THIS SOFTWARE IS PROVIDED BY Michael Graff ``AS IS'' AND ANY
# EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
# PARTICULAR PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL Micahel Graff
# BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED
# TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY
# OF SUCH DAMAGE.
#


class Luhn
  public
  def self.check_luhn(s)
    s.gsub!(/[^0-9]/, "")
    ss = s.reverse.split(//)

    alternate = false
    total = 0
    ss.each do |c|
      if alternate
        total += double_it(c.to_i)
      else
        total += c.to_i
      end
      alternate = !alternate
    end
    (total % 10) == 0
  end

  private
  def self.double_it(i)
    i = i * 2
    if i > 9
      i = i % 10 + 1
    end
    i
  end

end

if $0 == __FILE__
  def test_valid(s)
    result = Luhn::check_luhn(s)
    if result
      puts "VALID: #{s}"
    else
      puts "INVALID: #{s} (should be valid)"
    end
  end

  test_valid('5105 1051 0510 5100') # Mastercard
  test_valid('5555 5555 5555 4444') # Mastercard

  test_valid('4222 2222 2222 2')    # Visa
  test_valid('4111 1111 1111 1111') # Visa
  test_valid('4012 8888 8888 1881') # Visa

  test_valid('3782 8224 6310 005')  # American Express
  test_valid('3714 4963 5398 431')  # American Express
  test_valid('3787 3449 3671 000')  # American Express Corporate
  test_valid('3782 8224 6310 005')  # Amex
  test_valid('3400 0000 0000 009')  # Amex
  test_valid('3700 0000 0000 002')  # Amex

  test_valid('38520000023237')      # Diners Club (14 digits)
  test_valid('30569309025904')      # Diners Club (14 digits)

  test_valid('6011111111111117')    # Discover (16 digits)
  test_valid('6011 0000 0000 0004') # Discover
  test_valid('6011 0000 0000 0012') # Discover
  test_valid('6011000990139424')    # Discover (16 digits)
  test_valid('6011601160116611')    # Discover (16 digits)

  test_valid('3530111333300000')    # JCB (16 digits)
  test_valid('3566002020360505')    # JCB (16 digits)

  test_valid('5431111111111111')    # Mastercard (16 digits)
end

Filed under: Programming | Tagged with:    | 0 comments

Javascript application framework 'extjs' and privacy

Posted on March 19, 2008 by Michael

Out of the box, extjs version 2.0.2 leaks privacy information.

If you fail to change the value of Ext.BLANK_IMAGE_URL to something local, it will default to http://extjs.com/s.gif. At first this might not seem bad, but remember that every time this image is fetched the referring URL is sent to the extjs.com web server.

At worse, this is a minor information link. Depending on what you might place in your URL line, this could be a major issue.

I have posted a comment on the extjs forums, but so far the developers don’t see the problem. They say it is well documented in their FAQ, and that it is documented in the API docs.

I would prefer they opt for a warning message saying “You did not set …” rather than leaking information by default. I’ll probably have to post a CERT on this one. :/

Filed under: Programming | Tagged with:    | 0 comments

Simple Rails Caching

Posted on October 28, 2007 by Michael

In my apache configuration I have Apache serving static pages from the public directory. After playing around with Mephisto a bit, I’ve learned a few tricks on caching.

The common belief is to use memcache for caching on sites that are large enough to need it. However, I think with a few tricks, I can do better. I think in some cases, letting Apache serve the content is a far wiser move.

For example, I will begin writing out the images for avatars on guildcorner.org to files, and let Apache serve them. I will do this when they are created, updated, or rendered to a browser. This will let Apache serve the next request for it. I also plan on deleting the cache file if the avatar is deleted.

Since I use Capistrano to push out releases now, and the cached files are not in subversion, upgrading to a new application version won’t require anything special.

Filed under: Programming | Tagged with:    | 0 comments

RailsMode (not ENV['RAILS_ENV'])

Posted on October 27, 2007 by Michael

RailsMode

I’ve been spreading things like this all around my code, where I wanted to do something differently in production vs. development mode. Previously, I’d write something like this:

1
2
3

if ENV['RAILS_ENV'] == 'production'
  ... perform magic ...
end

While this is pretty simple, it just didn’t feel very DRY. So, I decided to use this as a reason to learn about modules and mixins.

I have a generic plug-in in vendor/plugins that I put small bits of code like this. You might as well, but if you don’t, you can drop this in a helper.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

module RailsMode
  def railsmode(*list)
    list.map! do |item|
      item.to_s
    end

    if block_given?
      if list.include?(ENV['RAILS_ENV'])
        yield
      end
    else
      return list.include?(ENV['RAILS_ENV'])
    end
  end
end

I also put this line in my environment.rb file:



include RailsMode

This mixes the module into the current class. Doing this in environment.rb makes it available everywhere in rails. Putting that line in a specific file would also work, such as a controller, or a helper.

With this, I can now write:

1
2
3

if railsmode(:production)
  ... perform magic ...
end

I can also check for multiple modes at once:

1
2
3

if railsmode(:production, :development)
  ... perform magic ...
end

And of course, who needs an if when I can pass in a block:

1
2
3

railsmode(:production) do
  ... perform magic ...
end

Filed under: Programming | Tagged with:     | 1 comment