rs/rest-layer

Should schema.Schema, Fields and FieldValidators be splitt into a compiled and uncompiled type?

smyrman opened this issue · 4 comments

A discussion in #109 suggest distinguishing between a resource.IndexBuilder (allows adding resources, and returning a compiled index) and resource.Index (allows finding active resources and fetching items).

There are some reasons that could make it worthwhile to take a similar approach for schema.Schema:

  • FieldValidors are often referred to as pointers (e.g. schema.IDField, to mention one). When the Field definition is reused in multiple schemas, or the same schema is compiled multiple times in parallel, e.g. for tests, a data race condition arise. When CIs run the tests with race detection enabled, they are likely to fail.
  • We could avoid doing things like #110 - instead, .Compile would be a method on a FieldValidatorBuilder returning a FieldValidator (naming not final).
  • We won't be able to accidentally use an uncompiled schema in place of a compiled one.

This changes should be considered alongside other potentially large schema package changes, such as #77, that basically suggests allowing any FieldValidator to be used a the Schema top-level.

PS! Not something I am going to work on anytime soon.

rs commented

Interesting idea.

Plausible interface.

To illustrate with today's naming (i.e. without #77):

type Index interface {
    GetResource(name string) query.Resource
}

type Type interface{
    Validator(field Field, idx Index) (FieldValidator, error)
    Serializer(field Field, idx Index) (FieldSerializer, error)
}

Where the new Index interface may be used for other things as well, such as Reference checks (replace RefrenceChecker) and embedding (see #129).

If assuming schema.Field and schema.Schema is "merged" as suggested in #77, the code becomes:

type Type interface{
    Parser(field Schema, idx Index) (Parser, error)
    Serializer(field Schema, idx Index) (Serializer, error)
}

Initially Parser and Serializer would be called in the Compile method of resource.Resource, after which the root schema no longer needs to be used. For neseted types, like Object, AnyOf, Array etc, the methods would work recursively.

Perhaps the interface also need one of these:

FieldGetter(field Schema, idx Index) (FieldGetter, error)

That is used by schema/query, and any changes may want to look into handling #170.

PS! I was contemplating letting the Validator and Serializer be extracted on index.Bind, but that would be to early mainly for two reasons:

  1. Cross reference checks (Resource A refer to B, B refer to A) would need to be delayed to be performed per request (rather than fail early).
  2. Connections needs to be created on a parent-schema -- which would need access to the schema.

Some less well founded remarks:

Later, as part of #109, the resource and index could perhaps be redesigned to not do an explicit compile but instead expose methods for retriveing the Validator and Serializer for a named resource. These could then be passed directly onto e.g. a rest.Handler. Past that point, if you choose to bind more resources to the index (or even remove some), and then make another rest-handler for a different HTTP port (or a GraphQL or custom RPC handler or something else), the already created rest.Handler would be unaffected, as it would hold no references to the orignal Index or Resource. Maybe this last bit isn't feasible, but that's up to the linked ticket to clarify.

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