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.

Sign up for GitHub

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

refactor: Conceptually clarify schema vs query-language #924

Merged
merged 2 commits into from
Nov 1, 2022
Merged

refactor: Conceptually clarify schema vs query-language #924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a325be5
Conceptually split up query-language from schema
AndrewSisley Oct 28, 2022
3cfc861
Remove schema definition from parser
AndrewSisley Oct 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions core/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,6 @@ type SchemaDefinition struct {
Body []byte
}

type Schema struct {
// The individual declarations of types defined by this schema.
Definitions []SchemaDefinition

// The collection descriptions created from/defined by this schema.
Descriptions []client.CollectionDescription
}

// Parser represents the object responsible for handling stuff specific to
// a query language. This includes schema and query parsing, and introspection.
type Parser interface {
Expand All @@ -48,5 +40,5 @@ type Parser interface {
Parse(request string) (*request.Request, []error)

// Adds the given schema to this parser's model.
AddSchema(ctx context.Context, schema string) (*Schema, error)
AddSchema(ctx context.Context, schema string) error
}
58 changes: 51 additions & 7 deletions db/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,34 @@ package db
import (
"context"

"github.com/graphql-go/graphql/language/ast"
dsq "github.com/ipfs/go-datastore/query"

"github.com/sourcenetwork/defradb/client"
"github.com/sourcenetwork/defradb/core"
"github.com/sourcenetwork/defradb/query/graphql/schema"
)

// LoadSchema takes the provided schema in SDL format, and applies it to the database,
// and creates the necessary collections, query types, etc.
func (db *db) AddSchema(ctx context.Context, schemaString string) error {
schema, err := db.parser.AddSchema(ctx, schemaString)
err := db.parser.AddSchema(ctx, schemaString)
if err != nil {
return err
}

for _, desc := range schema.Descriptions {
collectionDescriptions, schemaDefinitions, err := createDescriptions(ctx, schemaString)
if err != nil {
return err
}

for _, desc := range collectionDescriptions {
if _, err := db.CreateCollection(ctx, desc); err != nil {
return err
}
}

return db.saveSchema(ctx, schema)
return db.saveSchema(ctx, schemaDefinitions)
}

func (db *db) loadSchema(ctx context.Context) error {
Expand All @@ -50,17 +58,53 @@ func (db *db) loadSchema(ctx context.Context) error {
sdl += "\n" + string(buf)
}

_, err = db.parser.AddSchema(ctx, sdl)
return err
return db.parser.AddSchema(ctx, sdl)
}

func (db *db) saveSchema(ctx context.Context, schema *core.Schema) error {
func (db *db) saveSchema(ctx context.Context, schemaDefinitions []core.SchemaDefinition) error {
// save each type individually
for _, def := range schema.Definitions {
for _, def := range schemaDefinitions {
key := core.NewSchemaKey(def.Name)
if err := db.systemstore().Put(ctx, key.ToDS(), def.Body); err != nil {
return err
}
}
return nil
}

func createDescriptions(
ctx context.Context,
schemaString string,
) ([]client.CollectionDescription, []core.SchemaDefinition, error) {
// We should not be using this package at all here, and should not be doing most of what this package does
// Rework: https://github.com/sourcenetwork/defradb/issues/923
schemaManager, err := schema.NewSchemaManager()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new schema manager is required, as each instance caches the types and reusing the gql-query-lang one would cause collisions

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: I look forward do see what you're going to do here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be quite simple I think, and we'll be able to go straight from ast to collectionDescription without worrying about gql-types or some virtual fields (IIRC it has to skip over stuff like _group and aggregates)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't think we want to do a new schema manager for each createDescriptions exec. You should be able to refer to previous types when registering new types. That wouldn't be possible if we have a new manager on each exec.

  2. I don't necessarily see the benefit of going from AST to Description without a gql type intermediary, but thats a problem that can be kicked down the road.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to refer to previous types when registering new types. That wouldn't be possible if we have a new manager on each exec

Hmm, I forgot about that, and we have no tests for that either (and so it is quite possibly already broken, and I remember it having been broken before at one point - we need to cover this). For a test gap like this I'd be extremely tempted to say it is broken already, and we need to sort it out in another ticket anyway due to the scope-creep/likelyhood-of-finding-many-more-bugs.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Oct 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started working on this, and what you are saying is currently syntactically impossible with our current system. A schema cannot reference an existing type, as that type also needs to be amended to reference the new type (in the schema definition):

var bookAuthorGQLSchema = (`
	type book {
		name: String
		author: author
	}

	type author {
		name: String
		published: [book]
	}
`)

See how both definitions contain a reference to each other - you cannot add author, and then add book (without update, which we dont support atm).

Copy link
Member

@jsimnz jsimnz Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realized this after the fact, indeed very true.

I still would've preferred a single instance of the schema, I don't really see the value in creating one for each request. There's no way to reference the collection GQL types in a consistent way.

What benefit does this approach have in your mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What benefit does this approach have in your mind?

Absence of state, simplicity, ease of ripping out, minimized risk of state-polution, and the fact that persisting it provides no assertable benefits atm

if err != nil {
return nil, nil, err
}

types, astdoc, err := schemaManager.Generator.FromSDL(ctx, schemaString)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion(non-blocking): Do you want to expose the BuildTypesFromAST func instead of executing the entire FromSDL generation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I do not want to make the existing generator code more complex by adding a new entry point, for the sake of temporary code. Performance doesn't matter much here, so the code-cleanliness and 'keep it as before and dont worry' is much preferred.

if err != nil {
return nil, nil, err
}

colDesc, err := schemaManager.Generator.CreateDescriptions(types)
if err != nil {
return nil, nil, err
}

definitions := make([]core.SchemaDefinition, len(astdoc.Definitions))
for i, astDefinition := range astdoc.Definitions {
objDef, isObjDef := astDefinition.(*ast.ObjectDefinition)
if !isObjDef {
continue
}

definitions[i] = core.SchemaDefinition{
Name: objDef.Name.Value,
Body: objDef.Loc.Source.Body[objDef.Loc.Start:objDef.Loc.End],
}
}

return colDesc, definitions, nil
}
32 changes: 3 additions & 29 deletions query/graphql/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"github.com/sourcenetwork/defradb/query/graphql/schema"

gql "github.com/graphql-go/graphql"
"github.com/graphql-go/graphql/language/ast"
gqlp "github.com/graphql-go/graphql/language/parser"
"github.com/graphql-go/graphql/language/source"
)
Expand Down Expand Up @@ -96,32 +95,7 @@ func (p *parser) Parse(request string) (*request.Request, []error) {
return query, nil
}

func (p *parser) AddSchema(ctx context.Context, schema string) (*core.Schema, error) {
types, astdoc, err := p.schemaManager.Generator.FromSDL(ctx, schema)
if err != nil {
return nil, err
}

colDesc, err := p.schemaManager.Generator.CreateDescriptions(types)
if err != nil {
return nil, err
}

definitions := make([]core.SchemaDefinition, len(astdoc.Definitions))
for i, astDefinition := range astdoc.Definitions {
objDef, isObjDef := astDefinition.(*ast.ObjectDefinition)
if !isObjDef {
continue
}

definitions[i] = core.SchemaDefinition{
Name: objDef.Name.Value,
Body: objDef.Loc.Source.Body[objDef.Loc.Start:objDef.Loc.End],
}
}

return &core.Schema{
Definitions: definitions,
Descriptions: colDesc,
}, nil
func (p *parser) AddSchema(ctx context.Context, schema string) error {
_, _, err := p.schemaManager.Generator.FromSDL(ctx, schema)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Is this simply cashing the schema for the GQL package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generating (the query language types - used for validation and introspection) and caching them - this is done everytime the database is initialized (e.g. on restart), as well as when a new schema is provided

return err
}
2 changes: 1 addition & 1 deletion tests/bench/query/planner/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func buildParser(
if err != nil {
return nil, err
}
_, err = parser.AddSchema(ctx, schema)
err = parser.AddSchema(ctx, schema)
if err != nil {
return nil, err
}
Expand Down