From 1dca082f37ac1ecb8c64133e236b33613444b23d Mon Sep 17 00:00:00 2001 From: Vincent Gao Date: Fri, 24 Jul 2026 19:39:19 +0200 Subject: [PATCH] fix: ISBN-10 Generate outer mod-11 and restrict X to check-digit position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs in the ISBN-10 implementation (isbn.go): 1. Generate: `return 11 - sum%11, nil` missed the outer `% 11` required by the ISO 2108 check-digit formula `(11 - (sum mod 11)) mod 11`. When the weighted sum is divisible by 11, Generate returned 11 instead of 0 — an out-of-range value (valid range is 0-10) that breaks the Generator contract and contradicts the library's own Verify, which accepts Verify("0000000000")=true (correct check digit 0). Fixed by applying the outer `% 11`: `(11 - sum%11) % 11`. Only the sum%11==0 boundary changes (11 -> 0); all other check digits 1-10 are unchanged. 2. Verify: the `case n == 'X': digit = 10` branch accepted 'X' (value 10) in any position. Per ISO 2108, 'X' is valid only as the check digit (the final character). Codes such as "X026515627" (X at position 0) and "0X00000009" (X at position 1) were falsely accepted. Fixed by rejecting 'X' in any non-final position. Valid ISBNs with 'X' in the last position (e.g. "000000006X") remain accepted. Adds isbn_edge_test.go with regression tests for both bugs (Generate sum%11==0 cases, non-zero regression guards, Verify X-in-wrong-position false-accepts, valid-X-last controls, and a Generate->Verify round-trip property over seeds including the sum%11==0 boundary). `go test ./...`, `go vet ./...`, and `gofmt -l .` all pass clean. --- isbn.go | 10 ++- isbn_edge_test.go | 191 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+), 2 deletions(-) create mode 100644 isbn_edge_test.go diff --git a/isbn.go b/isbn.go index e5b0e0b..b53f02a 100644 --- a/isbn.go +++ b/isbn.go @@ -12,10 +12,14 @@ func (i10 isbn10) Verify(code string) bool { } sum, multiply := 0, 10 - for _, n := range code { + for i, n := range code { var digit int switch { + // 'X' (value 10) is valid only as the check digit (final position). case n == 'X': + if i != len(code)-1 { + return false + } digit = 10 case isNotNumber(n): return false @@ -48,7 +52,9 @@ func (i10 *isbn10) Generate(seed string) (int, error) { multiply-- } - return 11 - sum%11, nil + // ISBN-10 check digit (ISO 2108): (11 - (sum mod 11)) mod 11, range 0-10. + // The outer mod 11 maps the sum%11==0 case to 0 instead of an out-of-range 11. + return (11 - sum%11) % 11, nil } // Verify implements checkdigit.Verifier interface. diff --git a/isbn_edge_test.go b/isbn_edge_test.go new file mode 100644 index 0000000..79046e7 --- /dev/null +++ b/isbn_edge_test.go @@ -0,0 +1,191 @@ +package checkdigit_test + +import ( + "fmt" + "testing" + + "github.com/osamingo/checkdigit" +) + +// TestIsbn10_Generate_Mod11Zero covers the sum%11==0 boundary where the +// check digit must be 0 (not the out-of-range 11 produced by the buggy +// `11 - sum%11` expression that missed the outer `% 11`). +func TestIsbn10_Generate_Mod11Zero(t *testing.T) { + t.Parallel() + + cases := map[string]struct { + in string + out int + }{ + "All zeros": { + in: "000000000", + out: 0, + }, + "Weighted sum divisible by 11": { + in: "055555555", + out: 0, + }, + } + + for name, c := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + r, err := checkdigit.NewISBN10().Generate(c.in) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if c.out != r { + t.Errorf("not equal, expected = %d, given = %d", c.out, r) + } + if r < 0 || r > 10 { + t.Errorf("check digit out of range [0,10]: %d", r) + } + }) + } +} + +// TestIsbn10_Generate_NonZeroRegression guards the non-zero check-digit +// cases (sum%11 != 0) that were already correct and must remain unchanged. +func TestIsbn10_Generate_NonZeroRegression(t *testing.T) { + t.Parallel() + + cases := map[string]struct { + in string + out int + }{ + "Check digit 9": { + in: "000000001", + out: 9, + }, + "Check digit 10 (X)": { + in: "000000006", + out: 10, + }, + "Arbitrary seed -> 10 (X)": { + in: "123456789", + out: 10, + }, + } + + for name, c := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + r, err := checkdigit.NewISBN10().Generate(c.in) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if c.out != r { + t.Errorf("not equal, expected = %d, given = %d", c.out, r) + } + }) + } +} + +// TestIsbn10_Verify_XWrongPosition covers the false-accept defect where +// 'X' (value 10) was accepted in any position. 'X' is valid only as the +// check digit (the final character) per ISO 2108. +func TestIsbn10_Verify_XWrongPosition(t *testing.T) { + t.Parallel() + + cases := map[string]struct { + in string + out bool + }{ + "X at first position": { + in: "X026515627", + out: false, + }, + "X at second position": { + in: "0X00000009", + out: false, + }, + } + + for name, c := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + ret := checkdigit.NewISBN10().Verify(c.in) + if c.out != ret { + t.Errorf("not equal, expected = %v, given = %v", c.out, ret) + } + }) + } +} + +// TestIsbn10_Verify_ValidXLastPosition confirms that a valid ISBN-10 with +// 'X' in the check-digit position is still accepted, plus basic +// valid/invalid controls. +func TestIsbn10_Verify_ValidXLastPosition(t *testing.T) { + t.Parallel() + + cases := map[string]struct { + in string + out bool + }{ + "Valid X in last position": { + in: "000000006X", + out: true, + }, + "Valid all zeros": { + in: "0000000000", + out: true, + }, + "Invalid check digit": { + in: "0000000001", + out: false, + }, + } + + for name, c := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + ret := checkdigit.NewISBN10().Verify(c.in) + if c.out != ret { + t.Errorf("not equal, expected = %v, given = %v", c.out, ret) + } + }) + } +} + +// TestIsbn10_RoundTrip checks the Generate -> Verify round-trip property +// over a range of seeds, including the sum%11==0 boundary cases. +func TestIsbn10_RoundTrip(t *testing.T) { + t.Parallel() + + prov := checkdigit.NewISBN10() + + seeds := []string{ + "000000000", // sum%11==0 -> check digit 0 + "055555555", // sum%11==0 -> check digit 0 + "000000001", // -> 9 + "000000006", // -> 10 (X) + "123456789", // -> 10 (X) + "002651562", // -> 8 + "007231592", // -> 10 (X) + "155860832", // -> 10 (X) + } + + for _, seed := range seeds { + t.Run(seed, func(t *testing.T) { + t.Parallel() + + cd, err := prov.Generate(seed) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + var code string + if cd == 10 { + code = seed + "X" + } else { + code = fmt.Sprintf("%s%d", seed, cd) + } + if !prov.Verify(code) { + t.Errorf("round-trip failed: Generate(%q)=%d -> Verify(%q)=false", seed, cd, code) + } + }) + } +}