Added Val support for JSON #3947
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ A review job has been created and sent to the PullRequest network.
@harshil-goel you can click here to see the review status or cancel the code review job.
dgraph/cmd/alpha/upsert_test.go
Outdated
| } | ||
| }` | ||
| testutil.CompareJSON(t, res, expectedRes) | ||
|
|
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @harshil-goel and @manishrjain)
chunker/json_parser.go, line 155 at r1 (raw file):
} if strings.HasPrefix(s, "val(") {
I would also add a check that the parenthesis are properly closed.
chunker/json_parser_test.go, line 599 at r1 (raw file):
} func TestValInUpsert(t *testing.T) {
Add another test with an input such as {"uid":1000, "name": "val(name"} and make sure the parsing throws an error.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @harshil-goel)
There was a problem hiding this comment.
The test strings.hasPrefix("val(") could benefit from accepting a constant value for readability. Please review the comment describing the issue.
Reviewed with ❤️ by PullRequest
chunker/json_parser.go
Outdated
| @@ -152,6 +152,11 @@ func handleBasicType(k string, v interface{}, op int, nq *api.NQuad) error { | |||
| return nil | |||
| } | |||
|
|
|||
| if strings.HasPrefix(s, "val(") { | |||
There was a problem hiding this comment.
This is a somewhat unique structure to introspect for, should it be in a constant that others can use and or verify for correctness without looking for this exact string?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mangalaman93, @martinmr, and @pullrequest[bot])
chunker/json_parser.go, line 155 at r1 (raw file):
Previously, pullrequest[bot] wrote…
This is a somewhat unique structure to introspect for, should it be in a constant that others can use and or verify for correctness without looking for this exact string?
Done.
chunker/json_parser.go, line 155 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
I would also add a check that the parenthesis are properly closed.
It is done later while parsing the NQuad.
chunker/json_parser_test.go, line 599 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
Add another test with an input such as
{"uid":1000, "name": "val(name"}and make sure the parsing throws an error.
Done.
chunker/json_parser_test.go, line 602 at r1 (raw file):
Previously, mangalaman93 (Aman Mangal) wrote…
check the structure of the returned object too.
Done.
dgraph/cmd/alpha/upsert_test.go, line 147 at r1 (raw file):
Previously, mangalaman93 (Aman Mangal) wrote…
extra new line
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @mangalaman93, @manishrjain, @martinmr, and @pullrequest[bot])
chunker/json_parser.go, line 155 at r1 (raw file):
Previously, harshil-goel wrote…
It is done later while parsing the NQuad.
Actually, I got confused with RDF Lexing, and there was no such check. I have added the appropriate check now.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @manishrjain, @martinmr, and @pullrequest[bot])
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pullrequest[bot])
This lets the user to use val function inside a JSON format upsert.
This change is