From b2d8ab6ac6e9ce11eedb0d3b5b60fe53f869dbec Mon Sep 17 00:00:00 2001 From: Aman Mangal Date: Mon, 25 Nov 2019 22:41:40 +0530 Subject: [PATCH 1/5] Return error instead of panic when geo data is corrupted --- query/query.go | 5 +- query/query3_test.go | 109 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/query/query.go b/query/query.go index c2575b33263..1e9376cc3cc 100644 --- a/query/query.go +++ b/query/query.go @@ -394,7 +394,10 @@ func convertWithBestEffort(tv *pb.TaskValue, attr string) (types.Val, error) { // creates appropriate type from binary format sv, err := types.Convert(v, v.Tid) - x.Checkf(err, "Error while interpreting appropriate type from binary") + if err != nil { + // This can happen when a mutation ingests corrupt data into the database. + return v, errors.Wrap(err, "Error while interpreting appropriate type from binary") + } return sv, nil } diff --git a/query/query3_test.go b/query/query3_test.go index 2c44a06842b..830a3b7d5fd 100644 --- a/query/query3_test.go +++ b/query/query3_test.go @@ -18,12 +18,20 @@ package query import ( "context" + "encoding/binary" "encoding/json" "fmt" "strings" "testing" + "github.com/dgraph-io/dgo/v2" + "github.com/dgraph-io/dgo/v2/protos/api" + "github.com/dgraph-io/dgraph/testutil" "github.com/stretchr/testify/require" + geom "github.com/twpayne/go-geom" + "github.com/twpayne/go-geom/encoding/geojson" + "github.com/twpayne/go-geom/encoding/wkb" + "google.golang.org/grpc" "google.golang.org/grpc/metadata" ) @@ -2192,3 +2200,104 @@ func TestMultiRegexInFilter2(t *testing.T) { require.JSONEq(t, `{"data": {"q": [{"firstName": "Han", "lastName":"Solo"}]}}`, res) } } + +func TestGeoDataInvalidString(t *testing.T) { + conn, err := grpc.Dial(testutil.SockAddr, grpc.WithInsecure()) + require.NoError(t, err) + dg := dgo.NewDgraphClient(api.NewDgraphClient(conn)) + ctx := context.Background() + require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) + require.NoError(t, dg.Alter(ctx, &api.Operation{Schema: `loc: geo .`})) + + n := &api.NQuad{ + Subject: "_:test", + Predicate: "loc", + ObjectValue: &api.Value{ + Val: &api.Value_StrVal{ + StrVal: `{"type": "Point", "coordintaes": [1.0, 2.0]}`, + }, + }, + } + _, err = dg.NewTxn().Mutate(ctx, &api.Mutation{ + CommitNow: true, + Set: []*api.NQuad{n}, + }) + require.Contains(t, err.Error(), "geom: unsupported layout NoLayout") +} + +func TestGeoCorruptData(t *testing.T) { + conn, err := grpc.Dial(testutil.SockAddr, grpc.WithInsecure()) + require.NoError(t, err) + dg := dgo.NewDgraphClient(api.NewDgraphClient(conn)) + ctx := context.Background() + require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) + require.NoError(t, dg.Alter(ctx, &api.Operation{Schema: `loc: geo .`})) + + n := &api.NQuad{ + Subject: "_:test", + Predicate: "loc", + ObjectValue: &api.Value{ + Val: &api.Value_GeoVal{ + GeoVal: []byte(`{"type": "Point", "coordinates": [1.0, 2.0]}`), + }, + }, + } + _, err = dg.NewTxn().Mutate(ctx, &api.Mutation{ + CommitNow: true, + Set: []*api.NQuad{n}, + }) + require.NoError(t, err) + + query := ` +{ + all(func: has(loc)) { + uid + loc + } +}` + _, err = dg.NewReadOnlyTxn().Query(ctx, query) + require.Contains(t, err.Error(), "wkb: unknown byte order: 1111011") +} + +func TestGeoValidWkbData(t *testing.T) { + conn, err := grpc.Dial(testutil.SockAddr, grpc.WithInsecure()) + require.NoError(t, err) + dg := dgo.NewDgraphClient(api.NewDgraphClient(conn)) + ctx := context.Background() + require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) + require.NoError(t, dg.Alter(ctx, &api.Operation{Schema: `loc: geo .`})) + s := `{"type": "Point", "coordinates": [1.0, 2.0]}` + var gt geom.T + if err := geojson.Unmarshal([]byte(s), >); err != nil { + panic(err) + } + data, err := wkb.Marshal(gt, binary.LittleEndian) + if err != nil { + panic(err) + } + n := &api.NQuad{ + Subject: "_:test", + Predicate: "loc", + ObjectValue: &api.Value{ + Val: &api.Value_GeoVal{ + GeoVal: data, + }, + }, + } + + _, err = dg.NewTxn().Mutate(ctx, &api.Mutation{ + CommitNow: true, + Set: []*api.NQuad{n}, + }) + require.NoError(t, err) + query := ` +{ + all(func: has(loc)) { + uid + loc + } +}` + resp, err := dg.NewReadOnlyTxn().Query(ctx, query) + require.NoError(t, err) + require.Contains(t, string(resp.Json), `{"type":"Point","coordinates":[1,2]}`) +} From 9c859b6288f8b7209f12f5ebb9c3022d0239a79f Mon Sep 17 00:00:00 2001 From: Aman Mangal Date: Wed, 27 Nov 2019 16:35:26 +0530 Subject: [PATCH 2/5] address comments --- query/query.go | 2 +- query/query3_test.go | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/query/query.go b/query/query.go index 1e9376cc3cc..4e458c9a063 100644 --- a/query/query.go +++ b/query/query.go @@ -396,7 +396,7 @@ func convertWithBestEffort(tv *pb.TaskValue, attr string) (types.Val, error) { sv, err := types.Convert(v, v.Tid) if err != nil { // This can happen when a mutation ingests corrupt data into the database. - return v, errors.Wrap(err, "Error while interpreting appropriate type from binary") + return v, errors.Wrapf(err, "error interpreting appropriate type for %v", attr) } return sv, nil } diff --git a/query/query3_test.go b/query/query3_test.go index 830a3b7d5fd..7d42c764aef 100644 --- a/query/query3_test.go +++ b/query/query3_test.go @@ -2225,6 +2225,9 @@ func TestGeoDataInvalidString(t *testing.T) { require.Contains(t, err.Error(), "geom: unsupported layout NoLayout") } +// This test shows that GeoVal API doesn't accept string data. Though, mutation +// succeeds querying the data returns an error. Ideally, we should not accept +// invalid data in a mutation though that is left as future work. func TestGeoCorruptData(t *testing.T) { conn, err := grpc.Dial(testutil.SockAddr, grpc.WithInsecure()) require.NoError(t, err) @@ -2259,6 +2262,9 @@ func TestGeoCorruptData(t *testing.T) { require.Contains(t, err.Error(), "wkb: unknown byte order: 1111011") } +// This test shows how we could use the GeoVal API to store geo data. +// As far as I (Aman) know, this is something that should not be used +// by a common user unless user knows what she is doing. func TestGeoValidWkbData(t *testing.T) { conn, err := grpc.Dial(testutil.SockAddr, grpc.WithInsecure()) require.NoError(t, err) From d6c4a8cb4bad5df704c8142e116d07089f767f91 Mon Sep 17 00:00:00 2001 From: Aman Mangal Date: Wed, 27 Nov 2019 23:12:26 +0530 Subject: [PATCH 3/5] Login before running tests --- query/query3_test.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/query/query3_test.go b/query/query3_test.go index 7d42c764aef..04837d1f6bd 100644 --- a/query/query3_test.go +++ b/query/query3_test.go @@ -24,14 +24,12 @@ import ( "strings" "testing" - "github.com/dgraph-io/dgo/v2" "github.com/dgraph-io/dgo/v2/protos/api" "github.com/dgraph-io/dgraph/testutil" "github.com/stretchr/testify/require" geom "github.com/twpayne/go-geom" "github.com/twpayne/go-geom/encoding/geojson" "github.com/twpayne/go-geom/encoding/wkb" - "google.golang.org/grpc" "google.golang.org/grpc/metadata" ) @@ -2202,9 +2200,9 @@ func TestMultiRegexInFilter2(t *testing.T) { } func TestGeoDataInvalidString(t *testing.T) { - conn, err := grpc.Dial(testutil.SockAddr, grpc.WithInsecure()) + dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) require.NoError(t, err) - dg := dgo.NewDgraphClient(api.NewDgraphClient(conn)) + ctx := context.Background() require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) require.NoError(t, dg.Alter(ctx, &api.Operation{Schema: `loc: geo .`})) @@ -2229,9 +2227,9 @@ func TestGeoDataInvalidString(t *testing.T) { // succeeds querying the data returns an error. Ideally, we should not accept // invalid data in a mutation though that is left as future work. func TestGeoCorruptData(t *testing.T) { - conn, err := grpc.Dial(testutil.SockAddr, grpc.WithInsecure()) + dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) require.NoError(t, err) - dg := dgo.NewDgraphClient(api.NewDgraphClient(conn)) + ctx := context.Background() require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) require.NoError(t, dg.Alter(ctx, &api.Operation{Schema: `loc: geo .`})) @@ -2266,9 +2264,9 @@ func TestGeoCorruptData(t *testing.T) { // As far as I (Aman) know, this is something that should not be used // by a common user unless user knows what she is doing. func TestGeoValidWkbData(t *testing.T) { - conn, err := grpc.Dial(testutil.SockAddr, grpc.WithInsecure()) + dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) require.NoError(t, err) - dg := dgo.NewDgraphClient(api.NewDgraphClient(conn)) + ctx := context.Background() require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) require.NoError(t, dg.Alter(ctx, &api.Operation{Schema: `loc: geo .`})) From cc0c887059753706b144d461b36479d26973a9d1 Mon Sep 17 00:00:00 2001 From: Aman Mangal Date: Thu, 28 Nov 2019 12:27:52 +0530 Subject: [PATCH 4/5] Move test --- dgraph/cmd/alpha/run_test.go | 111 ++++++++++++++++++++++++++++++++++ query/query3_test.go | 113 ----------------------------------- 2 files changed, 111 insertions(+), 113 deletions(-) diff --git a/dgraph/cmd/alpha/run_test.go b/dgraph/cmd/alpha/run_test.go index 77b13e8e753..117b583d5e2 100644 --- a/dgraph/cmd/alpha/run_test.go +++ b/dgraph/cmd/alpha/run_test.go @@ -19,6 +19,7 @@ package alpha import ( "bytes" "context" + "encoding/binary" "encoding/json" "fmt" "log" @@ -40,6 +41,9 @@ import ( "github.com/pkg/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/twpayne/go-geom" + "github.com/twpayne/go-geom/encoding/geojson" + "github.com/twpayne/go-geom/encoding/wkb" "google.golang.org/grpc" "google.golang.org/grpc/encoding/gzip" ) @@ -1571,6 +1575,113 @@ func TestJSONQueryWithVariables(t *testing.T) { require.JSONEq(t, exp, res) } +func TestGeoDataInvalidString(t *testing.T) { + dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) + require.NoError(t, err) + + ctx := context.Background() + require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) + require.NoError(t, dg.Alter(ctx, &api.Operation{Schema: `loc: geo .`})) + + n := &api.NQuad{ + Subject: "_:test", + Predicate: "loc", + ObjectValue: &api.Value{ + Val: &api.Value_StrVal{ + StrVal: `{"type": "Point", "coordintaes": [1.0, 2.0]}`, + }, + }, + } + _, err = dg.NewTxn().Mutate(ctx, &api.Mutation{ + CommitNow: true, + Set: []*api.NQuad{n}, + }) + require.Contains(t, err.Error(), "geom: unsupported layout NoLayout") +} + +// This test shows that GeoVal API doesn't accept string data. Though, mutation +// succeeds querying the data returns an error. Ideally, we should not accept +// invalid data in a mutation though that is left as future work. +func TestGeoCorruptData(t *testing.T) { + dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) + require.NoError(t, err) + + ctx := context.Background() + require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) + require.NoError(t, dg.Alter(ctx, &api.Operation{Schema: `loc: geo .`})) + + n := &api.NQuad{ + Subject: "_:test", + Predicate: "loc", + ObjectValue: &api.Value{ + Val: &api.Value_GeoVal{ + GeoVal: []byte(`{"type": "Point", "coordinates": [1.0, 2.0]}`), + }, + }, + } + _, err = dg.NewTxn().Mutate(ctx, &api.Mutation{ + CommitNow: true, + Set: []*api.NQuad{n}, + }) + require.NoError(t, err) + + query := ` +{ + all(func: has(loc)) { + uid + loc + } +}` + _, err = dg.NewReadOnlyTxn().Query(ctx, query) + require.Contains(t, err.Error(), "wkb: unknown byte order: 1111011") +} + +// This test shows how we could use the GeoVal API to store geo data. +// As far as I (Aman) know, this is something that should not be used +// by a common user unless user knows what she is doing. +func TestGeoValidWkbData(t *testing.T) { + dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) + require.NoError(t, err) + + ctx := context.Background() + require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) + require.NoError(t, dg.Alter(ctx, &api.Operation{Schema: `loc: geo .`})) + s := `{"type": "Point", "coordinates": [1.0, 2.0]}` + var gt geom.T + if err := geojson.Unmarshal([]byte(s), >); err != nil { + panic(err) + } + data, err := wkb.Marshal(gt, binary.LittleEndian) + if err != nil { + panic(err) + } + n := &api.NQuad{ + Subject: "_:test", + Predicate: "loc", + ObjectValue: &api.Value{ + Val: &api.Value_GeoVal{ + GeoVal: data, + }, + }, + } + + _, err = dg.NewTxn().Mutate(ctx, &api.Mutation{ + CommitNow: true, + Set: []*api.NQuad{n}, + }) + require.NoError(t, err) + query := ` +{ + all(func: has(loc)) { + uid + loc + } +}` + resp, err := dg.NewReadOnlyTxn().Query(ctx, query) + require.NoError(t, err) + require.Contains(t, string(resp.Json), `{"type":"Point","coordinates":[1,2]}`) +} + var addr = "http://localhost:8180" // the grootAccessJWT stores the access JWT extracted from the response diff --git a/query/query3_test.go b/query/query3_test.go index 04837d1f6bd..2c44a06842b 100644 --- a/query/query3_test.go +++ b/query/query3_test.go @@ -18,18 +18,12 @@ package query import ( "context" - "encoding/binary" "encoding/json" "fmt" "strings" "testing" - "github.com/dgraph-io/dgo/v2/protos/api" - "github.com/dgraph-io/dgraph/testutil" "github.com/stretchr/testify/require" - geom "github.com/twpayne/go-geom" - "github.com/twpayne/go-geom/encoding/geojson" - "github.com/twpayne/go-geom/encoding/wkb" "google.golang.org/grpc/metadata" ) @@ -2198,110 +2192,3 @@ func TestMultiRegexInFilter2(t *testing.T) { require.JSONEq(t, `{"data": {"q": [{"firstName": "Han", "lastName":"Solo"}]}}`, res) } } - -func TestGeoDataInvalidString(t *testing.T) { - dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) - require.NoError(t, err) - - ctx := context.Background() - require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) - require.NoError(t, dg.Alter(ctx, &api.Operation{Schema: `loc: geo .`})) - - n := &api.NQuad{ - Subject: "_:test", - Predicate: "loc", - ObjectValue: &api.Value{ - Val: &api.Value_StrVal{ - StrVal: `{"type": "Point", "coordintaes": [1.0, 2.0]}`, - }, - }, - } - _, err = dg.NewTxn().Mutate(ctx, &api.Mutation{ - CommitNow: true, - Set: []*api.NQuad{n}, - }) - require.Contains(t, err.Error(), "geom: unsupported layout NoLayout") -} - -// This test shows that GeoVal API doesn't accept string data. Though, mutation -// succeeds querying the data returns an error. Ideally, we should not accept -// invalid data in a mutation though that is left as future work. -func TestGeoCorruptData(t *testing.T) { - dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) - require.NoError(t, err) - - ctx := context.Background() - require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) - require.NoError(t, dg.Alter(ctx, &api.Operation{Schema: `loc: geo .`})) - - n := &api.NQuad{ - Subject: "_:test", - Predicate: "loc", - ObjectValue: &api.Value{ - Val: &api.Value_GeoVal{ - GeoVal: []byte(`{"type": "Point", "coordinates": [1.0, 2.0]}`), - }, - }, - } - _, err = dg.NewTxn().Mutate(ctx, &api.Mutation{ - CommitNow: true, - Set: []*api.NQuad{n}, - }) - require.NoError(t, err) - - query := ` -{ - all(func: has(loc)) { - uid - loc - } -}` - _, err = dg.NewReadOnlyTxn().Query(ctx, query) - require.Contains(t, err.Error(), "wkb: unknown byte order: 1111011") -} - -// This test shows how we could use the GeoVal API to store geo data. -// As far as I (Aman) know, this is something that should not be used -// by a common user unless user knows what she is doing. -func TestGeoValidWkbData(t *testing.T) { - dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) - require.NoError(t, err) - - ctx := context.Background() - require.NoError(t, dg.Alter(ctx, &api.Operation{DropAll: true})) - require.NoError(t, dg.Alter(ctx, &api.Operation{Schema: `loc: geo .`})) - s := `{"type": "Point", "coordinates": [1.0, 2.0]}` - var gt geom.T - if err := geojson.Unmarshal([]byte(s), >); err != nil { - panic(err) - } - data, err := wkb.Marshal(gt, binary.LittleEndian) - if err != nil { - panic(err) - } - n := &api.NQuad{ - Subject: "_:test", - Predicate: "loc", - ObjectValue: &api.Value{ - Val: &api.Value_GeoVal{ - GeoVal: data, - }, - }, - } - - _, err = dg.NewTxn().Mutate(ctx, &api.Mutation{ - CommitNow: true, - Set: []*api.NQuad{n}, - }) - require.NoError(t, err) - query := ` -{ - all(func: has(loc)) { - uid - loc - } -}` - resp, err := dg.NewReadOnlyTxn().Query(ctx, query) - require.NoError(t, err) - require.Contains(t, string(resp.Json), `{"type":"Point","coordinates":[1,2]}`) -} From 31ca81c9899565de9052b311863ac3afc02d58e9 Mon Sep 17 00:00:00 2001 From: Aman Mangal Date: Thu, 28 Nov 2019 12:40:57 +0530 Subject: [PATCH 5/5] remove shadowing of packages --- dgraph/cmd/alpha/run_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dgraph/cmd/alpha/run_test.go b/dgraph/cmd/alpha/run_test.go index 117b583d5e2..5b14be8ab61 100644 --- a/dgraph/cmd/alpha/run_test.go +++ b/dgraph/cmd/alpha/run_test.go @@ -1625,14 +1625,14 @@ func TestGeoCorruptData(t *testing.T) { }) require.NoError(t, err) - query := ` + q := ` { all(func: has(loc)) { uid loc } }` - _, err = dg.NewReadOnlyTxn().Query(ctx, query) + _, err = dg.NewReadOnlyTxn().Query(ctx, q) require.Contains(t, err.Error(), "wkb: unknown byte order: 1111011") } @@ -1670,14 +1670,14 @@ func TestGeoValidWkbData(t *testing.T) { Set: []*api.NQuad{n}, }) require.NoError(t, err) - query := ` + q := ` { all(func: has(loc)) { uid loc } }` - resp, err := dg.NewReadOnlyTxn().Query(ctx, query) + resp, err := dg.NewReadOnlyTxn().Query(ctx, q) require.NoError(t, err) require.Contains(t, string(resp.Json), `{"type":"Point","coordinates":[1,2]}`) }