fix: schema compatibility fingerprint #354
Conversation
|
@redaLaanait Any chance you could review this please? |
|
That's a considerable improvement! The last time, it seems I missed the big picture regarding the cache key's logic. Following a similar reasoning, we should also ensure that the resolved composite schema can't be a child node in a tree (that has a normal schema as a root node)? |
| } | ||
|
|
||
| func withWriterFingerprintIfResolved(fp [32]byte, isConv bool) SchemaOption { | ||
| return func(opts *schemaConfig) { |
There was a problem hiding this comment.
It would be nice to rename the second arg resolved instead of isConv.
| if !field.HasDefault() { | ||
| continue | ||
| } | ||
| defs = append(defs, field.Default()) |
There was a problem hiding this comment.
I saw that you did omit the field name key and FieldIgnore flag from the cachefingerprint. I couldn't find a counter-example where they are needed here as they already force changing the canonical fingerprint. So you're right!
schema_internal_test.go
Outdated
| }) | ||
| } | ||
| // func TestSchema_CacheFingerprint(t *testing.T) { | ||
| // t.Run("invalid", func(t *testing.T) { |
There was a problem hiding this comment.
My understanding is that this test suite is kind of redundant, abrogated by TestConfig_ReusesDecoders.
There was a problem hiding this comment.
Yes, as it is not dependent on the encodedType or anything like that, but rather the writer fingerprint.
I don't think this is needed. You cannot naturally get into this situation without making the schema like this manually, in which case there is not a lot we can do about it. If it turns out to be a legit problem, we can deal with it then IMO. |
|
LGTM 👍 |
When a schema contains a compatibility change anywhere in its tree, the entire schema should show the change in its fingerprint. To make this simpler, the writer fingerprint is carried in the schema, to show this change. There is an edge case in records that the exact defaults must be accounted for in the cached fingerprint.
This has the benefit of slightly optimising the planning phase.