Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
smyrman opened this issue Jul 26, 2018 · 4 comments

Comments

@smyrman
Copy link
Collaborator

smyrman commented Jul 26, 2018

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 Move FieldValidator Validate() methods to be be on a pointer value #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.

@smyrman
Copy link
Collaborator Author

smyrman commented Jul 26, 2018

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

@rs
Copy link
Owner

rs commented Jul 26, 2018

Interesting idea.

@smyrman
Copy link
Collaborator Author

smyrman commented Sep 6, 2018

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.

@smyrman
Copy link
Collaborator Author

smyrman commented Sep 4, 2019

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

@smyrman smyrman closed this as completed Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants