Skip to content

Commit

Permalink
Do not fail immediately on invalid row
Browse files Browse the repository at this point in the history
ProcessGeofeed func doesn't fail on the first invalid row, but
returns ErrInvalidGeofeed if invalid row was detected during
geofeed check.
If there is need to set a failure threshold, error should be
checked for being ErrInvalidGeofeed and then use CheckResult
for analysis.
  • Loading branch information
ikolesn committed Jul 13, 2023
1 parent 085c7a0 commit 9db1cc0
Show file tree
Hide file tree
Showing 14 changed files with 329 additions and 180 deletions.
12 changes: 9 additions & 3 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ type config struct {
gf string
db string
isp string
version bool
laxMode bool
}

Expand All @@ -50,6 +49,12 @@ func run() error {

c, diffLines, asnCounts, err := verify.ProcessGeofeed(conf.gf, conf.db, conf.isp, conf.laxMode)
if err != nil {
if errors.Is(err, verify.ErrInvalidGeofeed) {
log.Printf("Found %d invalid rows out of %d rows in total, examples by type:", c.Invalid, c.Total)
for invType, invMessage := range c.SampleInvalidRows {
log.Printf("%s: '%s'", invType, invMessage)
}
}
return fmt.Errorf("unable to process geofeed %s: %w", conf.gf, err)
}

Expand Down Expand Up @@ -92,7 +97,8 @@ func parseFlags(program string, args []string) (c *config, output string, err er
"/usr/local/share/GeoIP/GeoIP2-City.mmdb",
"Path to MMDB file to compare geofeed file against",
)
flags.BoolVar(&conf.version, "V", false, "Display version")
displayVersion := false
flags.BoolVar(&displayVersion, "V", false, "Display version")
flags.BoolVar(
&conf.laxMode,
"lax",
Expand All @@ -104,7 +110,7 @@ func parseFlags(program string, args []string) (c *config, output string, err er
return nil, buf.String(), err
}

if conf.version {
if displayVersion {
log.Printf("mm-geofeed-verifier %s", version)
//nolint:revive // preexisting
os.Exit(0)
Expand Down
144 changes: 0 additions & 144 deletions main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import (
"testing"

"github.com/stretchr/testify/assert"

"github.com/maxmind/mm-geofeed-verifier/v2/verify"
)

type parseFlagsCorrectTest struct {
Expand Down Expand Up @@ -137,145 +135,3 @@ func TestParseFlagsError(t *testing.T) {
)
}
}

type processGeofeedTest struct {
gf string
db string
dl []string
c verify.Counts
em string
laxMode bool
}

func TestProcessGeofeed_Valid(t *testing.T) {
goodTests := []processGeofeedTest{
{
gf: "test_data/geofeed-valid.csv",
db: "test_data/GeoIP2-City-Test.mmdb",
dl: []string{
"Found a potential improvement: '2a02:ecc0::/29",
"current postal code: '34021'\t\tsuggested postal code: '1060'",
},
c: verify.Counts{
Total: 3,
Differences: 2,
},
em: "",
laxMode: false,
},
{
gf: "test_data/geofeed-valid.csv",
db: "test_data/GeoIP2-City-Test.mmdb",
dl: []string{
"Found a potential improvement: '2a02:ecc0::/29",
"current postal code: '34021'\t\tsuggested postal code: '1060'",
},
c: verify.Counts{
Total: 3,
Differences: 2,
},
em: "",
laxMode: true,
},
{
gf: "test_data/geofeed-valid-lax.csv",
db: "test_data/GeoIP2-City-Test.mmdb",
dl: []string{
"Found a potential improvement: '2a02:ecc0::/29",
"current postal code: '34021'\t\tsuggested postal code: '1060'",
},
c: verify.Counts{
Total: 3,
Differences: 2,
},
em: "",
laxMode: true,
},
{
gf: "test_data/geofeed-valid-optional-fields.csv",
db: "test_data/GeoIP2-City-Test.mmdb",
dl: []string{
"Found a potential improvement: '2a02:ecc0::/29",
"current postal code: '34021'\t\tsuggested postal code: '1060'",
},
c: verify.Counts{
Total: 3,
Differences: 2,
},
em: "",
laxMode: false,
},
}

// Testing the full content of the difference explanation strings is likely to be
// tedious and brittle, so we will just check for some substrings.
for _, test := range goodTests {
t.Run(
test.gf+" "+test.db, func(t *testing.T) {
c, dl, _, err := verify.ProcessGeofeed(test.gf, test.db, "", test.laxMode)
assert.NoError(t, err, "processGeofeed ran without error")
for i, s := range test.dl {
assert.Contains(
t,
dl[i],
s,
"got expected substring: '%s', substring",
)
}
assert.Equal(t, test.c, c, "processGeofeed returned expected results")
},
)
}
}

func TestProcessGeofeed_Invalid(t *testing.T) {
badTests := []processGeofeedTest{
{
gf: "test_data/geofeed-invalid-missing-fields.csv",
db: "test_data/GeoIP2-City-Test.mmdb",
dl: []string{},
c: verify.Counts{},
em: "saw fewer than the expected 5 fields at line 1: '2a02:ecc0::/29,US,US-NJ,Parsippany'",
laxMode: false,
},
{
gf: "test_data/geofeed-invalid-empty-network.csv",
db: "test_data/GeoIP2-City-Test.mmdb",
dl: []string{},
c: verify.Counts{},
em: "line 2: network field is empty",
laxMode: false,
},
{
gf: "test_data/geofeed-invalid-network.csv",
db: "test_data/GeoIP2-City-Test.mmdb",
dl: []string{},
c: verify.Counts{},
em: "line 1: unable to parse network 2a02:/29: invalid CIDR address: 2a02:/29",
laxMode: false,
},
{
gf: "test_data/geofeed-valid-lax.csv",
db: "test_data/GeoIP2-City-Test.mmdb",
dl: []string{},
c: verify.Counts{},
em: "line 1: invalid ISO 3166-2 region code format in strict (default) mode, line: " +
"'2a02:ecc0::/29,US,NJ,Parsippany,'",
laxMode: false,
},
}

for _, test := range badTests {
t.Run(
test.gf+" "+test.db, func(t *testing.T) {
_, _, _, err := verify.ProcessGeofeed(test.gf, test.db, "", test.laxMode)
assert.EqualError(
t,
err,
test.em,
"got expected error: %s", test.em,
)
},
)
}
}
2 changes: 0 additions & 2 deletions test_data/geofeed-invalid-empty-network.csv

This file was deleted.

2 changes: 0 additions & 2 deletions test_data/geofeed-invalid-network.csv

This file was deleted.

40 changes: 40 additions & 0 deletions verify/errors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package verify

import "errors"

// ErrInvalidGeofeed represents error that is returned in case of incomplete compliance
// with RFC 8805 standards and the mode in which the program is run.
var ErrInvalidGeofeed = errors.New("geofeed does not comply with the RFC 8805 standards")

// RowInvalidity represents type of row invalidity.
type RowInvalidity int

// Invalidity types.
const (
FewerFieldsThanExpected RowInvalidity = iota
EmptyNetwork
UnableToParseNetwork
UnableToFindCityRecord
UnableToFindISPRecord
InvalidRegionCode
)

// String implements the Stringer interface.
func (ri RowInvalidity) String() string {
switch ri {
case FewerFieldsThanExpected:
return "FewerFieldsThanExpected"
case EmptyNetwork:
return "EmptyNetwork"
case UnableToParseNetwork:
return "UnableToParseNetwork"
case UnableToFindCityRecord:
return "UnableToFindCityRecord"
case UnableToFindISPRecord:
return "UnableToFindISPRecord"
case InvalidRegionCode:
return "InvalidRegionCode"
default:
return "UnknownInvalidityType"
}
}
File renamed without changes.
2 changes: 2 additions & 0 deletions verify/test_data/geofeed-invalid-empty-network.csv
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
2a02:ecc0::/29 ,US,US-NJ, Parsippany ,# valid row with difference from GeoIP2-City_test.mmdb
,,,,
2 changes: 2 additions & 0 deletions verify/test_data/geofeed-invalid-network.csv
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
2a02:/29,,,,
2a02:ecc0::/29 ,US,US-NJ, Parsippany ,# valid row with difference from GeoIP2-City_test.mmdb
File renamed without changes.
File renamed without changes.
File renamed without changes.
Loading

0 comments on commit 9db1cc0

Please sign in to comment.