Validation Constraints on Optional Properties are enforced

[marstr]

As noted by @tombuildsstuff in Azure/azure-rest-api-specs#1324, when an optional parameter has a constraint, that constraint shouldn't be enforced when the value isn't provided at all.

I've tracked down this issue to here: https://github.com/Azure/go-autorest/blob/master/autorest/validation/validation.go#L105

I'm working on a solution now.

[marstr]

So, I may have picked out the wrong method as the problematic one above:
I wrote up a quick sample of using the reflect package: https://play.golang.org/p/rjrhpuT5me

There, it makes it seem that if you don't provide values for maxStalenessPrefix or maxIntervalInSeconds you dodge the issue of validating against 0 instead of nil; which was my initial concern. Since nothing is jumping out to me as a super obviously wrong, I'm going to have to build-out a repro and debug this.

[marstr]

Hey @tombuildsstuff, do you happen to able to point me at some code that already has this trouble? As far as I can tell, it shouldn't do validation on MaxStalenessPrefix and MaxIntervalInSeconds when they are nil.

[tombuildsstuff]

👋

Thanks for looking into this @marstr

There, it makes it seem that if you don't provide values for maxStalenessPrefix or maxIntervalInSeconds you dodge the issue of validating against 0 instead of nil; which was my initial concern. Since nothing is jumping out to me as a super obviously wrong, I'm going to have to build-out a repro and debug this.

I spent some time trying to put together some sample code to replicate this outside of the main repo and couldn't - so I took a fresh look at the code:

if stalenessPrefix, ok := input["max_staleness_prefix"].(int); ok {
	maxStalenessPrefix := int64(stalenessPrefix)
	policy.MaxStalenessPrefix = &maxStalenessPrefix
}

When I'd noticed the error (I was checking for the variables presence - but not to ensure it's not the default value) - I patched it to handle that and all is well:

if stalenessPrefix := input["max_staleness_prefix"].(int); stalenessPrefix > 0 {
	maxStalenessPrefix := int64(stalenessPrefix)
	policy.MaxStalenessPrefix = &maxStalenessPrefix
}

🤦 Apologies for wasting your time on user error 😞. As such I think we can shut this issue?

---

Having said that - the other issue in the Rest API Specs Repo still stands - in that it's not possible to set the maxStalenessPrefix or maxIntervalInSeconds fields unless the consistencyLevel is set to BoundedStaleness. If that's intentional, it'd be nice for the SDK to handle the validation for this if possible?

Thanks!

[marstr]

No worries, @tombuildsstuff! It was a good exercise for me to run through, definitely not a waste.

As you mention, I'll close this issue out for now. However, conditional parameters and validation do seem to be a problem. I'm going to meditate on that problem for a while and will file a bug if I think of anything actionable that we can do to improve our Swaggers or the runtime.