Add support for expand on type #3949
Conversation
Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
![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.
@balajijinnah you can click here to see the review status or cancel the code review job.
| span.Annotate(nil, fmt.Sprintf("expand(%s)", child.Params.Expand)) | ||
| _, ok := types[child.Params.Expand] | ||
| if !ok { | ||
| return out, errors.Errorf("Invalid expand type %d", child.Params.Expand) |
There was a problem hiding this comment.
printf: Errorf format %d has arg child.Params.Expand of wrong type string (from govet)
| if !ok { | ||
| return out, errors.Errorf("Invalid expand type %d", child.Params.Expand) | ||
| } | ||
| preds = getPredicatesFromTypes(map[string]struct{}{child.Params.Expand: struct{}{}}) |
There was a problem hiding this comment.
File is not gofmt-ed with -s (from gofmt)
| preds = getPredicatesFromTypes(map[string]struct{}{child.Params.Expand: struct{}{}}) | |
| preds = getPredicatesFromTypes(map[string]struct{}{child.Params.Expand: {}}) |
| // We already have the predicates populated from the var. | ||
| preds = getPredsFromVals(child.ExpandPreds) | ||
| // If type exist in expand function, check whether that type exist for the node or not. | ||
| // If not then thorow error. If exist obtain all the predicate for the type and expand |
| if !ok { | ||
| return out, errors.Errorf("Invalid expand type %d", child.Params.Expand) | ||
| } | ||
| preds = getPredicatesFromTypes(map[string]struct{}{child.Params.Expand: struct{}{}}) |
There was a problem hiding this comment.
why are we converting this into string?
There was a problem hiding this comment.
The change itself looks good to me but
- Are there any associated test cases with these files? Please update them if there are - or consider adding them if they are not present alredy (I would assume the former for this repo)
- Please make any documentation changes if necessary to reflect your change.
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
- I already have a PR to do this Add ability to expand only on certain types. #3920
- This needs tests.
- The test will probably fail if you run a query like
expand(Person) {
uid
}
some uids that should not show up will show up in that case. That's the reason my PR is still a draft. I need to figure out what the issue is.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @animesh2049, @balajijinnah, @gitlw, @golangcibot, @manishrjain, and @martinmr)
There was a problem hiding this comment.
Also, we shouldn't get rid of all, reverse, etc. The type argument is an addition, not a replacement to the current expand functionality.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @animesh2049, @balajijinnah, @gitlw, @golangcibot, and @manishrjain)
|
I'm closing this. |
close #3903
This PR will add support to use type on expand function.
Changes made
_all_. Now expand function can take any paramenter. But the given parameter should be assoicated with type.Now, user can query like this
Signed-off-by: பாலாஜி ஜின்னா [email protected]
This change is