Automatically execute middleware validations when configuration struct uses custom validation

[hariso]

Feature description

It's basically this: ConduitIO/conduit-connector-postgres#247.

If a configuration structs has custom validation (i.e. implements the Validate(ctx) method), then the SDK does not call the Validate() methods in the middleware. Because of that, a custom Validate(ctx) always needs to call DefaultXYZMiddleware.Validate() explicitly. It's easy to forget, but the SDK should be able to do automatically. There doesn't seem to be a case where a developer would like to override or completely skip the middleware validation.

[lovromazgon]

Consider the following example:

type CommonConfig struct {
	URL string `json:"url" validate:"required"`
}

func (cfg CommonConfig) Validate(ctx context.Context) error {
	_, err := url.ParseRequestURI("http://google.com/")
	if err != nil {
		return err
	}
	return nil
}

type SourceConfig struct {
	sdk.DefaultSourceMiddleware
	CommonConfig
	
	FetchInterval time.Duration `json:"fetchInterval" validate:"required"`
}

func (cfg SourceConfig) Validate(ctx context.Context) error {
	var errs []error
	
	// Should this be called?
	if err := cfg.CommonConfig.Validate(ctx); err != nil {
		errs = append(errs, err)
	}
	// While this should not be called?
	if err := cfg.DefaultSourceMiddleware.Validate(ctx); err != nil {
		errs = append(errs, err)
	}
	
	if cfg.FetchInterval == 2 {
		errs = append(errs, fmt.Errorf("fetchInterval can not be 2 for some reason"))
	}
	
	return errors.Join(errs...)
}

I'm not sure what the expected behavior is. On the one hand, letting the user call Validate can be a bit of a footgun, if they don't forward the call to the middleware object. On the other hand, maybe they expect that Validate will only be called on the SourceConfig and actually forward the calls, then the validation will be executed twice for nested structs. In the best case it would result in duplicated errors, in the worst case it would result in unexpected behavior (if the validate function is not idempotent, but actually changes some things and does so twice).

[hariso]

There are I think two separate things, which may or may not need to be solved in the same way. One is the embedded configurations that a user wrote, and the other is the SDK middleware. For the latter one I, as a user, expect that the SDK middleware validation is called. For the CommonConfig in this example, I'm not quite sure either, but I tend to say that I, as a user, am responsible for the code I wrote. The SDK needs to know about one entry point, and one Validate() method. Whether I implement it in one struct/method or split across multiple is more of an implementation detail. Plus, as you pointed it can happen that some validations are called twice. The order might play a role too (e.g. SourceConfig relies on something from CommonConfig) and so on.

Long story, short: I would expect the SDK validations to be automatically called, and I tend to say that the SDK should call just one Validate() method.

[lovromazgon]

When I was working on this, I was playing around with what you're proposing. I went as far as to see if we can even figure out if Validate is manually implemented by the user or if it's inherited from sdk.DefaultSourceMiddleware. It wasn't trivial so I posed a question in the Gophers Slack. Turns out it's not possible to figure this out, the information is simply lost when the code gets compiled (see this message).

To sum up - we can't know if the call to SourceConfig.Validate() will be directly forwarded to the middleware config, or if there is a manual implementation on SourceConfig. This means that we need to always call SourceConfig.Validate(), just in case the user manually implemented it. If we then use reflection to find the embedded middleware struct and call Validate on it, it's possible that we will be executing the same validation twice. We could get around this by deduplicating errors returned by the middleware config, but the bigger issue to me is that it feels shady, it's adding even more magic to the config parsing mechanism, which is already "magic" for Go's standards.

I agree with the notion that this poses a possible footgun though. I'm thinking, maybe we could have a custom linter that complains if you overwrite Validate and don't forward the call to any embedded struct that also implements the same method.

---

Hmm, let me rethink this. If we can identify the errors from the middleware (e.g. with a specific error type), we could first call SourceConfig.Validate() and if we spot a middleware error in the response, we can safely assume that the middleware was validated, as only middleware would produce this error type. If, however, we don't find a middleware error, the middleware is either valid or it wasn't validated at all. In that case, we can safely validate the middleware using reflection, just to be sure. In the worst case, we will be validating the same thing twice, but it's not in a performance-critical path, so no harm is done, while in the best case, we are actually validating the middleware, which the user forgot to do. So yeah, maybe this is the way to go.