Check that uid is not used as function attribute

[srfrog]

This change adds a check in newGraph() to prevent functions from using uid as an attribute. It also checks filter functions. uid is a protected attribute that should not be used directly but instead via function uid().

Additionally, this PR includes a change how empty string "" values are handled. Currently empty strings were blocked by the Gql parser. Now the empty string values are passed down to tasks and let the handler functions determine if the value is used.

e.g., New behavior with empty string values:

// Query: get all nodes with name that might be empty
{
  q(func: ge(name, "")) {
    uid
    name
  }
}

// Query: filter nodes with names at most 8 chars long.
{
  q(func: has(name)) @filter(match(name, "", 8)) {
    count(uid)
  }
}

Closes #3110

---

This change is 

[manishrjain]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @srfrog)

---

gql/parser_test.go, line 4302 at r1 (raw file):

	}{
		{in: `{q(func: eq(name, "uid")) { uid }}`},
		{in: `{q(func: gt(name, "uid")) { uid }}`, failure: true},

Why is this a failure? name can "uid".

[srfrog]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @manishrjain)

---

gql/parser_test.go, line 4302 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Why is this a failure? name can "uid".

just preserving what was already there, I can change it to allow that.

[manishrjain]

Why do this at parser level? We can do this at Subgraph level, which would be more robust.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain)

[srfrog]

At parser level we can educate the user with an error. At subgraph level it is a failure, plus wasted effort.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain)

[manishrjain]

Reviewed 5 of 5 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @manishrjain and @srfrog)

---

query/query.go, line 978 at r2 (raw file):

		} else {
			// Disallow uid as attribute - issue#3110
			if len(gq.Func.UID) == 0 {

Nice!

---

query/query1_test.go, line 1313 at r2 (raw file):

			failure: `Argument cannot be "uid`},
		{in: `{q(func:has(uid)) { uid }}`,
			failure: `Argument cannot be "uid`},

Can you bring the queries that you deleted in gql/parse_test.go here?

---

systest/queries_test.go, line 675 at r2 (raw file):

		{
			in:      `{q(func:match(term, "", 8)) {term}}`,
			failure: `Empty argument received`,

Why this change?

[srfrog]

Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions (waiting on @manishrjain)

---

query/query.go, line 978 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Nice!

Done.

---

query/query1_test.go, line 1313 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Can you bring the queries that you deleted in gql/parse_test.go here?

Sure, was missing one. It's back in.

---

systest/queries_test.go, line 675 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Why this change?

That test relied on the parser error, which is no longer needed. We will happily send the empty string and catch it later if it can't be allowed.

[manishrjain]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @manishrjain and @srfrog)

---

query/query.go, line 978 at r2 (raw file):

Previously, srfrog (Gus) wrote…

Done.

And where do we check the empty argument for eq func?

[MichelDiz]

Reviewable status: 3 of 8 files reviewed, 5 unresolved discussions (waiting on @manishrjain and @srfrog)

---

query/query1_test.go, line 1329 at r4 (raw file):

			out: `{"data":{"q":[]}}`},
		{in: `{q(func:uid(0x1)) { checkpwd(uid, "") }}`,
			out: `{"data":{"q":[{"checkpwd(uid)":false}]}}`},

I think this should return an error. UID is a reserved word. As checkpwd is a function to do comparison in Password type. So it should be denied the use of "uid" for this. Unless there's some other reason.

[srfrog]

Reviewable status: 3 of 8 files reviewed, 5 unresolved discussions (waiting on @manishrjain and @MichelDiz)

---

query/query1_test.go, line 1329 at r4 (raw file):

Previously, MichelDiz (Michel Conrado) wrote…

I think this should return an error. UID is a reserved word. As checkpwd is a function to do comparison in Password type. So it should be denied the use of "uid" for this. Unless there's some other reason.

Good point, I will change it.

[srfrog]

Reviewable status: 3 of 8 files reviewed, 5 unresolved discussions (waiting on @manishrjain and @MichelDiz)

---

query/query1_test.go, line 1329 at r4 (raw file):

Previously, srfrog (Gus) wrote…

Good point, I will change it.

Done.

[MichelDiz]

Reviewed 3 of 5 files at r2, 3 of 5 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @manishrjain)

[manishrjain]

Got a comment. Thanks!

Reviewed 3 of 5 files at r4, 2 of 2 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain and @srfrog)

---

worker/task.go, line 1598 at r6 (raw file):

		if err != nil {
			if e, ok := err.(*strconv.NumError); ok && e.Err == strconv.ErrSyntax {
				return nil, x.Errorf("Value in %s is not a number", q.SrcFunc.Name)

Might want to specify what the value is in the error. So, it's easier to figure out for the user.

[srfrog]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain)

---

worker/task.go, line 1598 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Might want to specify what the value is in the error. So, it's easier to figure out for the user.

good point, i will change it