Added invalid serialization/unserialization detector #78
Conversation
There was a problem hiding this comment.
This seems reasonable (although, it's mildly unsatisfying that unserializing serialized input (or serializing unserialized input, whichever) doesn't produce the original value).
However, looking at how this is used in arcalot/arcaflow-engine#153, I don't see any compelling reason why this needs to be in a standalone module in the arcaflow-plugin-sdk-go repo. This could be inserted into workflow/workflow_test.go (or imported into it from an "adjacent" file) and referenced directly.
So, I would recommend against merging this PR.
| func (d InvalidSerializationDetectorSchema) ValidateCompatibility(_ any) error { | ||
| // Not applicable to this data type | ||
| return nil |
There was a problem hiding this comment.
If this function is not applicable, why does it return "no-error"? It seems like it should either return an error (claiming that it is not applicable and/or that it should not have been called) or it should panic.
There was a problem hiding this comment.
It just doesn't validate compatibility because it's out of scope of its purpose.
There was a problem hiding this comment.
So, the comment that this function is "not applicable to this data type" is not accurate...instead, it's more the case either that this data type is never incompatible or that we just don't care...and, regardless, the action here is to never return an error when the check is made.
|
Moved to arcalot/arcaflow-engine#153. |
Changes introduced with this PR
Fixes #63
In the past, bugs were introduced due to unserializing a type twice. There wasn't a way to detect that before. With the schema type added in this PR, it can just be added to a property to detect double serialization or unserialization.
This type is purely for detecting invalid uses.
By contributing to this repository, I agree to the contribution guidelines.