Add variants to AggregateType to support spec version 1.5 #681
Conversation
* change `Algorithm`, validation is similar as other enum types instead of raising exceptions * refactor construction of `Algorithm`, in tests as well * refactor `AggregateType` & expand variants, validated similar to `Classification` Signed-off-by: Sebastian Ziebell <sebastian.ziebell@ferrous-systems.com>
| @@ -709,7 +708,7 @@ mod test { | |||
| tools: None, | |||
| }), | |||
| }), | |||
| signature: Some(Signature::single(Algorithm::HS512, "abcdefgh")), | |||
| signature: Some(Signature::single("HS512", "abcdefgh")), | |||
There was a problem hiding this comment.
It would be nice to also have a constructor that accepts an enum instead of having to hardcode strings. That would surface errors at compile time, instead of at runtime during validation.
This applies to all similar enums with "unknown" variant. I assume this appears elsewhere in the codebase?
Do you think that is reasonable?
There was a problem hiding this comment.
I reverted the change for the model type.
There is a pattern for these enum types, where the Unknown variant functions as the catch all for all (yet) unknown values. The enum types in models are typically constructed via a new_unchecked constructor that maps the a string to its associated variant. An accompanying validation function raises a ValidationError in case it's the Unknown variant. It might be a good idea to check the code base that this pattern (or a better alternative) is consistently used.
There was a problem hiding this comment.
While I recognize the need for a conversion from a string, I would prefer to have the primary constructor to be used in the API to accept the enum type.
Incidentally, why not just make the conversion from an &str simply an impl From<&str>? I see that new_unchecked<A: AsRef<str>> is generic and works with String etc, but on the flip side it causes the function to be separately monomorphised for each type it accepts, increasing compilation time and binary size.
There was a problem hiding this comment.
I wanted to be consistent with the other enum types & how they use new_unchecked. It's not an ideal pattern to be used & there is room for improvement. I also removed the FromStr for Algorithm as this returns an Err rather than the Unknown variant.
My suggestion is to refactor this pattern to more idiomatic code in a separate PR. Instead of using new_unchecked all affected enum types should implement From<&str> then.
There was a problem hiding this comment.
Sounds good. I'll open an issue for that, and merge this as is for now.
Instead of passing in a `&str`, the `Algorithm` is used. Signed-off-by: Sebastian Ziebell <sebastian.ziebell@ferrous-systems.com>
All other `enum` related types use a `new_unchecked` constructor that does not raise an `Err` but rather collects the unknown value into an `Unknown` variant. Signed-off-by: Sebastian Ziebell <sebastian.ziebell@ferrous-systems.com>
This PR expands the
aggregateTypeenum type with a few more variants.Algorithm, validation is similar as other enum types instead of raising exceptionsAlgorithm, in tests as wellAggregateType& expand variants, validated similar toClassificationInvalidEnumVarianterror variant, validation is handled similar to other enum types