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

feat(GraphQL): Disallow DQL schema changes for predicates used in GraphQL schema (DGRAPH-3245) #7742

Merged
merged 3 commits into from
Apr 22, 2021
Merged

feat(GraphQL): Disallow DQL schema changes for predicates used in GraphQL schema (DGRAPH-3245) #7742

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5889ed6
restrict alter for GraphQL predicates
abhimanyusinghgaur Apr 21, 2021
ea8565e
fix lint
abhimanyusinghgaur Apr 21, 2021
69d49e5
better error messages
abhimanyusinghgaur Apr 22, 2021
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
125 changes: 125 additions & 0 deletions edgraph/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,8 @@ func (s *Server) Alter(ctx context.Context, op *api.Operation) (*api.Payload, er
_, err := query.ApplyMutations(ctx, m)
return empty, err
}

// it is a schema update
result, err := parseSchemaFromAlterOperation(ctx, op)
if err == errIndexingInProgress {
// Make the client wait a bit.
Expand All @@ -528,6 +530,9 @@ func (s *Server) Alter(ctx context.Context, op *api.Operation) (*api.Payload, er
} else if err != nil {
return nil, err
}
if err = validateDQLSchemaForGraphQL(ctx, result, namespace); err != nil {
return nil, err
}

glog.Infof("Got schema: %+v\n", result)
// TODO: Maybe add some checks about the schema.
Expand All @@ -546,6 +551,126 @@ func (s *Server) Alter(ctx context.Context, op *api.Operation) (*api.Payload, er
return empty, nil
}

func validateDQLSchemaForGraphQL(ctx context.Context,
dqlSch *schema.ParsedSchema, ns uint64) error {
// fetch the GraphQL schema for this namespace from disk
_, existingGQLSch, err := GetGQLSchema(ns)
if err != nil || existingGQLSch == "" {
return err
}

// convert the existing GraphQL schema to a DQL schema
handler, err := gqlSchema.NewHandler(existingGQLSch, false)
if err != nil {
return err
}
dgSchema := handler.DGSchema()
if dgSchema == "" {
return nil
}
gqlReservedDgSch, err := parseSchemaFromAlterOperation(ctx, &api.Operation{Schema: dgSchema})
if err != nil {
return err
}

// create a mapping for the GraphQL reserved predicates and types
gqlReservedPreds := make(map[string]*pb.SchemaUpdate)
gqlReservedTypes := make(map[string]*pb.TypeUpdate)
for _, pred := range gqlReservedDgSch.Preds {
gqlReservedPreds[pred.Predicate] = pred
}
for _, typ := range gqlReservedDgSch.Types {
gqlReservedTypes[typ.TypeName] = typ
}

// now validate the DQL schema to check that it doesn't break the existing GraphQL schema

// Step-1: validate predicates
for _, dqlPred := range dqlSch.Preds {
gqlPred := gqlReservedPreds[dqlPred.Predicate]
if gqlPred == nil {
continue // if the predicate isn't used by GraphQL, no need to validate it.
}

// type (including list) must match exactly
if gqlPred.ValueType != dqlPred.ValueType || gqlPred.List != dqlPred.List {
gqlType := strings.ToLower(gqlPred.ValueType.String())
dgType := strings.ToLower(dqlPred.ValueType.String())
if gqlPred.List {
gqlType = "[" + gqlType + "]"
}
if dqlPred.List {
dgType = "[" + dgType + "]"
}
return errors.Errorf("type mismatch for GraphQL predicate %s. want: %s, got: %s",
x.ParseAttr(gqlPred.Predicate), gqlType, dgType)
}
// if gqlSchema had any indexes, then those must be present in the dqlSchema.
// dqlSchema may add more indexes than what gqlSchema had initially, but can't remove them.
if gqlPred.Directive == pb.SchemaUpdate_INDEX {
if dqlPred.Directive != pb.SchemaUpdate_INDEX {
return errors.Errorf("missing @index for GraphQL predicate %s",
x.ParseAttr(gqlPred.Predicate))
}
for _, t := range gqlPred.Tokenizer {
if !x.HasString(dqlPred.Tokenizer, t) {
return errors.Errorf("missing %s index for GraphQL predicate %s", t,
x.ParseAttr(gqlPred.Predicate))
}
}
}
// if gqlSchema had @reverse, then dqlSchema must have it. dqlSchema can't remove @reverse.
// if gqlSchema didn't had @reverse, it is allowed to dqlSchema to add it.
if gqlPred.Directive == pb.SchemaUpdate_REVERSE && dqlPred.Directive != pb.
SchemaUpdate_REVERSE {
return errors.Errorf("missing @reverse for GraphQL predicate %s",
x.ParseAttr(gqlPred.Predicate))
}
// if gqlSchema had @count, then dqlSchema must have it. dqlSchema can't remove @count.
// if gqlSchema didn't had @count, it is allowed to dqlSchema to add it.
if gqlPred.Count && !dqlPred.Count {
return errors.Errorf("missing @count for GraphQL predicate %s",
x.ParseAttr(gqlPred.Predicate))
}
// if gqlSchema had @upsert, then dqlSchema must have it. dqlSchema can't remove @upsert.
// if gqlSchema didn't had @upsert, it is allowed to dqlSchema to add it.
if gqlPred.Upsert && !dqlPred.Upsert {
return errors.Errorf("missing @upsert for GraphQL predicate %s",
x.ParseAttr(gqlPred.Predicate))
}
// if gqlSchema had @lang, then dqlSchema must have it. dqlSchema can't remove @lang.
// if gqlSchema didn't had @lang, it is allowed to dqlSchema to add it.
if gqlPred.Lang && !dqlPred.Lang {
return errors.Errorf("missing @lang for GraphQL predicate %s",
x.ParseAttr(gqlPred.Predicate))
}
}

// Step-2: validate types
for _, dqlType := range dqlSch.Types {
gqlType := gqlReservedTypes[dqlType.TypeName]
if gqlType == nil {
continue // if the type isn't used by GraphQL, no need to validate it.
}

// create a mapping of all the fields in the dqlType
dqlFields := make(map[string]bool)
for _, f := range dqlType.Fields {
dqlFields[f.Predicate] = true
}

// check that all the fields of the gqlType must be present in the dqlType
for _, f := range gqlType.Fields {
if !dqlFields[f.Predicate] {
return errors.Errorf("missing field %s in GraphQL type %s",
x.ParseAttr(f.Predicate), x.ParseAttr(gqlType.TypeName))
}
}
}

return nil
}

func annotateStartTs(span *otrace.Span, ts uint64) {
span.Annotate([]otrace.Attribute{otrace.Int64Attribute("startTs", int64(ts))}, "")
}
Expand Down
225 changes: 225 additions & 0 deletions graphql/e2e/schema/schema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,231 @@ func TestDeleteSchemaAndExport(t *testing.T) {
require.NotEqual(t, schemaResp.Id, newSchemaResp.Id)
}

// TestAlterWithGraphQLSchema ensures that the predicates used by GraphQL schema can't be
// modified using Alter directly.
func TestAlterWithGraphQLSchema(t *testing.T) {
common.SafelyDropAll(t)

// initially alter should succeed
dg, err := testutil.DgraphClient(groupOnegRPC)
require.NoError(t, err)
require.NoError(t, dg.Alter(context.Background(), &api.Operation{
Schema: `
type Person {
Person.name
}
Person.name: string @index(exact) .
dqlPred: int .
`,
}))

// now apply a GraphQL schema
common.SafelyUpdateGQLSchema(t, groupOneHTTP, `
type Person {
id: ID!
name: String! @id
age: Int
bio: String @search(by: [term,fulltext])
bioHi: String @dgraph(pred: "Person.bio@hi")
follows: [Person] @dgraph(pred: "Person.follows")
followedBy: [Person] @dgraph(pred: "~Person.follows")
relative: Person
}`, nil)

/******************
FAILURE CASES
******************/

// 1. int -> float should fail
require.Contains(t, dg.Alter(context.Background(), &api.Operation{
Schema: `
type Person {
Person.name
Person.age
Person.bio
Person.follows
Person.relative
}
Person.name: string @index(hash) @upsert .
Person.age: float .
Person.bio: string @index(term, fulltext) @lang .
Person.follows: [uid] @reverse .
Person.relative: uid .
`,
}).Error(), "type mismatch for GraphQL predicate Person.age. want: int, got: float")

// 2. int -> [int] should fail
require.Contains(t, dg.Alter(context.Background(), &api.Operation{
Schema: `
type Person {
Person.name
Person.age
Person.bio
Person.follows
Person.relative
}
Person.name: string @index(hash) @upsert .
Person.age: [int] .
Person.bio: string @index(term, fulltext) @lang .
Person.follows: [uid] @reverse .
Person.relative: uid .
`,
}).Error(), "type mismatch for GraphQL predicate Person.age. want: int, got: [int]")

// 3. [uid] -> uid should fail
require.Contains(t, dg.Alter(context.Background(), &api.Operation{
Schema: `
type Person {
Person.name
Person.age
Person.bio
Person.follows
Person.relative
}
Person.name: string @index(hash) @upsert .
Person.age: int .
Person.bio: string @index(term, fulltext) @lang .
Person.follows: uid @reverse .
Person.relative: uid .
`,
}).Error(), "type mismatch for GraphQL predicate Person.follows. want: [uid], got: uid")

// 4. removing @index should fail
require.Contains(t, dg.Alter(context.Background(), &api.Operation{
Schema: `
type Person {
Person.name
Person.age
Person.bio
Person.follows
Person.relative
}
Person.name: string @upsert .
Person.age: int .
Person.bio: string @index(term, fulltext) @lang .
Person.follows: [uid] @reverse .
Person.relative: uid .
`,
}).Error(), "missing @index for GraphQL predicate Person.name")

// 5. @index(hash) -> @index(term) should fail
require.Contains(t, dg.Alter(context.Background(), &api.Operation{
Schema: `
type Person {
Person.name
Person.age
Person.bio
Person.follows
Person.relative
}
Person.name: string @index(term) @upsert .
Person.age: int .
Person.bio: string @index(term, fulltext) @lang .
Person.follows: [uid] @reverse .
Person.relative: uid .
`,
}).Error(), "missing hash index for GraphQL predicate Person.name")

// 6. removing @reverse should fail
require.Contains(t, dg.Alter(context.Background(), &api.Operation{
Schema: `
type Person {
Person.name
Person.age
Person.bio
Person.follows
Person.relative
}
Person.name: string @index(hash) @upsert .
Person.age: int .
Person.bio: string @index(term, fulltext) @lang .
Person.follows: [uid] .
Person.relative: uid .
`,
}).Error(), "missing @reverse for GraphQL predicate Person.follows")

// 7. removing @upsert should fail
require.Contains(t, dg.Alter(context.Background(), &api.Operation{
Schema: `
type Person {
Person.name
Person.age
Person.bio
Person.follows
Person.relative
}
Person.name: string @index(hash) .
Person.age: int .
Person.bio: string @index(term, fulltext) @lang .
Person.follows: [uid] @reverse .
Person.relative: uid .
`,
}).Error(), "missing @upsert for GraphQL predicate Person.name")

// 8. removing @lang should fail
require.Contains(t, dg.Alter(context.Background(), &api.Operation{
Schema: `
type Person {
Person.name
Person.age
Person.bio
Person.follows
Person.relative
}
Person.name: string @index(hash) @upsert .
Person.age: int .
Person.bio: string @index(term, fulltext) .
Person.follows: [uid] @reverse .
Person.relative: uid .
`,
}).Error(), "missing @lang for GraphQL predicate Person.bio")

// 9. removing a GraphQL field from Person type should fail
require.Contains(t, dg.Alter(context.Background(), &api.Operation{
Schema: `
type Person {
Person.age
Person.bio
Person.follows
Person.relative
}
Person.name: string @index(hash) @upsert .
Person.age: int .
Person.bio: string @index(term, fulltext) @lang .
Person.follows: [uid] @reverse .
Person.relative: uid .
`,
}).Error(), "missing field Person.name in GraphQL type Person")

/******************
SUCCESS CASES
******************/
// 1. adding/updating a non-GraphQL predicate, as well as adding/removing them from a type
// 2. @index(term, fulltext) -> @index(exact, term, fulltext) for a GraphQL predicate
// 3. adding @count to a GraphQL predicate
// 4. adding @reverse to a GraphQL predicate
// 5. adding @upsert to a GraphQL predicate
// 6. adding @lang to a GraphQL predicate
require.NoError(t, dg.Alter(context.Background(), &api.Operation{
Schema: `
type Person {
Person.name
Person.age
Person.bio
Person.follows
Person.relative
dqlPred
}
Person.name: string @index(hash) @upsert @lang .
Person.age: int @index(int) @upsert .
Person.bio: string @index(exact, term, fulltext) @lang .
Person.follows: [uid] @reverse @count .
Person.relative: uid @reverse .
dqlPred: float @index(float) .
`,
}))
}

func updateGQLSchemaConcurrent(t *testing.T, schema, authority string) bool {
res := common.RetryUpdateGQLSchema(t, authority, schema, nil)
err := res.Errors.Error()
Expand Down