Permalink
Browse files

Index nodes by ID in NodeList.

This requires changes to a bunch of tests that previously were not setting a
proect URI.

To allow nodes to be looked up by ID without possibility of conflicting with
step names, also disallow names that match `/^[0-9a-f]{40}/`. Unlikely to
happen anyway, but best to be safe.

In passing, fix a bug in ID generation where the string to be hashed was not
first encoded as UTF-8.
  • Loading branch information...
1 parent 6c2f28d commit 1ea91db1d4abdd80a2317db2869ed432d9df74ae @theory committed Jun 14, 2012
Showing with 91 additions and 21 deletions.
  1. +10 −0 lib/App/Sqitch/Plan.pm
  2. +18 −6 lib/App/Sqitch/Plan/NodeList.pm
  3. +2 −1 lib/App/Sqitch/Plan/Step.pm
  4. +2 −1 lib/App/Sqitch/Plan/Tag.pm
  5. +5 −3 t/engine.t
  6. +15 −3 t/nodelist.t
  7. +39 −7 t/plan.t
@@ -153,6 +153,13 @@ sub _parse {
': "HEAD" is a reserved name',
) if $params{name} eq 'HEAD';
+ # It must not loo, like a SHA1 hash.
+ $self->sqitch->fail(
+ "Syntax error in $file at line ",
+ $fh->input_line_number,
+ qq{: "$params{name}" is invalid because it could be confused with a SHA1 ID},
+ ) if $params{name} =~ /^[0-9a-f]{40}/;
+
if ($type eq 'tag') {
# Fail if no steps.
unless ($prev_step) {
@@ -414,6 +421,9 @@ sub add_step {
sub _is_valid {
my ( $self, $type, $name ) = @_;
$self->sqitch->fail('"HEAD" is a reserved name') if $name eq 'HEAD';
+ $self->sqitch->fail(
+ qq{"$name" is invalid because it could be confused with a SHA1 ID}
+ ) if $name =~ /^[0-9a-f]{40}/;
$self->sqitch->fail(
qq{"$name" is invalid: ${type}s must not begin with punctuation },
@@ -11,6 +11,7 @@ sub new {
for my $node (@_) {
push @list => $node;
push @{ $index{ $node->format_name } } => $#list;
+ push @{ $index{ $node->id } } => $#list;
}
return bless {
@@ -110,8 +111,9 @@ sub append {
my $list = $self->{list};
for my $node (@_) {
push @{ $list } => $node;
- my $idx = $self->{lookup}{$node->format_name} ||= [];
+ my $idx = $self->{lookup}{ $node->format_name } ||= [];
push $idx, $#$list;
+ $self->{lookup}{ $node->id } = [$#$list];
}
return $self;
}
@@ -182,12 +184,21 @@ Returns the node at the specified index.
=head3 C<index_of>
+ my $index = $nodelist->index_of($node_id);
my $index = $nodelist->index_of($node_name);
-Returns the index of the named node. The name may be one of these forms:
+Returns the index of the node with the specified ID or name. The value passed
+may be one of these forms:
=over
+=item * An ID
+
+ my $index = $nodelist->index_of('6c2f28d125aff1deea615f8de774599acf39a7a1');
+
+This is the SHA1 hash of a step or tag. Currently, the full 40-character hexed
+hash string must be specified.
+
=item * A step name
my $index = $nodelist->index_of('users_table');
@@ -258,12 +269,13 @@ if the list contains no steps.
=head3 C<get>
+ my $node = $nodelist->get($node_id);
my $node = $nodelist->get($node_name);
-Returns the named node. The name may be specified as described for
-C<index_of()>. An exception will be thrown if more than one instance of the
-node appears. As such, it is best to specify it as unambiguously as possible:
-as a tag name or a tag-qualified step name.
+Returns the node for the specified ID or name. The name may be specified as
+described for C<index_of()>. An exception will be thrown if more than one
+instance of the node appears. As such, it is best to specify it as
+unambiguously as possible: as a tag name or a tag-qualified step name.
=head3 C<append>
@@ -4,6 +4,7 @@ use v5.10.1;
use utf8;
use namespace::autoclean;
use parent 'App::Sqitch::Plan::Line';
+use Encode;
use Moose;
has _dependencies => (
@@ -81,7 +82,7 @@ has id => (
isa => 'Str',
lazy => 1,
default => sub {
- my $content = shift->info;
+ my $content = encode_utf8 shift->info;
require Digest::SHA1;
return Digest::SHA1->new->add(
'step ' . length($content) . "\0" . $content
@@ -4,6 +4,7 @@ use v5.10.1;
use utf8;
use namespace::autoclean;
use Moose;
+use Encode;
use parent 'App::Sqitch::Plan::Line';
sub format_name {
@@ -31,7 +32,7 @@ has id => (
isa => 'Str',
lazy => 1,
default => sub {
- my $content = shift->info;
+ my $content = encode_utf8 shift->info;
require Digest::SHA1;
return Digest::SHA1->new->add(
'tag ' . length($content) . "\0" . $content
View
@@ -14,6 +14,7 @@ use Test::NoWarnings;
use Test::MockModule;
use Locale::TextDomain qw(App-Sqitch);
use App::Sqitch::X qw(hurl);
+use URI;
use lib 't/lib';
use MockOutput;
@@ -73,7 +74,8 @@ ENGINE: {
after seen => sub { @SEEN = () };
}
-ok my $sqitch = App::Sqitch->new(db_name => 'mydb'),
+my $uri = URI->new('https://github.com/theory/sqitch/');
+ok my $sqitch = App::Sqitch->new(db_name => 'mydb', uri => $uri),
'Load a sqitch sqitch object';
##############################################################################
@@ -244,7 +246,7 @@ can_ok $CLASS, '_sync_plan';
chdir 't';
my $plan_file = file qw(sql sqitch.plan);
-$sqitch = App::Sqitch->new( plan_file => $plan_file );
+$sqitch = App::Sqitch->new( plan_file => $plan_file, uri => $uri );
ok $engine = App::Sqitch::Engine::whu->new( sqitch => $sqitch ),
'Engine with sqitch with plan file';
my $plan = $sqitch->plan;
@@ -498,7 +500,7 @@ is_deeply +MockOutput->get_info, [
# Try a plan with no steps.
NOSTEPS: {
my $plan_file = file qw(nonexistent.plan);
- my $sqitch = App::Sqitch->new( plan_file => $plan_file );
+ my $sqitch = App::Sqitch->new( plan_file => $plan_file, uri => $uri );
ok $engine = App::Sqitch::Engine::whu->new( sqitch => $sqitch ),
'Engine with sqitch with no file';
throws_ok { $engine->deploy } 'App::Sqitch::X', 'Should die with no steps';
View
@@ -4,16 +4,19 @@ use strict;
use warnings;
use v5.10.1;
use utf8;
-use Test::More tests => 77;
+use Test::More tests => 86;
#use Test::More 'no_plan';
use Test::NoWarnings;
use Test::Exception;
use App::Sqitch;
use App::Sqitch::Plan;
+use URI;
BEGIN { require_ok 'App::Sqitch::Plan::NodeList' or die }
-my $sqitch = App::Sqitch->new;
+my $sqitch = App::Sqitch->new(
+ uri => URI->new('https://github.com/theory/sqitch/'),
+);
my $plan = App::Sqitch::Plan->new(sqitch => $sqitch);
my $foo = App::Sqitch::Plan::Step->new(plan => $plan, name => 'foo');
@@ -49,9 +52,13 @@ is $nodes->item_at(5), $yo2, 'Should have yo2 at 5';
is $nodes->index_of('non'), undef, 'Should not find "non"';
is $nodes->index_of('@non'), undef, 'Should not find "@non"';
is $nodes->index_of('foo'), 0, 'Should find foo at 0';
+is $nodes->index_of($foo->id), 0, 'Should find foo by ID at 0';
is $nodes->index_of('bar'), 1, 'Should find bar at 1';
+is $nodes->index_of($bar->id), 1, 'Should find bar by ID at 1';
is $nodes->index_of('@alpha'), 3, 'Should find @alpha at 3';
+is $nodes->index_of($alpha->id), 3, 'Should find @alpha by ID at 3';
is $nodes->index_of('baz'), 4, 'Should find baz at 4';
+is $nodes->index_of($baz->id), 4, 'Should find baz by ID at 4';
throws_ok { $nodes->index_of('yo') } qr/^\QKey "yo" at multiple indexes/,
'Should get error looking for index of "yo"';
@@ -67,9 +74,12 @@ is $nodes->index_of('baz@alpha'), undef, 'Should get undef for baz@alpha';
is $nodes->index_of('baz@HEAD'), 4, 'Should get 4 for baz@HEAD';
is $nodes->get('foo'), $foo, 'Should get foo for "foo"';
+is $nodes->get($foo->id), $foo, 'Should get foo by ID';
is $nodes->get('bar'), $bar, 'Should get bar for "bar"';
-is $nodes->get('@alpha'), $alpha, 'Should get @alpha for "@alpha"';
+is $nodes->get($bar->id), $bar, 'Should get bar by ID';
+is $nodes->get($alpha->id), $alpha, 'Should get @alpha by ID';
is $nodes->get('baz'), $baz, 'Should get baz for "baz"';
+is $nodes->get($baz->id), $baz, 'Should get baz by ID';
is $nodes->get('yo@alpha'), $yo1, 'Should get yo1 for yo@alpha';
is $nodes->get('yo@HEAD'), $yo2, 'Should get yo2 for yo@HEAD';
@@ -89,6 +99,8 @@ ok $nodes->append($hi), 'Push hi';
is $nodes->count, 7, 'Count should now be seven';
is_deeply [$nodes->items], [$foo, $bar, $yo1, $alpha, $baz, $yo2, $hi],
'Nodes should be in order with $hi at the end';
+is $nodes->index_of('hi'), 6, 'Should find "hi" at index 8';
+is $nodes->index_of($hi->id), 6, 'Should find "hi" by ID at index 8';
# Now try first_index_of().
is $nodes->first_index_of('non'), undef, 'First index of "non" should be undef';
View
@@ -14,6 +14,7 @@ use Test::File::Contents;
use Encode;
#use Test::NoWarnings;
use File::Path qw(make_path remove_tree);
+use URI;
use lib 't/lib';
use MockOutput;
@@ -34,7 +35,8 @@ can_ok $CLASS, qw(
open_script
);
-my $sqitch = App::Sqitch->new;
+my $uri = URI->new('https://github.com/theory/sqitch/');
+my $sqitch = App::Sqitch->new(uri => $uri);
isa_ok my $plan = App::Sqitch::Plan->new(sqitch => $sqitch), $CLASS;
# Set up some some utility functions for creating nodes.
@@ -56,7 +58,7 @@ sub clear {
sub step {
my @op = defined $_[4] ? split /([+-])/, $_[4] : ();
- return $prev_step = App::Sqitch::Plan::Step->new(
+ $prev_step = App::Sqitch::Plan::Step->new(
plan => $plan,
lspace => $_[0] // '',
name => $_[1],
@@ -67,17 +69,21 @@ sub step {
ropspace => $op[2] // '',
($prev_tag ? (since_tag => $prev_tag) : ()),
);
+ $prev_step->id;
+ return $prev_step;
}
sub tag {
- return $prev_tag = App::Sqitch::Plan::Tag->new(
+ $prev_tag = App::Sqitch::Plan::Tag->new(
plan => $plan,
step => $prev_step,
lspace => $_[0] // '',
name => $_[1],
rspace => $_[2] // '',
comment => $_[3] // '',
);
+ $prev_tag->id;
+ return $prev_tag;
}
sub prag {
@@ -285,6 +291,19 @@ cmp_deeply +MockOutput->get_fail, [[
': "HEAD" is a reserved name',
]], 'And the reserved tag error should have been output';
+# Try a plan with a step name that looks like a sha1 hash.
+my $sha1 = '6c2f28d125aff1deea615f8de774599acf39a7a1';
+$file = file qw(t plans sha1.plan);
+$fh = IO::File->new(\$sha1, '<:utf8');
+throws_ok { $plan->_parse($file, $fh) } qr/FAIL:/,
+ 'Should die on plan with SHA1 step name';
+is sorted, 0, 'Should have sorted steps nonce';
+cmp_deeply +MockOutput->get_fail, [[
+ "Syntax error in $file at line ",
+ 1,
+ qq{: "$sha1" is invalid because it could be confused with a SHA1 ID},
+]], 'And the SHA1 name error should have been output';
+
# Try a plan with a tag but no step.
$file = file qw(t plans tag-no-step.plan);
$fh = IO::File->new(\"\@foo\nbar", '<:utf8');
@@ -381,7 +400,7 @@ cmp_deeply { map { $_ => [$parsed->{$_}->items] } keys %{ $parsed } }, {
# Try a non-existent plan file with load().
$file = file qw(t hi nonexistent.plan);
-$sqitch = App::Sqitch->new(plan_file => $file);
+$sqitch = App::Sqitch->new(plan_file => $file, uri => $uri);
isa_ok $plan = App::Sqitch::Plan->new(sqitch => $sqitch), $CLASS,
'Plan with sqitch with nonexistent plan file';
@@ -390,7 +409,7 @@ cmp_deeply [$plan->nodes], [], 'Should have no nodes';
# Make sure that lines() loads the plan.
$file = file qw(t plans multi.plan);
-$sqitch = App::Sqitch->new(plan_file => $file);
+$sqitch = App::Sqitch->new(plan_file => $file, uri => $uri);
isa_ok $plan = App::Sqitch::Plan->new(sqitch => $sqitch), $CLASS,
'Plan with sqitch with plan file';
cmp_deeply [$plan->lines], [
@@ -615,6 +634,12 @@ cmp_deeply +MockOutput->get_fail, [[
'"HEAD" is a reserved name'
]], 'And the reserved name error should be output';
+throws_ok { $plan->add_tag($sha1) } qr/^FAIL:/,
+ 'Should get error for a SHA1 tag';
+cmp_deeply +MockOutput->get_fail, [[
+ qq{"$sha1" is invalid because it could be confused with a SHA1 ID},
+]], 'And the reserved name error should be output';
+
##############################################################################
# Try adding a step.
ok $plan->add_step('booyah'), 'Add step "booyah"';
@@ -666,7 +691,7 @@ cmp_deeply +MockOutput->get_fail, [[
'"HEAD" is a reserved name'
]], 'And the reserved name error should be output';
-# Try an invalid depednency.
+# Try an invalid dependency.
throws_ok { $plan->add_step('whu', ['nonesuch' ] ) } qr/^FAIL\b/,
'Should get failure for failed dependency';
cmp_deeply +MockOutput->get_fail, [[
@@ -682,10 +707,17 @@ cmp_deeply +MockOutput->get_fail, [[
'requires unknown tag "@nonesuch"'
]], 'The tag dependency error should have been emitted';
+# Should choke on a step that looks like a SHA1.
+throws_ok { $plan->add_step($sha1) } qr/^FAIL:/,
+ 'Should get error for a SHA1 step';
+cmp_deeply +MockOutput->get_fail, [[
+ qq{"$sha1" is invalid because it could be confused with a SHA1 ID},
+]], 'And the reserved name error should be output';
+
##############################################################################
# Try a plan with a duplicate step in different tag sections.
$file = file qw(t plans dupe-step-diff-tag.plan);
-$sqitch = App::Sqitch->new(plan_file => $file);
+$sqitch = App::Sqitch->new(plan_file => $file, uri => $uri);
isa_ok $plan = App::Sqitch::Plan->new(sqitch => $sqitch), $CLASS,
'Plan shoud work plan with dupe step across tags';
cmp_deeply [ $plan->lines ], [

0 comments on commit 1ea91db

Please sign in to comment.