From string validation

[smyrman]

Resolves #71.

---

commit 06d3085 (HEAD -> from-string, smyrman/from-string)
Author: Sindre Myren sindre@clarify.io
Date: Thu Mar 10 19:39:00 2022 +0100

fix: let decode look for additional base32 padding

Update FromString and XID.TextUnmarshal so that it looks for discarded
bits in the final source character. This ensures that XIDs that have
been manually tampered with in a way that's ignored by base32 decode,
will not pass as valid.

commit f288272
Author: Sindre Myren sindre@clarify.io
Date: Fri Mar 4 09:13:06 2022 +0100

add benchmark and new failing test for FromString

commit 2171fc4
Author: Sindre Myren sindre@clarify.io
Date: Fri Mar 4 09:12:03 2022 +0100

error.go: make ErrInvalidID a const

Make ErrInvalidID a constant instead of a variable. This prevent it from
being changed by external packages; a behavior that although allowed by
the compiler, should probably be considered an invalid operation.

commit b64cbc7
Author: Sindre Myren sindre@clarify.io
Date: Fri Mar 4 09:09:13 2022 +0100

chore: address linter issues

Perform changes suggested by linter; no functional changes.

[smyrman]

go test -bench 'FromString*'
goos: darwin
goarch: amd64
pkg: github.com/rs/xid
cpu: Intel(R) Core(TM) i7-6567U CPU @ 3.30GHz
BenchmarkFromString-4    	83381887	        13.95 ns/op
BenchmarkFromString2-4   	57998780	        19.95 ns/op
PASS
ok  	github.com/rs/xid	4.594s
go test -bench 'FromString*'  16.73s user 0.52s system 278% cpu 6.191 total

[smyrman]

% go test -bench 'FromString*' -benchmem
goos: darwin
goarch: amd64
pkg: github.com/rs/xid
cpu: Intel(R) Core(TM) i7-6567U CPU @ 3.30GHz
BenchmarkFromString-4           76597575                14.00 ns/op            0 B/op          0 allocs/op
BenchmarkFromString2-4          59851599                20.47 ns/op            0 B/op          0 allocs/op
PASS
ok      github.com/rs/xid       3.378s
go test -bench 'FromString*' -benchmem  12.67s user 0.83s system 214% cpu 6.290 total

[smyrman]

Verifying that fromString2 actually solves #71:

go test -v -run 'TestFromString.*Quick.*'
=== RUN   TestFromStringQuick
    id_test.go:292: comparing XIDs:
        a: "c8gtev2sahsk17no3shg"
        b: "c8gtev2sahsk17no3shi" (index 19 changed to i)
    id_test.go:307: #1: failed on input xid.ID{0x62, 0x21, 0xd7, 0x7c, 0x5c, 0x54, 0x79, 0x40, 0x9e, 0xf8, 0x1f, 0x23}, 0x69
--- FAIL: TestFromStringQuick (0.00s)
=== RUN   TestFromString2Quick
--- PASS: TestFromString2Quick (0.00s)
=== RUN   TestFromString2QuickInvalidChars
--- PASS: TestFromString2QuickInvalidChars (0.00s)
FAIL
exit status 1
FAIL	github.com/rs/xid	0.293s
1 smyrman@patchi ~/Code/xid (git)-[from-strin

[smyrman]

I will clean up this PR if we decide that we want the semantics of fromString2 over FromString, and that we are happy with the performance implications.

[smyrman]

Updated benchmarks.

Before change

$ go test -bench '.*' -run '^$'
goos: darwin
goarch: amd64
pkg: github.com/rs/xid
cpu: Intel(R) Core(TM) i7-6567U CPU @ 3.30GHz
BenchmarkNew-4          	29175643	        40.74 ns/op
BenchmarkNewString-4    	19683991	        54.16 ns/op
BenchmarkFromString-4   	77936979	        14.54 ns/op
PASS
ok  	github.com/rs/xid	5.598s
go test -bench '.*' -run '^$'  20.10s user 0.54s system 320% cpu 6.441 total

New failing test:

$ go test
--- FAIL: TestFromStringQuick (0.00s)
    id_test.go:265: comparing XIDs:
        a: "c8l4djisahsnf5k88go0"
        b: "c8l4djisahsnf5k88goa" (index 19 changed to a)
    id_test.go:280: #1: failed on input xid.ID{0x62, 0x2a, 0x46, 0xce, 0x5c, 0x54, 0x79, 0x77, 0x96, 0x88, 0x44, 0x30}, 0x61
--- FAIL: TestFromStringQuickInvalidChars (0.00s)
    id_test.go:292: comparing XIDs:
        a: "c8l4djisahsnf5k88gvg"
        b: "c8l4djisahsnf5k88gvl" (index 19 changed to l)
    id_test.go:307: #15: failed on input xid.ID{0x62, 0x2a, 0x46, 0xce, 0x5c, 0x54, 0x79, 0x77, 0x96, 0x88, 0x44, 0x3f}, 0x6c
FAIL
exit status 1
FAIL	github.com/rs/xid	0.160s

After change

$ go test -bench '.*' -run '^$'
goos: darwin
goarch: amd64
pkg: github.com/rs/xid
cpu: Intel(R) Core(TM) i7-6567U CPU @ 3.30GHz
BenchmarkNew-4          	28826906	        40.04 ns/op
BenchmarkNewString-4    	20088812	        50.65 ns/op
BenchmarkFromString-4   	64955624	        17.20 ns/op
PASS
ok  	github.com/rs/xid	5.484s
go test -bench '.*' -run '^$'  19.65s user 0.95s system 237% cpu 8.678 total

Passing tests:

$ go test
PASS
ok  	github.com/rs/xid	0.279s

[rs]

LGTM. Do you need to do something more before merge?

[smyrman]

Do you need to do something more before merge?

Well, I have some questions.

1. I changed errors to be const instead of var -- this might be viewed as a breaking change (although I would argue that programs that change pacakge errors are already invalid). Is this a change that you want, or should I pull it out?

2. Do you want a separate error for invalid padding (currently in the PR), or should there just be one error for all invalid IDs?

[rs]

Const is fine and same error may be fine as well, I let you judge. I'm not sure a separate error is actionable.

[smyrman]

👍 I agree; I will remove the second error.

[smyrman]

Done & rebased.

[smyrman]

Fixed commit messages.

[smyrman]

Ready for merge 👍