Skip to content
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

Redesign schema package to allow any FieldValidator at the top level #77

Open
smyrman opened this issue Jan 2, 2017 · 22 comments
Open

Redesign schema package to allow any FieldValidator at the top level #77

smyrman opened this issue Jan 2, 2017 · 22 comments

Comments

@smyrman
Copy link
Collaborator

@smyrman smyrman commented Jan 2, 2017

UPDATED on 2018-09-05. Originally this was a question between the difference of the schema.Object FieldValidator and the Schema paramter on Field, and it evolved from there.

Background

Today there are two ways to specify a schema:

s := schema.Schema{
        Fields: schema.Fields{
                "meta": {Schema: subSchema}
        }
}

and

s := schema.Schema{
        Fields: schema.Fields{
                "meta": {Validator: schema.Object{Schema: subSchema}}
        }
}

These are equivalent in principal, but in reality, the code supports them differently.

E.g. for validating and correctly setting e.g. read-only values, only the first method will work.

The second way of expressing this, the only one supported by the JSON-schmea encoding package, is the only syntax that allowed Schema validation within an Array, AnyOff, AllOf or other nested structure in the past.

Proposal

This ticket suggests a redesign of the schema package so that:

  • Allow any FieldValidator to be used in a top-level schema (renamed to just schema.Validator in example).
  • Merge the schema.Schema and schema.Field types into one
  • Fields is moved from schema to Object (and is now a map of Schemas)
  • Move the Required Field attribute (does not make sense on all fields) to be a list-attribute on Object.

Example struct definitions:

type Object struct{
    Fields   map[string]Schema
    Required []string
}

type Object struct{
    Fields   map[string]Schema
    Required []string
}

type Schema struct{
    Title       string
    Description string
    ReadOnly    bool
    Type        Validator
}

type Array struct{
    KeysValidator Validator
    Values        Schema
}
@rs
Copy link
Owner

@rs rs commented Jan 2, 2017

Good question. It was add by @yanfali In #24, he can certainly answer.

@yanfali
Copy link
Contributor

@yanfali yanfali commented Jan 2, 2017

Primarily use case. I wanted to use a schema in array types. Previously you could only use a type which didn't support schema.

@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Jan 2, 2017

Ah, ok so Field.Schema is the initial support for sub-schemas here. I actually like the Object FieldValidator better than having Field.Schema, as it's both more consistent, and avoids ambiguous meaning like:

 "meta": Field{Validator: schema.String{}, Schema: subSchema}

Would it be sensible to deprecate/remove the Schema attribute from Field - perhaps after tagging a release, as suggested in #65? I assume this could also simplify things internally and avoid handling the same case twice several places? For one I realize we don't handle Field.Schema in the jsonschema package.

@rs
Copy link
Owner

@rs rs commented Jan 3, 2017

Yes I think should remove Schema at Field level. It is indeed a breaking change but the project is still not in a stable state so I think it's ok.

For #65 I would rather wait for the Go official way to handle that.

@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Jan 3, 2017

For #65 I would rather wait for the Go official way to handle that.

OK, for the manifest part, but tagging a release in Git (e.g. v0.1.0), is something the team have already recommended us to do, and for end-users, rest-layer would be a lot safer/easier to use if you could rely on a tagged release. Especially when we do breaking changes.

@rs
Copy link
Owner

@rs rs commented Jan 3, 2017

Sure we can tag.

@smyrman smyrman changed the title Question: Field.Schema v.s. Field.Validator = schema.Object Replace Field.Schema with Field.Validator = schema.Object Jan 21, 2017
@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Jan 21, 2017

@rs - updated the title & description.

@rs
Copy link
Owner

@rs rs commented Jan 21, 2017

Are you going to submit a PR for this one?

@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Jan 22, 2017

I don't know if I can/want to figure out the implications on reference checking... there are some other easier issues I would rather look at, e.g. JSON schema ones...

@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Jan 22, 2017

At least not know

@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Feb 13, 2017

@rs Looking at the definition of Object:

type Object struct {
    Schema *Schema
}

Could we not make Schema implement FieldValidator or at least change the definition to:

type Object Schema
@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Feb 13, 2017

Maybe it's not so easy btw. At least not without introducing additional breaking changes everywhere...

It would be nice to make the API consistent by removing the Schema attribute, while still avoiding the additional level of nesting/indenting you get by using schema.Object.

@rs
Copy link
Owner

@rs rs commented Feb 13, 2017

I'm all for introducing breaking changes now that simplify the API rather than later.

@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Sep 19, 2017

I have done some background thinking on this, and I have a rather radical proposal:

  • Move schema.Fields from Schema to Object, and let the Schema have a Validator field instead. We might want to somehow restrict the validator to support only Object types! However, we should at least be able to hold any of schema.Dict, schema.Object, schema.AllOf and schema.AnyOf (where for the last two types, all entries are within the supported list, recursively checked). This may be hard-coded or based on interfaces.
  • Remove Schema from Field.

Part of the reasoning for this, is that I have a use-case to support something like this at a top level:

s := schema.Schema{
    Description: "My resource",
    Validator: &schema.AllOf{
        &schema.Object{
            Fields: commonFields,
        },
        $schema.AnyOf{
            $schema.Object{Fields: schema.Fields{
                "type":{Validator:&schema.String{Allowed:[]string{"x"}}}},
                "someField":{Validator: validatorX},
            },
            $schema.Object{Fields: schema.Fields{
                "type":{Validator:&schema.String{Allowed:[]string{"y"}}}},
                "someField":{Validator: validatorY},
            }
        }
    }
}

I can do this today via a hook with extra validator rools, but that would fail to generate the right documentation.

@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Sep 19, 2017

The error type returned from schema.Object (and any other nested type), will be the existing ErrorMap type.

@rs
Copy link
Owner

@rs rs commented Sep 20, 2017

Interesting idea. Then instead of Validator shouldn't we rename this field Type?

The root as Object might be enforced in resource. This restriction does not necessarily apply to all use-cases (outside of REST).

@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Oct 5, 2017

IN #151, I am proposing to replace ValuesValidator FieldValidator in schema.Dict with Values Fields and later do the same in schema.Array. As already proposed in this issue, schema.Object will also contain a set of fields.

However, for all except schema.Object, keeping the Required property inside the Field definition becomes a bit awkward, or at least we would have to ignore it. JSON Schema have solved this ackwardness by making required a list of strings in the schema. Maybe we should consider to do the same for rest-layer. That would allow us to e.g. specify:

Validator: &schema.Dict{
    Required: []string{"fieldA", "fieldB"},
    KeyValidator: &schema.String{},
    Values: schema.Field{...}
}

And also translate that to JSON Schema:

{
    "type": "object",
    "additionalProperties": {...},
    "required": ["fieldA", "fieldB"]
}

But more importantly, we would not have a unused Field.Required property that would cause confusion when a Field is used within a schema.Array or schema.Dict.

@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Jul 26, 2018

I have done some background thinking on this, and I have a rather radical proposal (...)

I want to drag this a little bit further. I think the schema.Field and schema.Schema types could be merged into one:

type Object struct{
    Fields   map[string]Schema
    Required []string
}

type Object struct{
    Fields   map[string]Schema
    Required []string
}

type Schema struct{
    Title       string
    Description string
    ReadOnly    bool
    Type        Validator
}

Or, a variant taking #194 into consideration:

type SchemaBuilder struct{
    Title       string
    Description string
    ReadOnly    bool
    Type        ValidatorBuilder
}

// A compiled schema of some sort...
type Schema interface{
    Title() string
    Description() string
    ReadOnly() bool
    Validate(ctx context.Context, changes, original interface{}) error
    JSONSchema() (map[string]interface{}, error)
}

For now it's just an idea to remember when starting on the actual implementation (some time in the future).

@smyrman smyrman changed the title Replace Field.Schema with Field.Validator = schema.Object Redesign schema package to allow any FieldValidator at the top level. Sep 5, 2018
@smyrman smyrman changed the title Redesign schema package to allow any FieldValidator at the top level. Redesign schema package to allow any FieldValidator at the top level Sep 5, 2018
@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Dec 12, 2018

I plan to start this issue by:

  • Creating a new (blank) repo for the schema package for experimental purposes.
  • Copy over just enough types to get the design right.

I will probably take my time, and try to address as many design issues as possible, and then we can see what we want to do when it's done.

@rs
Copy link
Owner

@rs rs commented Dec 12, 2018

Why not doing this in a branch?

@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented Dec 12, 2018

We use rest-layer actively. We are pretty satisfied with it, but over time we have hit most of the limitations regarding the current schema package at some point, and either worked around it, or left it in a slightly undesirable state.

To get up with a design that addresses all these issues, I basically first want to proto-type it, without having to take the other rest-layer packages (or even most of the schema package) into account.

In this proto-type I want to attempt to solve this issue as well as some linked issues (e.g. #194 and #192, and maybe #138, and perhaps other issues that I can think of. This may need to happen in several iterations. The proto-type will not copy the query package.

Once the proto-type is done, I see two possible futures for the schema package:

  1. We complete it and merge it back into rest-layer
  2. We check if the package could possibly reach a larger user-base (and more potential contributors), if it's maintained as a separate repo.

For case 2, the query package would necessarily remain in rest-layer, but except for that, there is not much dependencies on the schema validation/serialization package itself that can not be solved by a simple interface (making the schema-package pluggable). PS! This isn't really a goal though, so most likely it would still be 1.

For the proto-type, weather we choose 1 or 2 is mostly irrelevant.

@smyrman
Copy link
Collaborator Author

@smyrman smyrman commented May 20, 2019

Link to proto-type for new schema design:

It's not in a working state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.