go-playground / validator

:100:Go Struct and Field validation, including Cross Field, Cross Struct, Map, Slice and Array diving
MIT License
16.14k stars 1.29k forks source link

Struct experiment #1150

Closed deankarn closed 10 months ago

deankarn commented 11 months ago

PR

This PR does the following:

coveralls commented 11 months ago

Coverage Status

coverage: 73.843% (-0.03%) from 73.872% when pulling e912ff3e72a4329b539bfcf26c5547e1639be971 on struct-experiment into 3094d596c834c13d04bf7523a35a06aaa9965928 on master.

MysteriousPotato commented 11 months ago

Would you consider adding an opt-in option to enable the required tag for structs?

This would prevent users that are already using the required tag on non-pointer structs (such as myself) from having to refactor their codebase as well as allowing users to use it without having to wait for the next major version.

Something like:

type Validate struct {
    //...
    requiredStructEnabled bool // new field
}

type ValidateOpt func(*Validate)

// Option to enable required tag on nested structs
func EnableRequiredStruct(v *Validate) {
    v.requiredStructEnabled = true
}

func New(opts ...ValidateOpt) *Validate {
    //...
    v := &Validate{
        //...
    }

    for _, opt := range opts {
        opt(v)
    }
    //...
}

func (v *validate) traverseField(ctx context.Context, parent reflect.Value, current reflect.Value, ns []byte, structNs []byte, cf *cField, ct *cTag) {
    //...
    if ct != nil && ct.tag == requiredTag && !v.v.requiredStructEnabled {
        ct = ct.next
    }
    //...
}
deankarn commented 10 months ago

Yep @MysteriousPotato totally an option :)

was meaning to ask what u think of this alt approach that will support or tag etc on structs.

will be a few days before I can finish adding the tests and docs back and then add that option.

MysteriousPotato commented 10 months ago

@deankarn I think this is clearly an improvement over my implementation.

Besides adding or support (which I totally overlooked) it also, among other things, makes the flow of traverseField easier to follow IMO.

One thing though. Looking a bit more into it, I think this may have changed the behavior of structonly for non-struct types where validateStruct would get called.

Not that it's a problem in and of itself since it doesn't make any sense to do that but considering the recent events, maybe adding some kind of check before calling validateStruct or returning an insightful error would be a good idea just in case?

Unrelated to this MR: While perusing the code, I noticed the structonly tag fails for nil struct pointers. Is this behavior intended?

deankarn commented 10 months ago

@MysteriousPotato

One thing though. Looking a bit more into it, I think this may have changed the behavior of structonly for non-struct types where validateStruct would get called.

Nice catch agreed, handled in this commit to preserve old behaviour. https://github.com/go-playground/validator/pull/1150/commits/771b7bfcb74074976a24b0f834062b14a8220207