ReferenceChecker don't get passed in a propper context

[smyrman]

Code:

rest-layer/resource/index.go

Line 132 in 6d75064

_, err = rsc.Get(context.TODO(), id)

The ReferenceChecker does not get passed in a request-scoped context (or any context for that matter), which means that any hooks that might run OnGet / OnGot for the resource in question don't get passed the relevant request context.

use-cases

There are many good use-cases for wanting to access the correct context in a hook.

One use-case, is permission handling, when it's reasonable to assume that the context may contain some user or permission related information.

Fix: pass in context to FieldValidator

The obvious fix may be to change the shema.FieldValidator interface to accept a context to be passed to the validator function. This is possile to do as a backwards-compatible change as well, as one could add a new interface, schema.CtxFieldValidator or so, to take precedence over schema.FieldValidaotor in type checks, but not sure the backwards-compatible path here is worth it before the v1.0 release.

There may be other changes that could be done to pass along the right context that isn't yet clear to me, but having the opurtunity to pass in context to a FieldValidator, probably makes senes anyway...

[Dragomir-Ivanov]

@smyrman I think backward-compatibility doesn't need to be a problem at this stage. Moreover we don't know how many users rest-layer has, and maybe such breaking change is a good way to figure out which are the users. To me, adding a good comments in the code/docs is sufficient. IMO This is very needed feature indeed, having Context in the hooks.

[rs]

+1 for breaking change and have the right interface.

[smyrman]

I thing the right interface if we are going to break things anyway, looks more or less like this:

type Validator interface{
   Validate(ctx context.Context, value, original interface{}) (interface{}, error)
}

It would replace both the current schema.FieldValidator and current schema.Validator interfaces. This would also be a small but important step on the way towards #77, which is an old idea allowing for any type of schema.FieldValidator to be used at the top-level.

There are also good cases for FieldValidators where the original (or base) value is of interest for validation, e.g. for correctly performing read-only checks for FieldValidators containing sub-schemas of some sort, such as schema.Object, schema.Dict, schema.AnyOf and schema.AllOf.

For reference, the current schema.Validator interface (used by schema.Schema, and expected by the resource package), looks like this:

// Validator is an interface used to validate schema against actual data.
type Validator interface {
	GetField(name string) *Field
	Prepare(ctx context.Context, payload map[string]interface{}, original *map[string]interface{}, replace bool) (changes map[string]interface{}, base map[string]interface{})
	Validate(changes map[string]interface{}, base map[string]interface{}) (doc map[string]interface{}, errs map[string][]interface{})
}

The GetField method does not need to be here, as it now has it's own interface defined called FieldGetter, and when the method needs to be called on a schema, the schema could be type-casted to that type.

errs map[string][]interface{} could easily be replaced by schema.ErrorMap, and map[string]interface{} is presumably easily replaced by interface{}, though the resource package would need to do an additional type cast.

We need to see if it's practical to merge Prepare and Validate, depending on how much code that depend on them being separate calls.

[smyrman]

Will close issues that relates to a broader redesign of the schema package (such as this one), leaving only #77 open.

[smyrman]

Reopen as a bug; no matter if w are able to solve this easily in the current design, I guess we need to still track it.