fix: add versioning for trigger encoding #2561
Conversation
| if err != nil { | ||
| return err | ||
| } | ||
| if v2.valid() { |
There was a problem hiding this comment.
Is it expected to fail silently (returning nil) if both .valid() methods return false?
There was a problem hiding this comment.
Good catch. I added an error here
There was a problem hiding this comment.
Actually, this breaks a lot of things where the triggers are not completely defined, like tests with empty triggers.
The error should be returned if there's a problem parsing the json string, like invalid characters etc. The valid method is only here to check if the versions match, but if no version match, it doesn't mean that the JSON is invalid.
Since we cannot accurately detect the difference between an incompatible encoding and an actual error, doing so mixes unmarshalling and validation, and it's not even a solid validation.
The situation where we fail to unmarshall valid data should only happen during development, and mainly when working with encodings (as this PR does). So, failing silently is probably not THAT bad.
This PR fixes a retrocompat issue, where tests created before this version would fail to be correctly read from DB. This was caused by a change in the JSON encoding of the
model.Triggerentity.This PR fixes this by adding basic versioning support for the entity
Checklist