Harshil goel/e2e tests #5312
Harshil goel/e2e tests #5312
Conversation
There was a problem hiding this comment.
I like the table driven test that shows how different perspectives affect the result.
Not sure why this has the RBAC tests in it.
I'm happy to merge a minimal working version and then come back and refine/add tests. So don't add more tests at this point. Let's just get a working version merged.
Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @harshil-goel and @manishrjain)
graphql/e2e/auth/auth_test.go, line 73 at r1 (raw file):
because the schema says
permission: Permission @search
shouldn't these not be []
Also why is it "Permissions" does that deserialize correctly ?
graphql/e2e/auth/auth_test.go, line 135 at r1 (raw file):
testCases := []TestCase{{ user: "user1", role: "ADMIN",
I thought RBAC wasn't working ... how does this work?
graphql/e2e/auth/auth_test.go, line 198 at r1 (raw file):
func rootGetFilter(t *testing.T, col *Column, tcase TestCase) { testCases := []TestCase{{
why a slice? Looks like it can only ever be one value
graphql/e2e/auth/auth_test.go, line 255 at r1 (raw file):
role: "USER", result: `{"queryColumn": [{"name": "Column2"}, {"name": "Column3"}]}`, }}
yeah, I really like these table driven tests that show what different users see for the same query.
graphql/e2e/auth/auth_test.go, line 291 at r1 (raw file):
if len(result.QueryColumn) > 0 { rootGetFilter(t, result.QueryColumn[0], tcase)
I don't like this. I'd rather have one case for query and one for get. The query case can look like this fn and the get case should have a similar list of cases that show what different users can see.
graphql/e2e/auth/auth_test.go, line 297 at r1 (raw file):
} func TestRBACFilter(t *testing.T) {
again, I'm not sure how this works
graphql/e2e/auth/auth_test.go, line 544 at r1 (raw file):
query := ` query { queryMovie (order: {asc: content}) {
I thought this needed a change in the dgraph folder to make multiple queries write properly?
graphql/e2e/auth/test_data.json, line 104 at r1 (raw file):
because the schema says
permission: Permission @search
shouldn't these not be []
graphql/e2e/common/common.go, line 196 at r1 (raw file):
} client := dgo.NewDgraphClient(api.NewDgraphClient(d)) err = client.Alter(ctx, &api.Operation{DropAll: true})
I think this will result in badness. DropAll and GraphQL are incompatible because the GraphQL API can't refresh its schema properly after a DropAll.
Is this needed? It wasn't for e2e testing before.
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 9 unresolved discussions (waiting on @harshil-goel, @manishrjain, and @MichaelJCompton)
graphql/e2e/auth/auth_test.go, line 73 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
because the schema says
permission: Permission @searchshouldn't these not be []
Also why is it "Permissions" does that deserialize correctly ?
Done.
graphql/e2e/auth/auth_test.go, line 135 at r1 (raw file):
Done in different PR.
graphql/e2e/auth/auth_test.go, line 198 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
why a slice? Looks like it can only ever be one value
Done.
graphql/e2e/auth/auth_test.go, line 255 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
yeah, I really like these table driven tests that show what different users see for the same query.
Done.
graphql/e2e/auth/auth_test.go, line 297 at r1 (raw file):
Done in different PR.
graphql/e2e/auth/auth_test.go, line 544 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I thought this needed a change in the dgraph folder to make multiple queries write properly?
Done in different PR.
graphql/e2e/auth/test_data.json, line 104 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
because the schema says
permission: Permission @searchshouldn't these not be []
Done.
graphql/e2e/common/common.go, line 196 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I think this will result in badness. DropAll and GraphQL are incompatible because the GraphQL API can't refresh its schema properly after a DropAll.
Is this needed? It wasn't for e2e testing before.
Yup, this was resulting in failing tests due to DropAll. It was working before we merged the latest master. The idea behind adding a DropAll was that when we re-run tests, we inject the test_data.json again, failing the tests.
There was a problem hiding this comment.
Still contains lots of changes unrelated to this PR. If the RBAC stuff is coming in another PR, then this should just contain the changes to get the e2e tests going for what's working at the moment.
A set of table driven tests that show the various cases to investigate is what's required in this PR. The RBAC one should then be adding the cases for RBAC. It's too hard to work out what's going on when there is unrelated material, particularly when I can't see how that unrelated material even runs.
Reviewable status: 3 of 5 files reviewed, 10 unresolved discussions (waiting on @harshil-goel, @manishrjain, and @MichaelJCompton)
graphql/e2e/auth/auth_test.go, line 135 at r1 (raw file):
Previously, harshil-goel (Harshil Goel) wrote…
Done in different PR.
looks like a change unrelated to this PR
graphql/e2e/auth/auth_test.go, line 198 at r1 (raw file):
Previously, harshil-goel (Harshil Goel) wrote…
Done.
I'd like to see a table driven test like TestRootFilter that clearly shows the different conditions under which you do or don't get something back from the get
graphql/e2e/auth/auth_test.go, line 297 at r1 (raw file):
Previously, harshil-goel (Harshil Goel) wrote…
Done in different PR.
but how does it run? If this is unrelated to this PR, then this function should remain skipped and have no changes
graphql/e2e/auth/auth_test.go, line 347 at r2 (raw file):
} func TestAndRBACFilter(t *testing.T) {
same here, I'm confused as to how this runs or why this was changed if it's unrelated to this PR.
graphql/e2e/auth/test_data.json, line 104 at r1 (raw file):
Previously, harshil-goel (Harshil Goel) wrote…
Done.
looks unchanged?
graphql/e2e/common/common.go, line 196 at r1 (raw file):
Previously, harshil-goel (Harshil Goel) wrote…
Yup, this was resulting in failing tests due to DropAll. It was working before we merged the latest master. The idea behind adding a DropAll was that when we re-run tests, we inject the test_data.json again, failing the tests.
The e2e tests have a guard (allCountriesAdded) that check if the data is already there. That was meant to be so that you could run the tests multiple times in development without stopping the server or reloading data. You probably need to do something like that here.
There was a problem hiding this comment.
This is much better. I've noted some other tests that I think are required.
Reviewed 1 of 5 files at r3.
Reviewable status: 2 of 6 files reviewed, 19 unresolved discussions (waiting on @harshil-goel, @manishrjain, and @MichaelJCompton)
graphql/dgraph/graphquery.go, line 66 at r3 (raw file):
} if !root && query.Func == nil && hasOrderOrPage(query) {
Is the root argument needed at all now? Seems like this test means it could be removed.
Also, you've added this because there's now cases that mean what was there before wasn't good enough ... so I think you need to add a rewriting test to show that it writes the correct result. There's a test TestAuthMutationQueryRewriting add a case there with order and page to show that the rewriting is as expected and also in auth_query_test.yaml
graphql/e2e/auth/auth_test.go, line 198 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I'd like to see a table driven test like TestRootFilter that clearly shows the different conditions under which you do or don't get something back from the get
Perfect
graphql/e2e/auth/auth_test.go, line 125 at r3 (raw file):
h := make(http.Header) h.Add("auth-header", ss)
can we make this X-Test-Auth or similar ... just looks more like an auth header that way. I miss read it the first time I saw this.
graphql/e2e/auth/auth_test.go, line 149 at r3 (raw file):
t.Run(tcase.role+tcase.user, func(t *testing.T) { getUserParams := &common.GraphQLParams{ Headers: getJWT(t, tcase.user, tcase.role),
The previous version added an Authorization field in GraphQLParams and added some code the put that into the headers.
I like it much better the way you have it now. But please delete the now dead code.
graphql/e2e/auth/auth_test.go, line 210 at r3 (raw file):
require.Nil(t, gqlResponse.Errors) err := json.Unmarshal([]byte(gqlResponse.Data), &result)
is it simpler to require.JSONeq the expect and string versio, like
require.JSONEq(t, tcase.result, string(gqlResponse.Data))
graphql/e2e/auth/auth_test.go, line 239 at r3 (raw file):
query := ` query { queryColumn(order: {asc: name}) {
I'd like to see some cases where there's a filter in the query. Maybe have the query as part of the test case and you can have some cases where there's a filter eq "Column1"
Maybe also with a first as well - particularly give that these are the kinds of cases that you had to make the fix in dgraph dir for
graphql/e2e/auth/auth_test.go, line 259 at r3 (raw file):
require.Nil(t, gqlResponse.Errors) err := json.Unmarshal([]byte(gqlResponse.Data), &result)
same as above, is there a reason not to jsoneq ?
graphql/e2e/auth/auth_test.go, line 278 at r3 (raw file):
} func TestDeepFilter(t *testing.T) {
I think there should also be tests with deeper queries and also with filters down in the deeper parts of the query.
graphql/e2e/auth/auth_test.go, line 349 at r3 (raw file):
} func TestAndFilter(t *testing.T) {
should there also be an 'or' and a 'not' test?
...oh I think I see in the schema that this isn't just and, it's a much more complex beast. Then change the name to make it more representative.
Do we have a query rewriting test with a complex rule like this? If not can you add this to the auth_query_test.yaml tests, so we can have it recorded
graphql/e2e/auth/schema.graphql, line 212 at r3 (raw file):
# delete: { rule: "false" } ) { colID: ID!
this is just to make the 'get' test easier?
There was a problem hiding this comment.
Reviewable status: 1 of 8 files reviewed, 19 unresolved discussions (waiting on @harshil-goel, @manishrjain, and @MichaelJCompton)
graphql/dgraph/graphquery.go, line 66 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
Is the root argument needed at all now? Seems like this test means it could be removed.
Also, you've added this because there's now cases that mean what was there before wasn't good enough ... so I think you need to add a rewriting test to show that it writes the correct result. There's a test TestAuthMutationQueryRewriting add a case there with order and page to show that the rewriting is as expected and also in auth_query_test.yaml
Done.
graphql/e2e/auth/auth_test.go, line 135 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
looks like a change unrelated to this PR
Done.
graphql/e2e/auth/auth_test.go, line 291 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I don't like this. I'd rather have one case for query and one for get. The query case can look like this fn and the get case should have a similar list of cases that show what different users can see.
Done.
graphql/e2e/auth/auth_test.go, line 297 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
but how does it run? If this is unrelated to this PR, then this function should remain skipped and have no changes
Done.
graphql/e2e/auth/auth_test.go, line 347 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
same here, I'm confused as to how this runs or why this was changed if it's unrelated to this PR.
Done.
graphql/e2e/auth/auth_test.go, line 125 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
can we make this X-Test-Auth or similar ... just looks more like an auth header that way. I miss read it the first time I saw this.
Done.
graphql/e2e/auth/auth_test.go, line 149 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
The previous version added an Authorization field in GraphQLParams and added some code the put that into the headers.
I like it much better the way you have it now. But please delete the now dead code.
Done.
graphql/e2e/auth/auth_test.go, line 210 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
is it simpler to require.JSONeq the expect and string versio, like
require.JSONEq(t, tcase.result, string(gqlResponse.Data))
Done.
graphql/e2e/auth/auth_test.go, line 239 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I'd like to see some cases where there's a filter in the query. Maybe have the query as part of the test case and you can have some cases where there's a filter eq "Column1"
Maybe also with a first as well - particularly give that these are the kinds of cases that you had to make the fix in dgraph dir for
Done.
graphql/e2e/auth/auth_test.go, line 259 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
same as above, is there a reason not to jsoneq ?
Done.
graphql/e2e/auth/auth_test.go, line 278 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I think there should also be tests with deeper queries and also with filters down in the deeper parts of the query.
Done.
graphql/e2e/auth/auth_test.go, line 349 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
should there also be an 'or' and a 'not' test?
...oh I think I see in the schema that this isn't just and, it's a much more complex beast. Then change the name to make it more representative.
Do we have a query rewriting test with a complex rule like this? If not can you add this to the auth_query_test.yaml tests, so we can have it recorded
The test is already present.
graphql/e2e/auth/schema.graphql, line 212 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
this is just to make the 'get' test easier?
Done.
There was a problem hiding this comment.
Looks good. Please remove the changes that crept into query_rewriter.go before merging.
Reviewable status: 1 of 8 files reviewed, 22 unresolved discussions (waiting on @harshil-goel, @manishrjain, and @MichaelJCompton)
graphql/dgraph/graphquery.go, line 66 at r3 (raw file):
Previously, harshil-goel (Harshil Goel) wrote…
Done.
Yeah, this looks like a good simplification.
graphql/e2e/auth/auth_test.go, line 221 at r4 (raw file):
if id == "" { // set invalid id id = "0x1"
This is still a little convoluted. I can see what's going on much better, but this is a bit strange.
I would have done more like had a setup that used user1 to query the ID of column1 and user2 to query the id of column2 and then had the actual tests just run the gets for those ids. That would show that the get works correct when you can and can't see the id - as it is at the moment it doesn't show that get works correctly when you can't see the id, because it never actually tests that.
Let's get this PR merged and then you can put up a quick PR that fixes this test.
graphql/e2e/auth/auth_test.go, line 264 at r4 (raw file):
} } }
nice
graphql/resolve/auth_query_test.yaml, line 66 at r4 (raw file):
dgquery: |- query { queryUserSecret(func: uid(UserSecret1), orderasc: UserSecret.aSecret, first: 1) @filter(uid(UserSecret2)) {
nice, thanks for adding this one.
graphql/resolve/query_rewriter.go, line 428 at r4 (raw file):
q, f := authRw.rewriteRuleNode(typ, orRn) qrys = append(qrys, q...) if f != nil {
These changes weren't here in earlier reviews. What are they for?
If they are related to RBAC rules, please remove.
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 22 unresolved discussions (waiting on @harshil-goel, @manishrjain, and @MichaelJCompton)
graphql/e2e/auth/auth_test.go, line 221 at r4 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
This is still a little convoluted. I can see what's going on much better, but this is a bit strange.
I would have done more like had a setup that used user1 to query the ID of column1 and user2 to query the id of column2 and then had the actual tests just run the gets for those ids. That would show that the get works correct when you can and can't see the id - as it is at the moment it doesn't show that get works correctly when you can't see the id, because it never actually tests that.
Let's get this PR merged and then you can put up a quick PR that fixes this test.
Done.
graphql/resolve/query_rewriter.go, line 428 at r4 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
These changes weren't here in earlier reviews. What are they for?
If they are related to RBAC rules, please remove.
Done.
* add Auth directive (#5178) * parse auth rules (#5180) * added query rewriting and e2e tests (#5229) * parse and evaluate RBAC rules. (#5210) * added test cases for auth schema parsing. (#5195) * process auth query rules (#5181) * send auth variable in custom jwt token. (#5220) * auth on get and mutation results (#5259) * delete authorization (#5270) * parse auth meta info from schema. (#5269) * auth on add update mutations (#5300) * query e2e tests for authentication (#5312) * more testing around additional deletes and auth (#5357) * add RSA algo for JWT token verification. (#5358)
* add Auth directive (hypermodeinc#5178) * parse auth rules (hypermodeinc#5180) * added query rewriting and e2e tests (hypermodeinc#5229) * parse and evaluate RBAC rules. (hypermodeinc#5210) * added test cases for auth schema parsing. (hypermodeinc#5195) * process auth query rules (hypermodeinc#5181) * send auth variable in custom jwt token. (hypermodeinc#5220) * auth on get and mutation results (hypermodeinc#5259) * delete authorization (hypermodeinc#5270) * parse auth meta info from schema. (hypermodeinc#5269) * auth on add update mutations (hypermodeinc#5300) * query e2e tests for authentication (hypermodeinc#5312) * more testing around additional deletes and auth (hypermodeinc#5357) * add RSA algo for JWT token verification. (hypermodeinc#5358)
This change is
Docs Preview: