The behavior or `required` tag with empty structures was changed

[TelpeNight]

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

Package version: v10.15.0

Issue:

The behavior or required tag with empty structures was changed. In v10.14.1 the code below passes validation just fine.
Starting from version v10.15.0 it fails with error:

Key: 'OuterType.Component' Error:Field validation for 'Component' failed on the 'required' tag

It would be nice not to break backward compatibility. I know that this is a rare case, but we currently have problems with some autogenerated code. This is also a possible issue for some usecases of go generics.

Code sample, to showcase or reproduce:

func TestRequiredEmptyComponent(t *testing.T) {
	v := validator.New()
	type Component struct{}
	type OuterType struct {
		Component Component `validate:"required"`
		Other     int
	}
	var val OuterType = OuterType{
		Component: Component{},
		Other:     1,
	}
	if err := v.Struct(val); err != nil {
		t.Error(err)
	}
}

[MysteriousPotato]

First off, sorry to hear that, I know how frustrating unexpected breaking changes can be.

Just to clarify a bit, the behavior you mentioned was previously undefined since it was not implemented.
As of v.10.15.0, changes were made to implement this functionality. I did not expect this to cause breaking changes.

I'm not sure how feasible it would be for you, but removing the validation tags altogether would replicate the behavior seen in the previous versions.

[deankarn]

Yes sorry to hear that it has caused you issues.

The new logic though in this case has corrected the functionality previous to v10.15.0. It should have been failing before given the definition of required which validates if a value is not its default value which Component is in this case.

Along with @MysteriousPotato ‘s suggestion you could also make Component a pointer in this case if set which should also pass the validation.

Out of curiosity what value is Component the way it is defined? Or is it simply as you stated and issue with some code generation somewhere?

I would also be curious to learn about some of the generic use cases this could be an issue for, are you able to provide an example?

[asv]

Example real production code which broken after updating to 10.15.x:

type CutOff1 struct {
	DaysBefore int
}

type DeliveryDay1 struct {
	CanDeliver bool
	CutOff     *CutOff1 `json:"cutOff,omitempty" validate:"omitempty,dive"`
}

type DeliveryDays1 struct {
	Monday DeliveryDay1 `json:"mon" validate:"required,dive"`
}

func TestRequiredEmptyComponent(t *testing.T) {
	v := validator.New()
	val := DeliveryDays1{
		Monday: DeliveryDay1{
			CanDeliver: false,
		},
	}
	if err := v.Struct(val); err != nil {
		t.Error(err)
	}
}

Because CanDeliver is false (empty value for golang type bool), validator thinks the whole structure is empty.

[phoenix2x]

Hi guys, we're facing the same issue on v10.15.0. A lot of our code fails validation of http request bodies when body end up being equal to zero value. Here is our simplified example:

vdt := validator.New()
var req struct {
	UserID string `header:"X-User-Id" validate:"required"`
	Body   struct {
		BooleanUserProperty bool `json:"boolean_user_property,omitempty"`
	} `request:"body" validate:"required"`
}

// here we parse http request into struct
req.UserID = "asd"
req.Body.BooleanUserProperty = false

if err := vdt.StructCtx(ctx, req); err != nil {
	panic(err)
}

We use validation like this in all our services, so it will be a big lift for us to fix that on our side.

[MysteriousPotato]

@deankarn I'm not sure if this alone warrants a major version upgrade, but we could revert #1122 and include it in the next version.

A few users have already encountered this issue and I fear this may just be the tip of the iceberg.
This might create a bit of confusion among the early adopters of the new feature, but it may be a better alternative than introducing a breaking change.

I'd be willing to tackle the task if you agree with the proposition.
This would include adding clarifications and examples on how to use tags with nested structs.

I believe the issue raised by @TelpeNight regarding generics can be illustrated as follows:

func main() {
	type Option[T any] struct {
		V T `validate:"required"`
	}

	v := validator.New()
	if err := v.Struct(Option[struct{}]{}); err != nil {
		// Did not return an error prior to v10.15.0
	}
}

[deankarn]

Hi all, first off I want to say I'm sorry this is causing so many issues and all options are on the table.

I also think it's important to all be on the same page about the changes implemented in order to have an informed discussion before deciding on next actions.

let's start with the definition of required:

This validates that the value is not the data types default zero value. For numbers ensures value is not zero. 
For strings ensures value is not "". 
For slices, maps, pointers, interfaces, channels and functions ensures the value is not nil. 
For structs ensures value is not the zero value.

So required checks if something is it's default value in Go is the raw takeaway.

The behaviour of required wasn't actually changed in #1122. Yes a couple of lines here and here were updated to be more succinct, but are a no-op change and do not change the behaviour.

Why are people having these issues now after #1122?
Your defined validations weren't actually being run before this fix. Yes regrettably so, these validations were not even being attempted to be run and would reach this code here diving right into the structs fields not checking anything at the struct level.

To summarize so far
#1122 corrected the behaviour of the validator package and peoples expectations of how it worked.

For @asv 's, @phoenix2x's, @TelpeNight and @jonathan-innis' here examples are actually being applied as defined.

My hesitation in reverting this is that it hides some very nasty bugs in others code, whereby it appears to be working correctly but in fact is not working at all. An example of this is @jonathan-innis' issue which boils down to this line here where it's trying to validate a minimum 15 minute value of a custom duration type. Although it has caused it to start failing in a noticeable way, that's actually probably a good thing because the validation wasn't working at all before, likely causing undefined behaviour within the application/system itself potentially causing far worse issues.

options + pros & conns

1. I could revert that specific change and cut a new major version.

• pro - less work for community to correct a bunch of code.
• conn - continues to hide and not correctly evaluate struct validations.
• conn - can break code people have written since merging the change.

2. Leave it in place as it is a correction of behaviour.

• pro - corrects unexpected behaviour of this package
• pro - now highlights issues in codebases where logic appears to be working but is in fact not working at all. Super inconvenient, however will lead to correct behaviour of applications.
• conn - causes the community work to correct the incorrect validations defined that were not being applied before.

Which is the lesser of two evils? IMO undefined behaviour is far worse and I'd lean towards option 2 because of that.

Though experiments
For option 2 above, although not fun nor easy to correct all that logic, if they remain on version v10.14.1 then everything operates as before and have ample time to correct before upgrading to v10.15.x+

For option 1 above if I cut a new release, I then will put the v10 series into essentially archive mode in order to concentrate on v11+ series because along with the very generous help of some key people like @MysteriousPotato & @robinlieb (sorry if missing anybody) am the sole maintainer of this entire org and unfortunately demands on my time formidable.
As a result to get any new fixes or new functionality would have to be applied to the v11 series going forward causing at minimum just as much work as option 2, likely far more having to update all import paths also.

Final thoughts
Thank you for bearing with this long post and again all options still on the table for discussion but am clearly leaning today with keeping the changes as those nasty bugs being hidden are so bad.

I for one have had my code broken by Go STD library behavioural changes many many times and so understand and feel your pain.

[asv]

Hi all, first of all, thank you for your work, go-validator is a wonderful library.

But I see a problem here:

1. Naming problem. I'm sure 99% of newbies expect that required means "absolutely necessary", so maybe in v11 rename it to notempty / notblank / etc?
2. Compatibility problem. It's not so much important whether required worked correctly or not before, it's more important that a very large number of projects relied on this wrong behavior. So the right thing to do was to release v11 in this case, if there is even the slightest possibility of breaking other people's programs.

I'm not aware of it, but what is the problem to implement "fair required"? Any tickets, documentation explaining the problem?

[deankarn]

Hey @asv thanks you for discussing :)

Naming problem. I'm sure 99% of newbies expect that required means "absolutely necessary", so maybe in v11 rename it to notempty / notblank / etc?

Maybe that's true, naming is hard. But will also say in v8 there was an exists and it was changed to required because of the opposite argument and confusion. In either case this discussion really isn't about the name specifically.

Compatibility problem. It's not so much important whether required worked correctly or not before, it's more important that a very large number of projects relied on this wrong behavior.

I would take the opposite side of the argument and would say it is important for a few reasons:

1. There were other validations tags not correctly working prior to this change.
2. The behaviour of pointer structs and non-pointer structs was not consistent for required. One got validated, the other didn't, when it should have been.
3. a very large number of projects relied on this wrong behaviour. As shown this was very bad in @jonathan-innis' example, where it was assumed to be working, but in fact wasn't being validated at all.
4. Clearly a lot of people were adding validations to struct fields expecting it to work, whether they were correct validations defined or not, their expectations were not being met as they were not being run at all.

Yes the behaviour is different in this one specific way, but it comes in the form of a correction to validation behaviour and not an unintended change.

So the right thing to do was to release v11 in this case, if there is even the slightest possibility of breaking other people's programs.

I very much wish this statement was always true, unfortunately though corrections happen to code all the time which changes behaviour, in the form of a correction.

Just because behaviour could change in anyones code is not 100% cut and dry must make a major version bump, here's one example: #1146 which corrected how floats were parsed, which was a behavioural change which could cause some people programs to change behaviour and now fail validations, however they will now be correctly failing.

I see this correction to be very similar to the above because it's corrected calling the validations that were asked for, the difference is it's at a higher level and applies to more than just one validation.

I'm not aware of it, but what is the problem to implement "fair required"? Any tickets, documentation explaining the problem?

I am unsure what you mean here? The only thing I can think of is that in Go there is no concept of an uninitialized variable/value as in C.

Again still thinking about all of this and considering everything.

[phoenix2x]

Hi guys,

Just want to add our opinion here.

The problem with Option 2 is that it introduces a significant risk of producing bugs in existing projects. The change is very subtle so it's not easy to catch it with tests, in our case it was not found by unit tests or integration tests. So the question is do we want to sneakily break a potentially large number of projects that were working before and fix some number of projects that never worked? Or do we want to cut a Major version with big patch notes that explain the behavior and allow people to be mindful about this upgrade? I personally don't see any drawbacks with a Major version except that it might take more time for some people to adopt it, which should be fine I suppose.

Thank you for maintaining the library:)

[deankarn]

Hey all, been thinking and losing a lot of sleep over this and think we can meet in the middle with a compromise.

I've created this POC PR, still need to add back a few tests and docs but mostly there, which re-implements the struct level validations in a different way that supports more built-in tags.

The gist of it being I've added a special case which will ignore the required tag on non-pointer structs as before the change, however still run and fail on other validations.

Reasoning is I can understand and appreciate the misuse of the required tag, and don't want to break a ton of existing code. However, I also don't want to allow/hide failed validations on structs that were not being run before leading to undefined behaviour such as in @jonathan-innis' case which is a fix in my eyes.

I'd like to hear your thoughts on this compromise as I don't wish to release a new Major version at this time.

[deankarn]

Hey all,

PR is now ready for review, tests, docs, etc.. all added back and even new way to opt-in to nee behaviour #1150

Going to leave the PR open for your, or anyones, comment for a few days but then will merge if there's no issue found.

[bradleygore]

Hi @deankarn I think I have a related issue - but may be stemming from same thing, not quite sure. I'm happy to write as separate issue if needed. It looks like the v10.15.2 update is not handling []byte type (or possibly just alias for that type) that doesn't even have validation tags on it. Check this out:

Describe("V10.15 bug", func() {
	type Thing struct {
		Name  string `validate:"required"`
		Token uuid.UUID
	}

	It("should not error on validation", func() {
		var err error
		Expect(func() {
			err = validate.Struct(Thing{Name: "test"})
		}).NotTo(Panic())
		Expect(err).To(BeNil())
	})
})

results in output:

V10.15 bug [It] should not error on validation
  [FAILED] Expected
      <func()>: 0x1053bc9b0
  not to panic, but panicked with
      <*reflect.ValueError | 0x1400000dbf0>: 
      reflect: call of reflect.Value.Type on zero Value
      {
          Method: "reflect.Value.Type",
          Kind: 0,
      }
  In [It] at: /path/to/the_test.go:28 @ 08/29/23 16:42:47.511

I can work around this by explicitly using validate:"-" tag, but since there is no validate tag on this field at all, should it even try to delve into its type info?

[MysteriousPotato]

@bradleygore I tried to reproduce it using both []byte and an alias and could not.

I think it's unlikely that this is related to the new changes. Maybe a new issue would be better.

Is it possible you're using RegisterCustomTypeFunc on uuid.UUID and the CustomTypeFunc somehow returns nil?

Edit: Turns out it was related to the new changes. I did not realize returning nil was a valid thing to do.

[bradleygore]

@MysteriousPotato I didn't even think of the custom TypeFunc registration. You're right, we do have one - and it does return nil in a specific scenario:

validCop.RegisterCustomTypeFunc(func(field reflect.Value) interface{} {
	if uid, ok := field.Interface().(uuid.UUID); ok {
		if uid == uuid.Nil {
			return nil // <--------
		}
		return uid.String()
	}
	return field
}, uuid.New())

It has been working with the nil value up until now, however if that was kind of an unintended usage I'm happy to adjust it. I should likely just return empty string for that case as an empty string would fail the required tag (I think that's the only tag we use on uuid fields). I'll do some more with this tomorrow within the codebase in question and see if anything pops up with the empty string approach.

Thanks for taking the time to help think this through! 😃