Add Equal method for CrossSigningKey #444
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #444 +/- ##
==========================================
+ Coverage 59.68% 59.83% +0.15%
==========================================
Files 51 51
Lines 7158 7201 +43
==========================================
+ Hits 4272 4309 +37
- Misses 2489 2493 +4
- Partials 397 399 +2 ☔ View full report in Codecov by Sentry. |
| @@ -46,6 +47,49 @@ type CrossSigningKey struct { | |||
|
|
|||
| func (s *CrossSigningKey) isCrossSigningBody() {} // implements CrossSigningBody | |||
|
|
|||
| func (s *CrossSigningKey) Equal(other *CrossSigningKey) bool { | |||
There was a problem hiding this comment.
Why can't we just use reflect.DeepEqual here?
There was a problem hiding this comment.
This method is does not allocate at all, while reflect.DeepEqual does. (not that it matters much on that endpoint)
> go test -bench=Equal -run=^# -benchmem -count=10
goos: linux
goarch: amd64
pkg: github.com/matrix-org/gomatrixserverlib/fclient
cpu: 12th Gen Intel(R) Core(TM) i5-12500H
BenchmarkReflectDeepEqual-16 468159 2381 ns/op 288 B/op 14 allocs/op
BenchmarkReflectDeepEqual-16 460086 2376 ns/op 288 B/op 14 allocs/op
BenchmarkReflectDeepEqual-16 494133 2333 ns/op 288 B/op 14 allocs/op
BenchmarkReflectDeepEqual-16 496840 2338 ns/op 288 B/op 14 allocs/op
BenchmarkReflectDeepEqual-16 481978 2351 ns/op 288 B/op 14 allocs/op
BenchmarkReflectDeepEqual-16 461346 2327 ns/op 288 B/op 14 allocs/op
BenchmarkReflectDeepEqual-16 511146 2390 ns/op 288 B/op 14 allocs/op
BenchmarkReflectDeepEqual-16 501966 2443 ns/op 288 B/op 14 allocs/op
BenchmarkReflectDeepEqual-16 503883 2345 ns/op 288 B/op 14 allocs/op
BenchmarkReflectDeepEqual-16 503308 2415 ns/op 288 B/op 14 allocs/op
BenchmarkEqual-16 5371452 214.8 ns/op 0 B/op 0 allocs/op
BenchmarkEqual-16 5578748 227.6 ns/op 0 B/op 0 allocs/op
BenchmarkEqual-16 5169262 230.6 ns/op 0 B/op 0 allocs/op
BenchmarkEqual-16 5215892 234.2 ns/op 0 B/op 0 allocs/op
BenchmarkEqual-16 5207256 232.0 ns/op 0 B/op 0 allocs/op
BenchmarkEqual-16 5564870 244.5 ns/op 0 B/op 0 allocs/op
BenchmarkEqual-16 4907815 229.9 ns/op 0 B/op 0 allocs/op
BenchmarkEqual-16 5178279 233.5 ns/op 0 B/op 0 allocs/op
BenchmarkEqual-16 4840867 247.9 ns/op 0 B/op 0 allocs/op
BenchmarkEqual-16 5193268 215.0 ns/op 0 B/op 0 allocs/op
| return false | ||
| } | ||
| for i := range s.Usage { | ||
| if s.Usage[i] != other.Usage[i] { |
There was a problem hiding this comment.
Is this slice guaranteed to be ordered? If not you might get some false negatives.
There was a problem hiding this comment.
Fair point, but does usage currently even contain more than one value?
There was a problem hiding this comment.
Per spec no, but you want to be forwards compatible, otherwise you might need to prevent such cross signing keys from being uploaded.
| @@ -46,6 +47,49 @@ type CrossSigningKey struct { | |||
|
|
|||
| func (s *CrossSigningKey) isCrossSigningBody() {} // implements CrossSigningBody | |||
|
|
|||
| func (s *CrossSigningKey) Equal(other *CrossSigningKey) bool { | |||
There was a problem hiding this comment.
It would be nice to document what the comparison is taking into account, i.e. there can be different equality implementations depending on the use-case. I presume here we care about the fact that a new object needs to be stored in the DB and we test for equality of all fields.
There was a problem hiding this comment.
Exactly. The Equal method here checks all fields and receives the MasterKey, SelfSigningKeys and UserSigningKeys from Dendrite, see this (which should be much like element-hq/synapse#16943
| @@ -57,6 +58,14 @@ func (s *CrossSigningKey) Equal(other *CrossSigningKey) bool { | |||
| if len(s.Usage) != len(other.Usage) { | |||
| return false | |||
| } | |||
|
|
|||
| // Make sure the slices are sorted before we compare them. | |||
| if !slices.IsSorted(s.Usage) { | |||
There was a problem hiding this comment.
Not necessarily needed to check, but is slightly faster doing so.
Playing around with Copilot, code is mostly generated (but reviewed)
Also fixes CI by using the correct repository for Dendrite.