From 530a5f7ca41a7e313667fad9b85e9034e7588f6f Mon Sep 17 00:00:00 2001 From: Amit Mishra Date: Tue, 30 Jun 2026 21:51:00 +0530 Subject: [PATCH 1/2] rpc: fix MatchMirror substring ambiguity for exact-match names When a mirror name is a substring of other mirror names (e.g. "fcix.net" vs. "mirror.fcix.net" and "paducahix.mm.fcix.net"), MatchMirror used a plain strings.Contains scan, so every mirror whose name contained the pattern would show up as a match. This made it impossible to address the shorter mirror on its own via "mb edit" or similar commands, since the CLI always reported "Multiple match" and aborted. Extract the matching logic into matchMirrorsByPattern and prefer a single case-insensitive exact match over partial substring matches, while keeping the existing substring-match behavior when no exact match exists. Fixes #134 This PR was written primarily by Claude Code; I reviewed the change and ran the tests before submitting. --- rpc/rpc.go | 30 +++++++++++++-- rpc/rpc_test.go | 98 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 rpc/rpc_test.go diff --git a/rpc/rpc.go b/rpc/rpc.go index 8955a0ad..ce9c36a1 100644 --- a/rpc/rpc.go +++ b/rpc/rpc.go @@ -129,18 +129,40 @@ func (c *CLI) MatchMirror(ctx context.Context, in *MatchRequest) (*MatchReply, e return nil, fmt.Errorf("can't fetch the list of mirrors: %w", err) } - reply := &MatchReply{} + reply := &MatchReply{ + Mirrors: matchMirrorsByPattern(mirrors, in.Pattern), + } + + return reply, nil +} + +// matchMirrorsByPattern returns the mirrors whose name contains pattern +// (case-insensitive). If exactly one mirror name matches pattern exactly +// (case-insensitive), only that mirror is returned, even if other mirror +// names also contain pattern as a substring. This allows a mirror whose +// name is a substring of other mirror names (e.g. "fcix.net" vs. +// "mirror.fcix.net") to still be matched unambiguously. +func matchMirrorsByPattern(mirrors map[int]string, pattern string) []*MirrorID { + lowerPattern := strings.ToLower(pattern) + var matches []*MirrorID for id, name := range mirrors { - if strings.Contains(strings.ToLower(name), strings.ToLower(in.Pattern)) { - reply.Mirrors = append(reply.Mirrors, &MirrorID{ + lowerName := strings.ToLower(name) + if lowerName == lowerPattern { + return []*MirrorID{{ + ID: int32(id), + Name: name, + }} + } + if strings.Contains(lowerName, lowerPattern) { + matches = append(matches, &MirrorID{ ID: int32(id), Name: name, }) } } - return reply, nil + return matches } func (c *CLI) ChangeStatus(ctx context.Context, in *ChangeStatusRequest) (*empty.Empty, error) { diff --git a/rpc/rpc_test.go b/rpc/rpc_test.go new file mode 100644 index 00000000..f061dec3 --- /dev/null +++ b/rpc/rpc_test.go @@ -0,0 +1,98 @@ +// Copyright (c) 2014-2019 Ludovic Fauvet +// Licensed under the MIT license + +package rpc + +import ( + "sort" + "testing" +) + +func names(mirrors []*MirrorID) []string { + var out []string + for _, m := range mirrors { + out = append(out, m.Name) + } + sort.Strings(out) + return out +} + +func TestMatchMirrorsByPatternExactMatchAmongSubstrings(t *testing.T) { + // Regression test for https://github.com/etix/mirrorbits/issues/134 + // A mirror whose ID is a substring of other mirror names could never + // be matched on its own, because every other mirror containing it as + // a substring would also show up as a match. + mirrors := map[int]string{ + 1: "fcix.net", + 2: "mirror.fcix.net", + 3: "paducahix.mm.fcix.net", + 4: "forksystems.mm.fcix.net", + } + + got := matchMirrorsByPattern(mirrors, "fcix.net") + want := []string{"fcix.net"} + + if gotNames := names(got); len(gotNames) != len(want) || gotNames[0] != want[0] { + t.Fatalf("matchMirrorsByPattern(%q) = %v, want %v", "fcix.net", gotNames, want) + } +} + +func TestMatchMirrorsByPatternExactMatchIsCaseInsensitive(t *testing.T) { + mirrors := map[int]string{ + 1: "FCIX.net", + 2: "mirror.fcix.net", + } + + got := matchMirrorsByPattern(mirrors, "fcix.net") + want := []string{"FCIX.net"} + + if gotNames := names(got); len(gotNames) != len(want) || gotNames[0] != want[0] { + t.Fatalf("matchMirrorsByPattern(%q) = %v, want %v", "fcix.net", gotNames, want) + } +} + +func TestMatchMirrorsByPatternMultipleSubstringMatches(t *testing.T) { + mirrors := map[int]string{ + 1: "mirror.fcix.net", + 2: "paducahix.mm.fcix.net", + } + + got := matchMirrorsByPattern(mirrors, "fcix.net") + want := []string{"mirror.fcix.net", "paducahix.mm.fcix.net"} + + gotNames := names(got) + if len(gotNames) != len(want) { + t.Fatalf("matchMirrorsByPattern(%q) = %v, want %v", "fcix.net", gotNames, want) + } + for i := range want { + if gotNames[i] != want[i] { + t.Fatalf("matchMirrorsByPattern(%q) = %v, want %v", "fcix.net", gotNames, want) + } + } +} + +func TestMatchMirrorsByPatternNoMatch(t *testing.T) { + mirrors := map[int]string{ + 1: "alpha", + 2: "beta", + } + + got := matchMirrorsByPattern(mirrors, "gamma") + if len(got) != 0 { + t.Fatalf("matchMirrorsByPattern(%q) = %v, want empty", "gamma", names(got)) + } +} + +func TestMatchMirrorsByPatternSingleSubstringMatch(t *testing.T) { + mirrors := map[int]string{ + 1: "mirror.example.com", + 2: "other.example.org", + } + + got := matchMirrorsByPattern(mirrors, "example.com") + want := []string{"mirror.example.com"} + + if gotNames := names(got); len(gotNames) != len(want) || gotNames[0] != want[0] { + t.Fatalf("matchMirrorsByPattern(%q) = %v, want %v", "example.com", gotNames, want) + } +} From a0e68cc62afffe58c85f7db179b17bbe7bdbc056 Mon Sep 17 00:00:00 2001 From: Amit Mishra Date: Sat, 4 Jul 2026 02:34:37 +0530 Subject: [PATCH 2/2] rpc: refactor tests to use sub-tests; fix copyright and issue URL - Convert 5 separate test functions for matchMirrorsByPattern into a single table-driven test using t.Run, as requested by reviewer. - Fix copyright header on rpc_test.go to reflect the actual author. - Correct regression test URL from etix/mirrorbits to videolabs/mirrorbits. - Adopt improved function comment wording suggested by reviewer. --- rpc/rpc.go | 12 ++-- rpc/rpc_test.go | 145 +++++++++++++++++++++++------------------------- 2 files changed, 75 insertions(+), 82 deletions(-) diff --git a/rpc/rpc.go b/rpc/rpc.go index ce9c36a1..d8dba9d3 100644 --- a/rpc/rpc.go +++ b/rpc/rpc.go @@ -136,12 +136,12 @@ func (c *CLI) MatchMirror(ctx context.Context, in *MatchRequest) (*MatchReply, e return reply, nil } -// matchMirrorsByPattern returns the mirrors whose name contains pattern -// (case-insensitive). If exactly one mirror name matches pattern exactly -// (case-insensitive), only that mirror is returned, even if other mirror -// names also contain pattern as a substring. This allows a mirror whose -// name is a substring of other mirror names (e.g. "fcix.net" vs. -// "mirror.fcix.net") to still be matched unambiguously. +// matchMirrorsByPattern returns a list of mirrors: +// - if the pattern matches a mirror's name exactly, only that mirror is returned +// - otherwise, all mirrors containing the pattern as a substring are returned +// - all matches are case-insensitive +// This allows a mirror whose name is a substring of other mirror names +// (e.g. "fcix.net" vs. "mirror.fcix.net") to still be matched unambiguously. func matchMirrorsByPattern(mirrors map[int]string, pattern string) []*MirrorID { lowerPattern := strings.ToLower(pattern) diff --git a/rpc/rpc_test.go b/rpc/rpc_test.go index f061dec3..94984ce5 100644 --- a/rpc/rpc_test.go +++ b/rpc/rpc_test.go @@ -1,4 +1,4 @@ -// Copyright (c) 2014-2019 Ludovic Fauvet +// Copyright (c) 2026 Amit Mishra // Licensed under the MIT license package rpc @@ -17,82 +17,75 @@ func names(mirrors []*MirrorID) []string { return out } -func TestMatchMirrorsByPatternExactMatchAmongSubstrings(t *testing.T) { - // Regression test for https://github.com/etix/mirrorbits/issues/134 - // A mirror whose ID is a substring of other mirror names could never - // be matched on its own, because every other mirror containing it as - // a substring would also show up as a match. - mirrors := map[int]string{ - 1: "fcix.net", - 2: "mirror.fcix.net", - 3: "paducahix.mm.fcix.net", - 4: "forksystems.mm.fcix.net", +func TestMatchMirrorsByPattern(t *testing.T) { + // Regression test for https://github.com/videolabs/mirrorbits/issues/134 + tests := []struct { + name string + mirrors map[int]string + pattern string + want []string + }{ + { + name: "exact match takes priority over substring matches", + mirrors: map[int]string{ + 1: "fcix.net", + 2: "mirror.fcix.net", + 3: "paducahix.mm.fcix.net", + 4: "forksystems.mm.fcix.net", + }, + pattern: "fcix.net", + want: []string{"fcix.net"}, + }, + { + name: "exact match is case-insensitive", + mirrors: map[int]string{ + 1: "FCIX.net", + 2: "mirror.fcix.net", + }, + pattern: "fcix.net", + want: []string{"FCIX.net"}, + }, + { + name: "multiple substring matches returned when no exact match", + mirrors: map[int]string{ + 1: "mirror.fcix.net", + 2: "paducahix.mm.fcix.net", + }, + pattern: "fcix.net", + want: []string{"mirror.fcix.net", "paducahix.mm.fcix.net"}, + }, + { + name: "no match returns empty", + mirrors: map[int]string{ + 1: "alpha", + 2: "beta", + }, + pattern: "gamma", + want: nil, + }, + { + name: "single substring match", + mirrors: map[int]string{ + 1: "mirror.example.com", + 2: "other.example.org", + }, + pattern: "example.com", + want: []string{"mirror.example.com"}, + }, } - got := matchMirrorsByPattern(mirrors, "fcix.net") - want := []string{"fcix.net"} - - if gotNames := names(got); len(gotNames) != len(want) || gotNames[0] != want[0] { - t.Fatalf("matchMirrorsByPattern(%q) = %v, want %v", "fcix.net", gotNames, want) - } -} - -func TestMatchMirrorsByPatternExactMatchIsCaseInsensitive(t *testing.T) { - mirrors := map[int]string{ - 1: "FCIX.net", - 2: "mirror.fcix.net", - } - - got := matchMirrorsByPattern(mirrors, "fcix.net") - want := []string{"FCIX.net"} - - if gotNames := names(got); len(gotNames) != len(want) || gotNames[0] != want[0] { - t.Fatalf("matchMirrorsByPattern(%q) = %v, want %v", "fcix.net", gotNames, want) - } -} - -func TestMatchMirrorsByPatternMultipleSubstringMatches(t *testing.T) { - mirrors := map[int]string{ - 1: "mirror.fcix.net", - 2: "paducahix.mm.fcix.net", - } - - got := matchMirrorsByPattern(mirrors, "fcix.net") - want := []string{"mirror.fcix.net", "paducahix.mm.fcix.net"} - - gotNames := names(got) - if len(gotNames) != len(want) { - t.Fatalf("matchMirrorsByPattern(%q) = %v, want %v", "fcix.net", gotNames, want) - } - for i := range want { - if gotNames[i] != want[i] { - t.Fatalf("matchMirrorsByPattern(%q) = %v, want %v", "fcix.net", gotNames, want) - } - } -} - -func TestMatchMirrorsByPatternNoMatch(t *testing.T) { - mirrors := map[int]string{ - 1: "alpha", - 2: "beta", - } - - got := matchMirrorsByPattern(mirrors, "gamma") - if len(got) != 0 { - t.Fatalf("matchMirrorsByPattern(%q) = %v, want empty", "gamma", names(got)) - } -} - -func TestMatchMirrorsByPatternSingleSubstringMatch(t *testing.T) { - mirrors := map[int]string{ - 1: "mirror.example.com", - 2: "other.example.org", - } - - got := matchMirrorsByPattern(mirrors, "example.com") - want := []string{"mirror.example.com"} - - if gotNames := names(got); len(gotNames) != len(want) || gotNames[0] != want[0] { - t.Fatalf("matchMirrorsByPattern(%q) = %v, want %v", "example.com", gotNames, want) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := names(matchMirrorsByPattern(tc.mirrors, tc.pattern)) + + if len(got) != len(tc.want) { + t.Fatalf("matchMirrorsByPattern(%q) = %v, want %v", tc.pattern, got, tc.want) + } + for i := range tc.want { + if got[i] != tc.want[i] { + t.Fatalf("matchMirrorsByPattern(%q) = %v, want %v", tc.pattern, got, tc.want) + } + } + }) } }