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

Crash server when failed to convert facet value #2074

Closed
munisystem opened this Issue Feb 1, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@munisystem
Copy link

munisystem commented Feb 1, 2018

I use api.Faset{} to improve mutation performance with go client.
Just as follow.

...
nquads := []*api.NQuad {
  &api.NQuad{
    Subject: "0x01",
    Predicate: "friend",
    ObjectId: "0x02",
    Fasets: []*api.Facet{
      {
        Key: "since",
        Value: []bytes(time.Now().Format(time.RFC3339)),
        ValType: api.Faset_DATETIME,
      },
    },
  },
}
mu := &api.Mutation{Set: nquads, CommitNow: true}
resp, err := txn.Mutate(context.Background(), mu)
...

But this code cause server to crash.
( Valuable of api.Faset.Value is invalid. Actually, should be used time.Now().MarshalBinary())
Because call log.Fatal() when failed to convert facet value.

e.g.

x.AssertTruef(err == nil,
"We should always be able to covert facet into val. %v %v", f.Value, typId)

dgraph/x/error.go

Lines 57 to 75 in 02dc3bd

// AssertTrue asserts that b is true. Otherwise, it would log fatal.
func AssertTrue(b bool) {
if !b {
log.Fatalf("%+v", errors.Errorf("Assert failed"))
}
}
// AssertTruef is AssertTrue with extra info.
func AssertTruef(b bool, format string, args ...interface{}) {
if !b {
log.Fatalf("%+v", errors.Errorf(format, args...))
}
}
func AssertTruefNoTrace(b bool, format string, args ...interface{}) {
if !b {
log.Fatalf("%+v", fmt.Errorf(format, args...))
}
}

I think that should be return error instead of log.Fatal() in public API.
How about you?

@pawanrawal pawanrawal self-assigned this Feb 1, 2018

@pawanrawal pawanrawal added the bug label Feb 1, 2018

@pawanrawal

This comment has been minimized.

Copy link
Contributor

pawanrawal commented Feb 5, 2018

This issue is reproducible when querying for a facet as ValFor is only called in the query path. We should have some kind of validation while mutating the facet so that this error is not encountered. To reproduce, run the mutation mentioned above and then query for the facet.

@munisystem I doubt mutation performance would improve much because of this as parsing the facet is not the bottleneck. You should typically use the JSON format to set the facets or the string format (as part of RDF's). Though if you really want to set it like this, I'd suggest using FacetFor so that value is converted to a format the server expects.

@munisystem

This comment has been minimized.

Copy link
Author

munisystem commented Feb 7, 2018

@munisystem I doubt mutation performance would improve much because of this as parsing the facet is not the bottleneck. You should typically use the JSON format to set the facets or the string format (as part of RDF's).

I guess so.
But I have other intention.
For example, cannot use JSON in case of that add a user and many friends having facets like https://docs.dgraph.io/mutations/#facets.

this json object doesn't apply facets

{
  "name": "Alice",
  "friend": [
    {
      "name": "Bob",
      "~friend|close": "yes"
    },
    {
      "name": "Carol",
      "~friend|close": "no"
    },
  ]
}

So I have to divide query adding "Alice" and friends or to use string formats.
However, above approaches complicate coding and more compared to using JSON...

Though if you really want to set it like this, I'd suggest using FacetFor so that value is converted to a format the server expects.

Sounds good.
I try to use it, thanks.

@pawanrawal

This comment has been minimized.

Copy link
Contributor

pawanrawal commented Feb 7, 2018

Try with the JSON below. You don't need to add ~. Reverse edges automatically get the same facets as the forward edges.

{
  "name": "Alice",
  "friend": [
    {
      "name": "Bob",
      "friend|close": "yes"
    },
    {
      "name": "Carol",
      "friend|close": "no"
    },
  ]
}

@manishrjain manishrjain added the bug label Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.