feat(graphql): adds @default directive for setting default field values at create and update

[dpeek]

Adds support for @default directive to GraphQL input schema.

Syntax:

type Type {
  field: FieldType @default(
    add: {value: "value"}
    update: { value: "value"}
  )
}

Where a value is not provided as input for a mutation, the add value will be used if the node is being created, and the update value will be used if the node exists and is being updated. Values are provided as strings, parsed into the correct field type by Dgraph.

The string $now is replaced by the current DateTime string on the server, ie:

type Type {
  createdAt: DateTime! @default(
    add: { value: "$now" }
  )
  updatedAt: DateTime! @default(
    add: { value: "$now" }
    update: { value: "$now" }
  )
}

Schema validation will check that:

• Int field values can be parsed strconv.ParseInt
• Float field values can be parsed by strconv.ParseFloat
• Boolean field values are true or false
• $now can only be used with fields of type DateTime (could be extended to include String?)

Schema validation does not currently ensure that @default values for enums are a valid member of the enum, so this is allowed:

enum State {
  HOT
  NOT
}

type Type {
  state: State @default(add: { value: "FOO"})
}

Schema validation also prevents use of @default on:

• Types with @remote directive
• Fields of type ID
• Fields that are not scalar types (Int, Float, String, Boolean, DateTime) or an enum
• Fields that are list types (ie `[String])
• Fields with @id, @custom, @lambda

Output schema changes:

• Fields with @default(add) values become optional in AddTypeInput

---

This change is 

[maaft]

Could you add arguments to both directives?

@createdAt(allowMutate: true)
@updatedAt(allowMutate: true)

default would be false.

[dpeek]

If you need to manually update your server generated timestamps, you might be doing it wrong :) Are you on the Dgraph team? It's not something I need but happy to add if needed for merging.

Thinking about it, the behaviour would be kind of confusing... the fields would be available on mutation inputs (update / upsert) but would get set anyway if you didn't provide a value. Might make more sense on @created but not @updated?

Also happy to take input on the name of the directives, was trying not to overthink it but maybe createdAt updatedAt are better?

[aman-bansal]

Hi @dpeek @maaft, I am from the Dgraph team. Really happy that you started to work on this. But I have few suggestions for you. The way i see it @created and @Updated kind of creates a confusion and has very limited scope of improvement and extension. For example, what if i just want to have a default value say 1 for Int data type etc.

My suggestion is to have something like @default as the directive to set a default value for a field in a type if a value for it wasn’t provided in mutation. Example:

type Author {
 id: ID!
 name: String!
 credits: Int @default(add: {value: 5})
 createdAt: DateTime @default(add: {expr: $now-4s})
 updatedAt: DateTime @default(add: {expr: $now-4s}, update: {expr: $now})
}

Now we can extend the list of keywords in future easily and for now $now would be enough for data time use case.
Do let me know if this makes sense to you. I am happy to help

[dpeek]

Hi @aman-bansal, thanks for your response and the (much better) suggested approach! I kind of assumed you guys would have an approach in mind, happy to update the implementation to match that.

What do you think about the ability to remove fields from mutation inputs, perhaps not just for timestamps but other fields with default or computed values. Do you have a directive schema in mind for something like that?

Also regarding my question about how best to provide the timestamp, does context.Context make sense? And how should I mock it for the context of the tests?

I'll get started on changing the directive schema and update here when done. This is some of the first Go I'm writing, so if the scope grows beyond small adjustments I might need some guidance :)

[aman-bansal]

For the schema I was thinking to always have non nullable option in schema itself. So for example for a type Defined like this

type Author {
 id: ID!
 name: String!
 credits: Int! @default(add: {value: 5})
}

both are correct mutations

mutation {
 addAuthor(input: [{name: "Shakespeare"}]) { ... }
}

and

mutation {
 addAuthor(input: [{name: "Shakespeare", credits: 10 }]) { ... }
}

i would suggest to integrate basic feature first. Having just add and update options with default value and $now as one single key word is enough. you can defer expr for now. That was just for example.

Really sorry but I dont understand this question Also regarding my question about how best to provide the timestamp, does context.Context make sense? And how should I mock it for the context of the tests?

The way i see it, you wanted to know at which point you should assign timestamps or evaluate default values, I would suggest not to take timestamps or other default values across the code with context. You will get stuck across multiple validations which dgraph might have. https://github.com/dgraph-io/dgraph/blob/master/graphql/resolve/mutation.go#L105 Here we rewrite the GraphQL queries into DQL queries. This seems like the best place to me to evaluate and apply all the default values.

[maaft]

@dpeek My reasoning for allowing mutating these fields optionally is the following:
GraphQL is a very nice standard, with a bunchload of awesome tooling at hand, for instance client code generation in many languages.

Of course you could directly use DQL, but honestly, nobody wants to learn it who's coming from the GraphQL world. DQL is very complex at times and the GraphQL <-> DQL translation already exist.

By the way, my use-case is synchronizing an offline database (same schema) with the always-online main dgraph instance. There I need control over the timestamps, otherwise the UI would show different information when connected to the offline database or the online database.

@aman-bansal I very much like the @default approach you suggested.

This could also finally fix issues when doing schema migrations which introduce new predicates. Currently, I have to manually update all database entries which is non-ideal.

[dpeek]

@aman-bansal I've just pushed the main changes to get @default working. Need to add a bunch of tests making sure different scalar types are working, will work on those next.

I see your point about my timestamp question: I was actually asking whether the timestamps for multiple node updates in a single mutation should be exactly the same, but that's kind of silly given we're talking about milliseconds. So now we just create the timestamp when we request the default value in mutation_rewriter.go. It seemed the best place to put it as we know while rewriting whether the node is an add or an update.. but happy to try move it to mutation.go if you think it fits better there.

My remaining question about timestamps is how I should go about mocking the time for the tests in add_mutation_test.yaml and update_mutation_test.yaml. We either need someway or returning a static value during tests or having the test harness recognise a DateTime and act accordingly. Let me know how you think it should be handled.

@maaft @default fields are back in mutation inputs for now as the ability to remove them should probably be another directive and I want to keep this PR focused on @default

[dpeek]

Ok, it's looking better now. Added some more tests and simplified getting the default value for a field. Main other things I might test are deep add mutations and making sure fields with @default provided by interfaces work correctly – but assume those are covered by other tests?

A few notes / questions:

In testing different scalars in the yaml tests, when an Int is provided in gqlvariables I get:

input: variable.patch.set.count cannot use float64 as Int

A quick look around suggests that no other tests are testing Ints either :P

Defaults are currently provided only as String, I suppose Dgraph will coerce to correct type on mutation? Not sure how we could support strict types here... Unless maybe @default(add: {date: DateTime, int: Int, string: String ...}) (although this still doesn't work for enums)

For now I'm handling unit tests like this when getting the default value:

if value.Raw == "$now" {
  if flag.Lookup("test.v") == nil {
    return time.Now().Format(time.RFC3339)
  } else {
    return "2000-01-01T00:00:00.00Z"
  }
}

Let me know if there is some better way :)

[maaft]

@dpeek That's awesome! Thank you so much for your work! :)

[aman-bansal]

@dpeek is it ready for review?

[dpeek]

Sure, there a a couple of questions above in my comments but probably worth you taking a look now to see if it in the right direction :)

[dpeek]

@aman-bansal any chance you will get to this this week? My availability to address feedback will be limited next week.

[aman-bansal]

@dpeek Thank you so much for working so hard on this feature. Me and my team will review the PR this week. I have already shared the PR in internal channels for a review

[aman-bansal]

Reviewed 61 of 68 files at r2, 6 of 7 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dpeek)

---

graphql/schema/rules.go, line 1300 at r4 (raw file):

	secrets map[string]x.Sensitive) gqlerror.List {
	if typ.Directives.ForName(remoteDirective) != nil {
		return []*gqlerror.Error{gqlerror.ErrorPosf(

lets add one more validation where $now can be part of fields whose data type are DateTime! Also values should be of same data type as their fields definitions. Say "user-random" cannot be default value for a field with Int Data type

---

graphql/schema/wrappers.go, line 2372 at r4 (raw file):

	}
	if value.Raw == "$now" {
		if flag.Lookup("test.v") == nil {

Let's not put testing logic in the code. we can put non null checks and time date parsing logic to verify that things are working fine. I guess that would be enough validation to work with.

[NamanJain8]

The change looks really nice to me. Thanks, David for the work. 🎉

[dpeek]

Reviewable status: 65 of 68 files reviewed, 4 unresolved discussions (waiting on @aman-bansal, @Custom, @default, @deprecated, @dgraph, @id, @include, @lambda, @NamanJain8, @secret, and @Skip)

---

graphql/schema/rules.go, line 1294 at r4 (raw file):

Previously, NamanJain8 (Naman Jain) wrote…

I think there are several other checks that need to be incorporated (thought of) here.
Like if it should work with secret, dgraph, custom, etc directives. It is possible that we might be handling that case somewhere which I am not aware of.

Ok, I've reviewed the existing directives and implemented checks based on my understanding:

@auth - allowed, auth rules run after schema validation, mutation rewrite
@cascade - NA, not in schema def
@lambda + @Custom - not allowed, added to validation
@deprecated - allowed
@dgraph - allowed, pred mapping handled in mutation rewrite
@generate - allowed
@hasInverse - allowed, @default not allowed on edges to types
@id - not allowed
@include - NA, not in schema def
@Remote - not allowed
@remoteResponse - allowed
@search - allowed
@secret - allowed, password check is handled seperately
@Skip - NA, not in schema def
@withSubscription - allowed
@lambdaOnMutate - allowed

---

graphql/schema/rules.go, line 1299 at r4 (raw file):

Previously, NamanJain8 (Naman Jain) wrote…

Also, it would be great to have a test that can check the functioning of these validations.

OK, I've added tests for each to graphql/schema/gqlschema_test.yml

---

graphql/schema/rules.go, line 1300 at r4 (raw file):

Previously, aman-bansal (aman bansal) wrote…

lets add one more mutation where $now can be part of fields whose data type are DateTime! Also values should be of same data type as their fields definitions. Say "user-random" cannot be default value for a field with Int Data type

OK, added type validations for Int, Float, and Boolean. $now verifies field DateTime, but I'm not sure how DateTime string should be validated – ie how Dgraph is parsing strings when storing values. Also I'm parsing Floats / Ints as 64bit right now, can you confirm that is right?

---

graphql/schema/wrappers.go, line 2372 at r4 (raw file):

Previously, aman-bansal (aman bansal) wrote…

Let's not put testing logic in the code. we can put non null checks and time date parsing logic to verify that things are working fine. I guess that would be enough validation to work with.

OK, I've removed the testing logic, but I'm still not sure how you want to change test harness to support this kind of validation so the test suite is currently failing.

It seems like we need to add some logic to graphql/resolve/mutation_test.go:245 to recognise we are comparing two date times and support some kind of matching, but without inspecting the schema first I don't know how to do this in a non-fragile way. Suggestions appreciated :)

[maaft]

@id - not allowed

Do you have in mind that this could change in the future?

See last post:
https://discuss.dgraph.io/t/feature-request-directive-that-indicates-that-id-fields-should-be-auto-populated-on-creation/14992/7?u=maaft

[dpeek]

@maaft sure, @aman-bansal hinted at some other use-cases in his original response – support for expressions and other tokens like $now that that would resolve to something like a GUID.

@aman-bansal do you think we need to create a distinction between value (literal values) and expr ($now or other future additions) to ensure future flexibility, or are you OK with the content of value being parsed looking dollar prefixed tokens?

[dpeek]

Theres one lint warning for line length remaining, but it seems like most of the function signatures ignore it anyway so I went for consistency over correctness :)

[dpeek]

Hi team Dgraph 👋🏻 Any feedback on changes?

[dpeek]

Gentle reminder :D

[iyinoluwaayoola]

Thanks @dpeek. @aman-bansal Did you consider the proposal here? https://discuss.dgraph.io/t/graphql-error-non-nullable-field-was-not-present-in-result-from-dgraph/9503/6?u=iyinoluwaayoola

The design uses @input directive to define fields that should be allowed in the schema and their default value... seem very similar here. I find the ability to skip fields in the schema very powerful and useful as well.

[dpeek]

No comment from Dgraph team in a month... I guess you guys are busy right now, but given all the community shouting for missing GraphQL features, ignoring contributions seems like an unwise strategy :)

Really the only missing bit is some advice on what I should do WRT to testing timestamps. Should I build a specific test harness for just for these? Adapt the existing one? I'm still finding my feet in golang so I worry if I can't just adapt something that exists I'll likely implement something that doesn't align with the ethos of the rest of the codebase.

[NamanJain8]

Hey @dpeek , I apologize for the delay. The team was busy with the next release-related stuff, etc. Hence, we deferred this for later. This change looks great to me and much appreciated.

Really the only missing bit is some advice on what I should do WRT to testing timestamps.

It should be okay to use test.v flag lookup that you had earlier.

Thanks a lot for the PR. I am approving it. We can merge it once CI passes.

[NamanJain8]

Nice change. 🎉

[aman-bansal]

@dpeek can you please fix the test cases? Once fixed we can merge the PR

[dpeek]

Hi @aman-bansal, I brought back the test flag check. I don't have much visibility into failing CI details so you might need to address those yourselves. I'm also moving on from dgraph in my project, so won't have much more time to invest in it.

[codeninja]

@dpeek I might pick this up as the timestamp feature is rather important to me but I wouldn't be able to do so until later. In the meantime, the blocking changes seem to be pretty simple. The failing lint test is shown here in GraphqlWrappers:

I suspect simply changing that conditional will pass the lint.

[tylerstillwater]

This should fix the lint error.

Suggested change

	} else {
	return "2000-01-01T00:00:00.00Z"
	}
	}
	return "2000-01-01T00:00:00.00Z"

[dpeek]

🥳

[iyinoluwaayoola]

Just adding this here for when it may be picked up for a future release.

Food for thought: why not use [gotemplate](https://pkg.go.dev/text/template) for templated default values to satisfy the expression use cases. With this, we can use [sprg](http://masterminds.github.io/sprig/) functions like now etc. We can even provide the non-defaulted predicate values, headers, or auth values as additional context to render.

For example, the default value can be {{ .parent.name }} which will result to the predicate field called name.
Or {{ .auth.workspace }} will default to the workspace value in the auth token.
Or {{ .headers.workspace }} will default to the workspace value in the request headers.
Or {{ uuidv4 }} will default to a uuid.
Or {{ now }}, will result to the current timestamp.
Or an expression {{ now | date_modify "+2h" }} will result to 2hrs later than the time of evaluation.

This may even support edges who's IDs can be templated. I.e.,
{ "id": "{{ .parent.name }}" } will create or link an edge that share the same ID as the parent.
Or { "id": "{{ uuidv4 }}" } will create or link an edge with a unique identifier.