protoc-gen-validate icon indicating copy to clipboard operation
protoc-gen-validate copied to clipboard

Add Mac address validation to string well-known format

Open ahetmanski opened this issue 7 years ago • 7 comments
trafficstars

This is feature request Add Mac address validation for string type using next Golang code.

import "net"
// func ParseMAC(s string) (hw HardwareAddr, err error)
if _, err := net.ParseMAC(macAddress); err != nil {
    return Error("Mac is invalid")
}

ahetmanski avatar Nov 01 '18 15:11 ahetmanski

Why not use the existing regular expression support?

akonradi avatar Nov 01 '18 15:11 akonradi

Sure, that's what I did as workaround. But due to your logic protoc-gen-validate library shouldn't implement any well-known formats validations. All of them could be done with regexp. Just remove all of them 😆 But it is not user-friendly I think 😄

I see next advantages from using golang standard library for mac validation:

  • Usage of net.ParseMAC() gives great improvement in code readability. If you write regexp with all checks from standard function regexp will look horrible.
  • Users would not implement this regexp again and again. This will lead to avoiding bunch of mistakes on users' side.
  • All validations will be implemented by proven function, which follows standards.

ahetmanski avatar Nov 01 '18 16:11 ahetmanski

I'm not opposed to including rules for well-known formats, my concern is about cross-language support. The Go standard library is a lot more comprehensive than the C++ standard library, which I don't believe includes any sort of ParseMAC-like utility function. To implement the validation in C++, we'd need to either depend on a library that includes such a function or write our own utility function. The problem with these is that either the library or utility function would have to be linked into every library that uses the validation code, regardless of whether or not the validation rules included MAC address validation. The problem gets worse as more languages are added (#108 adds Java as a third target language).

It might be possible to get the usability properties of saying something like (validation.rules).string.format = MAC_ADDRESS (not currently possible) in a more language-independent way by having a set of well-known regexes, where MAC_ADDRESS maps to the appropriate one. The language-specific backends would then insert these regexes in the correct places. That addresses your second point and maybe the third, though the regex behind the nice rule declaration would still be ugly.

akonradi avatar Nov 01 '18 17:11 akonradi

As @akonradi describes, this is actually a current problem with the well known string formats. The original set of them was taken from the JSON schema rules and implemented in Go using the advanced utilities of its standard library. I believe if you attempt to use most of them in C++ they'll currently blow up as unimplemented (or possibly be ignored, I don't remember).

I do believe the vast majority of them can be solved using regex (either explicitly by the end user or hidden away behind a well-known string rule); it's not too important to me how readable the generated code is, just that it is correct and is readable/ergonomic to apply the rule as an option in the IDLs.

rodaine avatar Nov 01 '18 18:11 rodaine

All that said, I'm not opposed to having this added.

rodaine avatar Nov 01 '18 18:11 rodaine

@akonradi, @rodaine Thank you guys for explanation!

Looks like this is a common issue with cross languages libraries. As a proposal I think that the good approach here is to have different documentation for each language with different well known validators support. If user wants to generate code with unsupported well known validator for some specific language, library could generate code which raises Not implemented error. Then you won't restrict of cool function usage in one language when it is not implemented in another. As well library developer will have opportunity to add unimplemented functionality support with regexp later for some specific language.

ahetmanski avatar Nov 02 '18 04:11 ahetmanski

If user wants to generate code with unsupported well known validator for some specific language, library could generate code which raises Not implemented error.

Yep! We do that already 😅 https://github.com/lyft/protoc-gen-validate/blob/master/templates/cc/register.go

rodaine avatar Nov 02 '18 19:11 rodaine