Feature/supporting types graphql

[akhiltak]

New PR for #172

Need to implement/discuss:

• Use init() functionality added by Jay
• Address the ParseType func return type (it's interface{}, which should be something stricter)
• About Schema file, I am thinking it will have to be lexed using existing function and a write a parser for it. That would catch any declaration errors and form a json internally from which Schema Object Types could be constructed. Or, could there be a better way. (Not thinking directly reading a file because will have to support [] and nulltypes later and file reading code would be messy).
• strconv: add equivalents of Parsexxx() with []byte arguments golang/go#2632 (for type inference/conversion directly from a []byte)

---

This change is

[houndci-bot]

if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

[coveralls]

Coverage increased (+0.4%) to 54.563% when pulling 3535663 on feature/supporting-types-graphql into 7c0b59a on master.

[coveralls]

Coverage increased (+1.003%) to 55.365% when pulling 5a21cc1 on feature/supporting-types-graphql into 01bbadc on master.

[manishrjain]

Getting closer.

---

Reviewed 6 of 6 files at r6.
Review status: 4 of 9 files reviewed at latest revision, 25 unresolved discussions, some commit checks broke.

---

query/query.go, line 268 [r7] (raw file):

          m[sg.Attr] = string(val)
      } else {
          // the values should always be of scalar types here, do type assertion

Form proper sentences.

---

query/query_test.go, line 563 [r5] (raw file):

Previously, akhiltak (Akhil Tak) wrote…

As we discussed, keeping just the object category for now and not the more composite actors/friends type objects. Will do that in next iteration because the current system does not relies on or checks for the presence of specific object fields (which in turn become predicates), we keep global predicate types.
Please let me know if you think we should have those specific definitions before we start using them in the next step (to have a blueprint present).

`object` to me, isn't a type. It's a category. If this is for the next PR, then make it part of the next PR, and delete from here. Otherwise, convert it to a predicate-scalar map or something.

---

query/query_test.go, line 640 [r5] (raw file):

Previously, akhiltak (Akhil Tak) wrote…

As mentioned earlier, this would come when we have composite types like person/actor/friend defined. Then, we will check if the predicates following friend in the query belong to edges defined in friendType in schema.

Remove this for now then.

---

types/definitions.go, line 31 [r3] (raw file):

Previously, akhiltak (Akhil Tak) wrote…

Done. Wanted to denote it's for graphql spec but I guess it's not necessary. Comments would suffice.

Also, if you move this to gql, then `gql.Scalar`, `gql.Schema`, etc.

---

types/definitions.go, line 17 [r5] (raw file):

Previously, akhiltak (Akhil Tak) wrote…

Had kept this package separate based on our initial discussion that in starting, we would keep the type/schema system separate and then move it if we feel like it.
I also think that since we will be developing type/schema system more from Dgrpah's perspective and not strictly adhering to the GraphQL spec (but more as a guidelines), it would be okay to keep it in a separate module.
Please let me know if you think it would still serve us better to classify type system under gql package, will move it.

types is already a package, though internal to posting package. I think this can just lie within gql itself. I doubt we can use this in other languages, at least based on current understanding.

---

types/definitions.go, line 25 [r5] (raw file):

Previously, akhiltak (Akhil Tak) wrote…

Wow, that a great idea. It will work as interface implementation method as well as for assertions (in tests).
But if I use IsScalar() bool, IsObject() bool, will have to implement both methods (and prob. more) in all structs that implement this interface which will be too much.
Rather, it gives me an idea to use OfType(string) bool. How about it?

OfType string requies one to pass a string, which can be anything. If you want to keep it to just one function, then just use IsScalar() for now.

---

types/definitions.go, line 42 [r6] (raw file):

// String function to implement string interface
func (s Scalar) String() string {
  return fmt.Sprint(s.Name)

What is this function for?

---

types/definitions.go, line 47 [r6] (raw file):

// OfType function to assert scalar type
func (s Scalar) OfType(name string) bool {
  return name == "scalar"

yeah, that isn't very intuitive to the caller.

---

types/definitions.go, line 61 [r6] (raw file):

// String function to implement string interface
func (o Object) String() string {
  return fmt.Sprint(o.Name)

o.Name is already a string.

---

types/schema.go, line 31 [r5] (raw file):

Previously, akhiltak (Akhil Tak) wrote…

)
 
Hmm, can't seem to think of a better descriptive name, can't just name it schema since we already denote actual schema with that name internally.

Will keep this in mind about naming in future but for now, sfile seems like closest to what I want the flag to mean here (but won't that break camel-case convention?)
Also, we already have flags like postingDir and workerPort which follow camel-case and not the lower-case convention.

Sorry about this question, but asking this because I have come to believe in last few years that names should be descriptive to quickly and clearly indicate what values they carry.
But I figure golang wants code to be as clean and quickly readable as possible, which is why shorter names are encouraged?
Have been caught between these differing views and had a hard time naming stuff in this PR. :P
So, would be great to know your further take on this, thanks!

Why not schemaFile internally, and just schema for the flag name. That's descriptive, but without the ending capital case.

instanceIdx is the only flag name which has camelcase. And that needs to be fixed. In fact, it should just be idx.

---

types/schema.go, line 73 [r5] (raw file):

Previously, akhiltak (Akhil Tak) wrote…

Again, that would be when the composite objects are defined in schema. Let's have a quick call about this, where I could explain what I have in mind and we could go over it to iron out inconsistencies. Please let me know if that would be possible?

Sure, we can have a chat. But, the first PR can just be about the scalar types, as was the initial plan anyway. So, let's just make it about them.

---

types/schema.json, line 11 [r5] (raw file):

Previously, akhiltak (Akhil Tak) wrote…

This is the default file for schema in case no input file is being provided using the flag.
In the test, already pushing schema with a string.

We shouldn't set default values, except possibly for internal predicates, but those are internal and so they don't need to be checked via schema. So, maybe we don't need this file. This can go as an example in wiki.

---

types/schema_test.go, line 57 [r5] (raw file):

Previously, akhiltak (Akhil Tak) wrote…

Yep, I was indeed taking filepath as argument in my first implementation. But that wouldn't work with Jay's init() functionality (func AddInit() there only accepts non-parameter functions)

Why didn't we just call `LoadSchema` in main? That's how we do everything else.

---

Comments from Reviewable

[akhiltak]

Review status: 4 of 9 files reviewed at latest revision, 24 unresolved discussions, some commit checks broke.

---

types/definitions.go, line 42 [r6] (raw file):

Previously, manishrjain (Manish R Jain) wrote…

What is this function for?

to implement string interface{}, so that while printing types, just their names are printed and not the entire struct.

---

types/schema.json, line 11 [r5] (raw file):

Previously, manishrjain (Manish R Jain) wrote…

We shouldn't set default values, except possibly for internal predicates, but those are internal and so they don't need to be checked via schema. So, maybe we don't need this file. This can go as an example in wiki.

Done.

---

types/schema_test.go, line 57 [r5] (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Why didn't we just call LoadSchema in main? That's how we do everything else.

Was doing that earlier but moved it out to make it a part of package init() func. Reverted my code to initial state.

---

Comments from Reviewable

[coveralls]

Coverage increased (+1.2%) to 55.565% when pulling 5f80e6e on feature/supporting-types-graphql into 01bbadc on master.

[coveralls]

Coverage increased (+1.2%) to 55.563% when pulling ad7cb74 on feature/supporting-types-graphql into 01bbadc on master.

[manishrjain]

Reviewed 11 of 12 files at r8, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

---

cmd/dgraph/main.go, line 65 [r9] (raw file):

  nomutations = flag.Bool("nomutations", false, "Don't allow mutations on this server.")
  tracing     = flag.Float64("trace", 0.5, "The ratio of queries to trace.")
  schemaFile  = flag.String("schema", "", "Path to the file that specifies schema in json format")

hmm.. wouldn't the loader need this? If so, this should lie in the package where it's used.

---

gql/schema.go, line 65 [r9] (raw file):

  v, present := schema[p]
  if !present {
      x.Trace(ctx, "Schema does not have type definition for:%v", p)

This is a very simplistic function. This doesn't even need to be a function. Also, the trace adds little value here, because most predicates won't have an type def.

---

query/query.go, line 490 [r9] (raw file):

          isDebug:  sg.isDebug,
          Attr:     gchild.Attr,
          AttrType: gql.SchemaType(ctx, gchild.Attr),

The context is being passed just for SchemaType, which is a map lookup. I don't think this adds much value. Remove this.

---

Comments from Reviewable

[akhiltak]

Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

---

cmd/dgraph/main.go, line 65 [r9] (raw file):

Previously, manishrjain (Manish R Jain) wrote…

hmm.. wouldn't the loader need this? If so, this should lie in the package where it's used.

This is just being used here in the main func. I took you advice. So, now I am calling LoadSchema from main instead of init().

---

gql/schema.go, line 65 [r9] (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This is a very simplistic function. This doesn't even need to be a function. Also, the trace adds little value here, because most predicates won't have an type def.

Made this a function to keep `schema` as a package variable and not pollute global scope with more variables. Earlier, had logged this warning in logs but you said we should not be doing it but use Trace instead.

I want this warning to be logged somewhere, even though I know most predicates won't have type def. As a client, it would be good to know if you missed specifying a predicate->type mapping in your schema file for and edge that points to a value (and not an entity).
What do you suggest? Please let me know. :)

---

Comments from Reviewable

[coveralls]

Coverage decreased (-1.5%) to 52.838% when pulling 2549da5 on feature/supporting-types-graphql into 01bbadc on master.

[coveralls]

Coverage increased (+1.2%) to 55.577% when pulling 9d3188b on feature/supporting-types-graphql into 01bbadc on master.

[manishrjain]

Reviewed 2 of 12 files at r8, 3 of 3 files at r11.
Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

gql/schema.go, line 60 [r11] (raw file):

// SchemaType fetches types for a predicate from schema map
func SchemaType(p string) Type {
  return schema[p]

what happens if p isn't inside the schema map?

---

Comments from Reviewable

[akhiltak]

Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

gql/schema.go, line 60 [r11] (raw file):

Previously, manishrjain (Manish R Jain) wrote…

what happens if p isn't inside the schema map?

the return value will be set to nil, and will be ignored later when we validate for types (e.g., for non scalar type predicates)

---

Comments from Reviewable

[akhiltak]

Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

gql/schema.go, line 60 [r11] (raw file):

Previously, akhiltak (Akhil Tak) wrote…

the return value will be set to nil, and will be ignored later when we validate for types (e.g., for non scalar type predicates)

I check for `nil` values when I validate types in query package

---

Comments from Reviewable

[manishrjain]

Thanks for all the iterations! Feel free to submit.

---

Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[akhiltak]

Got LGTM. Closing as merged to master.

[rstorr]

@CodeLingoBot capture Check Type Before Casting

[codelingo]

Rule captured at https://dash.codelingo.io/repos/dgraph-io/dgraph/rules/1571

CodeLingoBot help

[rstorr]

@CodeLingoBot capture Avoid Ending Flag Arg Name In UpperCase

[codelingo]

Rule captured at https://dash.codelingo.io/repos/dgraph-io/dgraph/rules/1572

CodeLingoBot help

[rstorr]

@CodeLingoBot capture Don't Sprint values that are already strings

[codelingo]

Rule captured at https://dash.codelingo.io/repos/dgraph-io/dgraph/rules/1573

CodeLingoBot help

[rstorr]

@CodeLingoBot capture comments end in a full stop

[codelingo]

Rule captured at https://dash.codelingo.io/repos/dgraph-io/dgraph/rules/1574

CodeLingoBot help