Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No validations triggering with altNames across all fields on a top-level struct #1337

Open
2 tasks done
ivucica opened this issue Nov 30, 2024 · 0 comments
Open
2 tasks done

Comments

@ivucica
Copy link

ivucica commented Nov 30, 2024

  • I have looked at the documentation here first?
  • I have looked at the examples provided that may showcase my question here?

Package version eg. v9, v10:

v10

Issue, Question or Enhancement:

I'm creating structs from Protobufs. This means I cannot annotate them with custom tags -- at least not easily.

Therefore I extended the proto field descriptors with a custom option:

message UserFieldValidation {
  repeated string rule = 1;
}

extend google.protobuf.FieldOptions {
  optional UserFieldValidation user_field_validation = 5675474; // TODO: use a different option, this is just a mashed keyboard number
}

Then I annotated fields with rules:

message User {
  // ...
  optional
  string user_principal_name = 2 [(user_field_validation) = { rule: "required" } ];
}

Generated struct:

type User struct {
	UserPrincipalName *string `protobuf:"bytes,2,opt,name=user_principal_name,json=userPrincipalName" json:"user_principal_name,omitempty"`
}

This now allows me to generate the rules map:

		rules := map[string]string{}

		// Build rule map using proto reflect (e.g. "field_name": "min=4,max=6"),
		// joining all options on that field using a comma.
		/* unused: fileDesc */
		_, msgDesc := pbdescriptor.ForMessage(u)

		// Get all fields in the message.
		for _, fieldDesc := range msgDesc.Field {
			// Get extensions for this field, if any.
			ef, err := proto.GetExtension(fieldDesc.Options, pb.E_UserFieldValidation)
			if err != nil {
				// No validation rules set, continue to next field.
				mylog.Infof("no validation rules set for field %s", fieldDesc.GetName())
				continue
			}

			// Cast the extension.
			efv, ok := ef.(*pb.UserFieldValidation)
			if !ok {
				// Wrong type. Ignore.
				mylog.Errorf("wrong type for field %s", fieldDesc.GetName())
				continue
			}

			// Join all rules on this field using a comma.
			// We use proto field name, so we later need to register a function
			// to perform the mapping.
			rules[fieldDesc.GetName()] = strings.Join(efv.GetRule(), ",")
		}

Once the map is built using the Protobuf field names (such as user_principal_name rather than UserPrincipalName, the former of which I cannot trivially get from a Protobuf descriptor structure -- even though there's likely some utility function), I can easily register it:

		validateUser = validator.New()
                // calling RegisterTagNameFunc here
		validateUser.RegisterStructValidationMapRules(rules, pb.User{})

but I do it after I have registered a tag name func, just to be sure the tag names can be accessed if RegisterStructValidationMapRules needs them, with a preference for the proto field name over JSON name -- even though the JSON tag name without use of jsonpb should be matching the proto field name, so it should be the same:

		validateUser.RegisterTagNameFunc(func(fld reflect.StructField) string {
			jsonName := strings.SplitN(fld.Tag.Get("json"), ",", 2)[0]
			if jsonName == "-" {
				return ""
			}

			protoTag := fld.Tag.Get("protobuf")
			for _, val := range strings.Split(protoTag, ",") {
				// type,index,whetherOptional,name=actualName,...
				if strings.HasPrefix(val, "name=") {
					fmt.Printf("name for %q: %q\n", fld.Name, val)
					return val[len("name="):] // return proto name
				}
			}

			return jsonName
		})

Proto message structs are always being sent around as pointers to structs. However, to be sure, I invoked validate.Struct() on non-pointer: assuming var u *pb.User, I passed validate.Struct(*u).

I then followed the code path, and I landed here:

validator/validator.go

Lines 190 to 205 in 6c3307e

if ct == nil || !ct.hasTag || (isNestedStruct && len(cf.name) == 0) {
// isNestedStruct check here
if isNestedStruct {
// if len == 0 then validating using 'Var' or 'VarWithValue'
// Var - doesn't make much sense to do it that way, should call 'Struct', but no harm...
// VarWithField - this allows for validating against each field within the struct against a specific value
// pretty handy in certain situations
if len(cf.name) > 0 {
ns = append(append(ns, cf.altName...), '.')
structNs = append(append(structNs, cf.name...), '.')
}
v.validateStruct(ctx, parent, current, typ, ns, structNs, ct)
}
return
}

Essentially:

  • I have a field that has type *string in Go. (To preserve 'presence' information, I use the proto2 syntax, because I have not adopted new 'editions' APIs. Therefore, it gets generated as a pointer to a string, rather than a string)
  • This field has no validate tag.
  • There is a rules map which defines which validation options to apply for this field.
  • The altName for the field never gets used.
  • In fact, I don't seem to be hitting any codepath where the string gets accessed.
  • extractStructCache does seem to be building the mappings for my struct correctly (so, altName gets populated).

Is this as expected? I thought ExtractType etc would figure out that this is a string, despite it being a string-pointer, and I would expect mapping to happen using altName. However, I have not found what is the intended path for this.

It almost seems like this the block if ct.hasTag { is meant to cover this case, but obviously I have no recognized tag on my field:

validator/validator.go

Lines 120 to 169 in 6c3307e

if ct.hasTag {
if kind == reflect.Invalid {
v.str1 = string(append(ns, cf.altName...))
if v.v.hasTagNameFunc {
v.str2 = string(append(structNs, cf.name...))
} else {
v.str2 = v.str1
}
v.errs = append(v.errs,
&fieldError{
v: v.v,
tag: ct.aliasTag,
actualTag: ct.tag,
ns: v.str1,
structNs: v.str2,
fieldLen: uint8(len(cf.altName)),
structfieldLen: uint8(len(cf.name)),
param: ct.param,
kind: kind,
},
)
return
}
v.str1 = string(append(ns, cf.altName...))
if v.v.hasTagNameFunc {
v.str2 = string(append(structNs, cf.name...))
} else {
v.str2 = v.str1
}
if !ct.runValidationWhenNil {
v.errs = append(v.errs,
&fieldError{
v: v.v,
tag: ct.aliasTag,
actualTag: ct.tag,
ns: v.str1,
structNs: v.str2,
fieldLen: uint8(len(cf.altName)),
structfieldLen: uint8(len(cf.name)),
value: getValue(current),
param: ct.param,
kind: kind,
typ: current.Type(),
},
)
return
}
}

Do I have to fall back to using VarWithValue or similar? At this point, since I have spent so much time with proto descriptors and reflections, this would not be a problem. Or I could do something horrifying like serialize the pb.User proto.Message into JSON, then deserialize that into a map[string]interface{} and pass that through the validator, but that seems just all sorts of horrible.

But, I sort of expected that a not-specially-tagged struct with *string fields could have rules defined in the map, and then validated using ValidateStruct. Is this not intended?

Code sample, to showcase or reproduce:

Due to complexity of providing instructions on writing a complete Protobuf file and on generating it in Go, this is omitted here, aside from explanation above. This is especially so since I am not currently using latest Protobuf APIs, so I'd have to be careful about specifying exact versions. (In fact, the whole contraption is sort of going away with newer proto API, since structs are not directly exposing fields anymore for performance reasons; now fields are obtained with getters.)

The minimum case involves simply not defining validate:"a,b,c" on fields, possibly defining someunrelatedtag:"one,two", registering the mapping + the lookup function (so altName gets generated), and seeing it not used during validate.Struct(exactType).

Code for this is not publicly available at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant