Reject over-length codes in ABA, CUSIP, ISBN-10 and EAN-13 check digits#427
Conversation
|
Seems reasonable to me. I wonder if this should even be an (optional) feature of ModulusCheckDigit, as I suspect it's common for patterns that use a modulus check to have a constant length |
|
It could be optional but it seems it would only apply to 5 of the 11 subclasses. |
|
That matches what I found when I surveyed the package: only the five fixed-length routines would use a base-class guard (these four plus ISSN), while ISIN is length agnostic by design, Sedol already caps its length, and the rest are regex checked or variable length, which is why I kept it per routine. Thanks for merging this. |
|
Gary's count matches what I found when I swept the package: the fixed-length routines come out at five, these four plus ISSN from #426. ISIN is deliberately length agnostic (the 3133EHHF3 fixture from VALIDATOR-422 is a valid check digit on a non ISIN length), Sedol's calculateModulus already throws on anything over seven characters, CAS and EC are length checked by their regexes, and Luhn and Verhoeff are genuinely variable length. An optional expected-length field on ModulusCheckDigit (constructor argument, unset meaning no check) would fold the five overrides into one guard and give any future fixed-length routine the check for free, at the cost of a wider constructor surface for the six that don't want it. I can put that refactor together as a follow-up if that's the preferred shape. Thanks for merging this one. |
Following up on #426, I went back through the rest of the check digit package looking for the same length hole. ModulusCheckDigit.isValid never checks the code length, and calculateModulus weights each character by rightPos = length minus index, which does not move when a character is prepended. So the routines that weight purely on rightPos accept an over-length code whenever the prepended character lands on a no-op weight: ABANumberCheckDigit validates 0123456780, CUSIPCheckDigit validates 0037833100, and EAN13CheckDigit validates a genuine thirteen digit code carrying a leading zero. ISBN10CheckDigit is worse because it is modulus 11, so the leading position weight is a multiple of the modulus and any prepended digit is swallowed, for example 51930110995. Each of these singletons is a public entry point reachable without its wrapper validator, so a caller running a code straight through the routine treats the padded string as genuine.
The guard sits in each routine's isValid and rejects a code whose length is not the fixed length before the check digit test, the same shape merged for ISSNCheckDigit and the length guard already present in IBANCheckDigit. Keeping it in the routine covers the direct callers, not just the wrapper paths that already enforce length through CodeValidator. I deliberately left ISINCheckDigit alone since it is length agnostic by design (the 3133EHHF3 fixture from VALIDATOR-422 is a valid check digit on a non ISIN length), and SedolCheckDigit already caps its length; CAS, EC, Verhoeff and Luhn are either regex length checked or variable length and so are not affected. Each routine gets a regression test that fails before the guard and passes with it.