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.

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

More information in response for Upsert #4048

Closed
MichelDiz opened this issue Sep 23, 2019 · 5 comments
Closed

More information in response for Upsert #4048

MichelDiz opened this issue Sep 23, 2019 · 5 comments
Assignees
Labels
area/upsert Issues related to upsert operations. kind/enhancement Something could be better. priority/P2 Somehow important but would not block a release. status/needs-specs Issues that require further specification before implementation.

Comments

@MichelDiz
Copy link
Contributor

MichelDiz commented Sep 23, 2019

Experience Report

What you wanted to do

I would like to execute Upserts transactions and have a logical response from what is happening. Sometimes we send a transaction that does not return contextual information about the transaction itself. We only have some information when the transaction creates a new object in the DB. But it's not human-friendly.

Why that wasn't great, with examples

For example, if I want to bind two entities in one Upsert. I will get a "Success" + "Done" code and an empty "uids" field. This information does not help me debug what is happening. I know there was writing because I can check directly by making a new query.

{
  data: { code: 'Success', message: 'Done', uids: {} },
  extensions: {
    server_latency: { parsing_ns: 73769, processing_ns: 5960909 },
    txn: { start_ts: 653, commit_ts: 654 }
  }
}

What we need is something like (That would be just a sketching idea, certainly should follow established standards/patterns):

If the user sends an upsert and there was a writing in Dgraph.

{
  data: { code: 'Success', recorded: 'true', message: 'Done', uids: { 'uid(USER)': '0x5092d' } }
 //(...)
}

If the user sends an upsert and there was no writing in Dgraph.

{
  data: { code: 'Success', recorded: 'false', message: 'Done', uids: {} }
 //(...)
}

Now let's say I use "cond()" on a mutation. I have no information about the context of this condition when performed. Whether it wrote it or not. Let's say my cond is "@if (NOT eq (len (USER), 1))" - ie it will only write if the value is zero. This happens without the dev/user being aware of what is happening.

In the Cond hypothesis, it would be ideal to return values ​​confirming or not the writing.

If the user sends an upsert with a condition (@if(NOT eq(len(USER), 1))) and there was a writing in Dgraph.

In this case, we assume that the result of the condition was "found no user". Since the condition proposal matches the result then the answer is "cond: 'true'".

{
  data: { code: 'Success', recorded: 'true', message: 'Done', uids: { 'uid(USER)': '0x5092d' }, cond: 'true' }
 //(...)
}

In the case of multiple conditions (as it would be in the case of multiple mutations), then each new condition would have to have a field of its own. e.g. cond0: 'true', cond1: 'false', cond2: 'true',

Maybe

cond: {0: 'true', 1: 'false', 2: 'true'},

named mutations

cond: { "userMutation": 'true', "schoolEnroll": 'false', "inSelection": 'true'},
@MichelDiz MichelDiz added the area/upsert Issues related to upsert operations. label Sep 23, 2019
@mangalaman93 mangalaman93 added kind/enhancement Something could be better. priority/P2 Somehow important but would not block a release. status/needs-specs Issues that require further specification before implementation. labels Sep 24, 2019
@pawanrawal
Copy link
Contributor

I am going to add another field back to the Response called Mutated with the type map[string][]string where the map would contain the variables used in the mutations and the corresponding uids. This would tell the user if any mutations happened and what uids were involved in it.

@mangalaman93 had a good point that this could make the response large for bulk upserts, so we could return these uids only when a flag is set to true. We could use the debug flag as well for this.

This doesn't exactly give what you are looking for here @MichelDiz but is required anyway for the GraphQL layer, hence I am going to add it anyway. We can decide later if we want to return the result of the conditions that were evaluated as part of the request.

@MichelDiz
Copy link
Contributor Author

MichelDiz commented Oct 9, 2019

PS. What do you think?

Attention to those who read this in the future, nothing that is written below works. Are just a discussion.

@pawanrawal In addition to that I was thinking of something more "optional", like this conversation with a user https://dgraph.slack.com/archives/C13LH03RR/p1570556249210700?thread_ts=1570520340.199700&cid=C13LH03RR

This could be supported instead of what I proposed above in the issue. However, it would be interesting to extend to all mutations (not just upserts) as well, making the mutations "GraphQL like" once for all*.

In this case, the user could choose if he wanna in the response all of it, part of it or something else.

e.g:

upsert {
  query {
    USER as var(func: eq(sha256, "abc")) {
    found as count(uid)
   }
  }
  mutation @if(eq(len(u), 0)) {
    set {
      uid(USER) <sha256> "abc" .
      uid(USER) <dgraph.type> "File" .
    }
  }
  return query {
  # The user can return as many blocks he needs.
  # This would be optional, If the user does not type "return query", then will return only the information we return todays.

   #this first block is the one that will return the updated data
    me(func: uid(USER)) {
      uid
      expand(_all_)
    }

    #This is a custom block - Everything here are just examples and would be optional.
    infoMe() { 
      mutation : (len(USER), 0) #This would return TRUE/FALSE if we support len() in the query body - it is just an idea
      total_sum as sum: sum(val(found))
      mutation2 : math(total_sum == 0 ) # This actually works in queries today. It will return TRUE.
    }

  }
}

About the mutations, we just add an extra section.

something like e.g:

{
  set {
    _:alice <name> "Alice" .
  }
  query {
      #"alice" is an implicit variable coming from the blank node. 
    q(func: uid(alice) ) { 
      uid
      name
    }  
  }
}

We could also just assume that all is implicit and avoid creating hard typing. Just like GraphQl.
That would be good because it would have the same behavior as GraphQL. Which immediately returns the mutated values ​​in a single batch.

In Dgraph it's kind of annoying to do the mutation and then a new transaction to get the data I need.

{
  set {
    _:alice <name> "Alice" .
  }
  query { 
      uid
      name
  }
}

JSON

{
  "set": [
    {
     "uid": "_:alice",
     "name": "Alice"
    }
  ],
  "query":  " { \n uid  \n name }" 
}

@mangalaman93 mangalaman93 changed the title Upserts: Create a better transaction execution message. More information in response for Upsert Oct 9, 2019
@devinbost
Copy link

This feature will also be helpful for my team's use cases.

@mangalaman93
Copy link
Contributor

#4210 changes the way we return the response.

@MichelDiz
Copy link
Contributor Author

Closing this in favor of #4653

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/upsert Issues related to upsert operations. kind/enhancement Something could be better. priority/P2 Somehow important but would not block a release. status/needs-specs Issues that require further specification before implementation.
Development

No branches or pull requests

4 participants