Improve protovalidate overloads #36
Conversation
Update all of the protovalidate overload functions to add proper type checking and return Err.noSuchOverload if the types are incorrect. Greatly simplify the unique implementation as it didn't need to support non-list arguments. Fix a bug in the unique implementation for bytes (if the bytes contained invalid UTF-8). Add testcases for unique and additional tests for invalid arguments. Update Format.format to use exceptions for control flow instead of having to distinguish Val.Err from other types (a lot of places were just returning null). Remove all places where we used '==' to compare object type instances with comparing using enum types (which is safe for '==' comparison).
| try { | ||
| return StringT.stringOf(Format.format(formatString, list)); | ||
| } catch (Err.ErrException e) { | ||
| return e.getErr(); |
There was a problem hiding this comment.
Updated this to use exceptions for control flow instead of dealing with interpreting the returned Val type (either Err or String).
| case Int: | ||
| case String: | ||
| case Uint: | ||
| return unaryOpForPrimitiveVal(val).invoke(val); |
There was a problem hiding this comment.
A lot of these weren't valid for unique() - it should only operate on list types according to the Go implementation.
| Val val = list.get(IntT.intOf(i)); | ||
| if (exist.contains(val)) { | ||
| if (!exist.add(val)) { |
There was a problem hiding this comment.
The add() method returns false if the value is already contained in the set.
| for (int i = 0; i < list.size().intValue(); i++) { | ||
| Object val = list.get(IntT.intOf(i)).value(); | ||
| if (val instanceof byte[]) { | ||
| val = new String((byte[]) val, StandardCharsets.UTF_8); |
There was a problem hiding this comment.
This was the source of a bug if the bytes contained invalid UTF-8. If so, the string would contain replacement characters for invalid bytes and two different bytes values would return equal.
| return Err.newErr("format: unparsable format specifier %s", c); | ||
| } | ||
| if (status.type() == Err.ErrType) { | ||
| return status; |
There was a problem hiding this comment.
A lot of the complexity in here went away with switching to exceptions for control flow.
| // Previously, the unique() method returned false here as both bytes were converted | ||
| // to UTF-8. Since both contain invalid UTF-8, this would lead to them treated as equal | ||
| // because they'd have the same substitution character. | ||
| .put("[b'\\xFF', b'\\xFE'].unique()", true) |
There was a problem hiding this comment.
This failed before fixing the unique implementation (this test doesn't exist in the Go implementation but it doesn't suffer the same issues).
Update all of the protovalidate overload functions to add proper type checking and return Err.noSuchOverload if the types are incorrect. Greatly simplify the unique implementation as it didn't need to support non-list arguments. Fix a bug in the unique implementation for bytes (if the bytes contained invalid UTF-8). Add testcases for unique and additional tests for invalid arguments.
Update Format.format to use exceptions for control flow instead of having to distinguish Val.Err from other types (a lot of places were just returning null).
Remove all places where we used '==' to compare object type instances with comparing using enum types (which is safe for '==' comparison).