Skip to content

Refactor region validation scope from topology #44

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mateu merged 2 commits into from
Apr 5, 2026
Merged

Refactor region validation scope from topology #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0eeb9e
refactor: derive region validation scope from bracket graph
mateu Apr 5, 2026
a661854
Update lib/Bracket/Service/BracketValidator.pm
mateu Apr 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 38 additions & 4 deletions lib/Bracket/Service/BracketValidator.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ sub validate_region_payload {
my ($pick_map, $parse_errors) = _extract_pick_map($params);
return { ok => 0, errors => $parse_errors } if @{$parse_errors};

my $min_game = 1 + 15 * ($region_id - 1);
my $max_game = 15 + 15 * ($region_id - 1);
my $region_game_ids = _region_game_ids_for_region($schema, $region_id);

my @errors;
my %existing = map { $_->game->id => $_->pick->id }
Expand All @@ -24,7 +23,7 @@ sub validate_region_payload {

my @changed_games;
foreach my $game_id (sort { $a <=> $b } keys %{$pick_map}) {
if ($game_id < $min_game || $game_id > $max_game) {
if (!$region_game_ids->{$game_id}) {
push @errors, "Game ${game_id} is outside region ${region_id}";
next;
}
Expand All @@ -36,7 +35,7 @@ sub validate_region_payload {
\@changed_games,
sub {
my ($game_id) = @_;
return $game_id >= $min_game && $game_id <= $max_game;
return $region_game_ids->{$game_id};
}
);
Comment on lines 17 to 40
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate_region_payload now triggers a full GameGraph->search({})->all scan in _region_game_ids_for_region, and then _affected_games_in_scope performs another full scan of GameGraph. Consider reusing a single precomputed edge list / parent-child maps (e.g., pass it into _affected_games_in_scope or cache per call) to avoid duplicate DB reads on each validation request.

Copilot uses AI. Check for mistakes.

Expand Down Expand Up @@ -129,6 +128,41 @@ sub _extract_pick_map {
return (\%pick_map, \@errors);
}

sub _region_game_ids_for_region {
my ($schema, $region_id) = @_;

my $region_winner_games = Bracket::Service::BracketStructure->region_winner_games_by_region($schema);
my $region_winner_game_id = $region_winner_games->{$region_id};
return _fallback_region_game_ids($region_id) if !$region_winner_game_id;

my %parents_by_game;
foreach my $edge ($schema->resultset('GameGraph')->search({})->all) {
my $game_id = $edge->game;
my $parent_game_id = $edge->parent_game;
next if !defined $game_id || !defined $parent_game_id;

push @{$parents_by_game{$game_id}}, $parent_game_id;
}

my %allowed_games;
my @queue = ($region_winner_game_id);
while (@queue) {
my $game_id = shift @queue;
next if $allowed_games{$game_id}++;
push @queue, @{$parents_by_game{$game_id} || []};
}

return \%allowed_games;
}

sub _fallback_region_game_ids {
my ($region_id) = @_;

my $min_game = 1 + 15 * ($region_id - 1);
my $max_game = 15 + 15 * ($region_id - 1);
return { map { $_ => 1 } ($min_game .. $max_game) };
}
Comment on lines +158 to +164
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The legacy fallback path (_fallback_region_game_ids) is new but not covered by tests. Adding a small regression test that forces region_winner_games_by_region to return no entry (or an empty hash) would ensure the range-based scope still behaves as intended when topology mapping is unavailable.

Copilot uses AI. Check for mistakes.

sub _affected_games_in_scope {
my ($schema, $changed_games, $is_in_scope) = @_;

Expand Down
27 changes: 27 additions & 0 deletions t/bracket_validator.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,5 +177,32 @@ my $bad_param = Bracket::Service::BracketValidator->validate_region_payload(
);
ok(!$bad_param->{ok}, 'non-numeric team id is rejected');

{
no warnings 'redefine';
local *Bracket::Service::BracketStructure::region_winner_games_by_region = sub {
return { 1 => 30 };
};

my $derived_region_games = Bracket::Service::BracketValidator::_region_game_ids_for_region($schema, 1);
ok($derived_region_games->{30}, 'region game scope includes remapped region-winner game');
ok($derived_region_games->{16}, 'region game scope follows remapped ancestry');
ok(!$derived_region_games->{1}, 'region game scope excludes original range when topology remaps');

my $remapped_region_validation = Bracket::Service::BracketValidator->validate_region_payload(
$schema,
$player_id,
1,
{
p30 => 1,
}
);
ok(!$remapped_region_validation->{ok}, 'remapped validation still enforces pick correctness');
unlike(
join(' ', @{$remapped_region_validation->{errors}}),
qr/outside region 1/i,
'remapped topology is used instead of fixed 1..15 range'
);
}


done_testing();
Loading