Added function to login with refresh token etc

[gitlw]

1. Added function to login with refresh token
2. Refactored the test code to reuse the function to GetDgraphClient, which is also being used inside Dgraph tests

---

This change is 

[manishrjain]

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @gitlw, @manishrjain, @codexnull, and @srfrog)

---

client.go, line 56 at r1 (raw file):

		Password: password,
	}
	resp, err := dc.Login(ctx, loginRequest)

Put entire code within lock.

---

client.go, line 66 at r1 (raw file):

}

func (d *Dgraph) LoginWithRefreshJwt(ctx context.Context) error {

Make this a private function. Put the entire code around within jwtMutex.Lock().

Also, hook into the right places, so you can automatically call this func, when you encounter an unauthorized access or something.

---

test/test_utils.go, line 29 at r1 (raw file):

type CloseFunc func()

func GetDgraphClient() (*dgo.Dgraph, CloseFunc) {

No need to have a file just for one func. Can be located in any of the test files in this package.

---

test/examples_test.go, line 31 at r1 (raw file):

func ExampleDgraph_Alter_dropAll() {
	dg, close := GetDgraphClient()

Don't overwrite close function.

---

test/examples_test.go, line 165 at r1 (raw file):

	ctx := context.Background()
	err := dg.Alter(ctx, op)

if err := dg.Alter(...); err != nil { ... }

[gitlw]

retest this please

[manishrjain]

Reviewable status: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @gitlw, @codexnull, and @srfrog)

---

client.go, line 146 at r2 (raw file):

	st, ok := status.FromError(err)
	if ok && st.Code() == codes.Unauthenticated &&
		strings.Contains(err.Error(), "jwt token has expired") {

This piece of code can be refactored out into a separate function to avoid duplication.

func checkLogin(err error) (context.Context, error)

Also, we need to make sure that the client remains easy to implement, so we can port the logic over to other languages as well.

---

txn.go, line 64 at r2 (raw file):

func (d *Dgraph) NewTxn() *Txn {
	return &Txn{
		dg:      d,

We choose a client upfront so that all the queries and mutations go to that one server. If we switch the client in between our calls, it could cause issues.

[gitlw]

Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @manishrjain, @codexnull, and @srfrog)

---

client.go, line 56 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Put entire code within lock.

Done.

---

client.go, line 66 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Make this a private function. Put the entire code around within jwtMutex.Lock().

Also, hook into the right places, so you can automatically call this func, when you encounter an unauthorized access or something.

Done.

---

client.go, line 146 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This piece of code can be refactored out into a separate function to avoid duplication.

func checkLogin(err error) (context.Context, error)

Also, we need to make sure that the client remains easy to implement, so we can port the logic over to other languages as well.

Done.

---

txn.go, line 64 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

We choose a client upfront so that all the queries and mutations go to that one server. If we switch the client in between our calls, it could cause issues.

Done.

---

test/test_utils.go, line 29 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

No need to have a file just for one func. Can be located in any of the test files in this package.

Done.

---

test/examples_test.go, line 31 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Don't overwrite close function.

Done.

---

test/examples_test.go, line 165 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if err := dg.Alter(...); err != nil { ... }

Done.

[gitlw]

Reviewed 3 of 4 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @manishrjain, @codexnull, and @srfrog)

[manishrjain]

Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @codexnull, @gitlw, @manishrjain, and @srfrog)

---

client.go, line 128 at r4 (raw file):

// checkJwtExpiration returns whether the operation should be retried
// and if so the updated context with refreshed access JWT
func (d *Dgraph) checkJwtExpiration(ctx context.Context, err error) (bool, context.Context) {

Not a fan of a func which takes in an error. Instead, you could call this func, if the error is nil.

func retryLogin(ctx context.Context) (context.Context, error)

---

client.go, line 137 at r4 (raw file):

		strings.Contains(err.Error(), "Token is expired") {
		// try to login with the refreshJwt
		if e := d.loginWithRefreshJwt(ctx); e != nil {

if e is an err, call it so. And then return that error back. Do not convert error to a bool.

Also, prefer returning errors to bool.

---

txn.go, line 93 at r4 (raw file):

		ReadOnly: txn.readOnly,
	}
	ctxWithJwt := txn.dg.getContext(ctx)

You can just call this ctx. No need to assign it a different var.

---

txn.go, line 95 at r4 (raw file):

	ctxWithJwt := txn.dg.getContext(ctx)
	resp, err := txn.dc.Query(ctxWithJwt, req)
	retry, newCtx := txn.dg.checkJwtExpiration(ctx, err)

if isAuthError(err) {
ctx, err = txn.dg.retryLogin(ctx)
}

Also, is there a reason for ctx being used here and not ctxWithJwt?

[manishrjain]

I think still needs work around the flow of the code.

Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @codexnull, @gitlw, and @srfrog)

[gitlw]

Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @codexnull, @gitlw, @manishrjain, and @srfrog)

---

client.go, line 128 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Not a fan of a func which takes in an error. Instead, you could call this func, if the error is nil.

func retryLogin(ctx context.Context) (context.Context, error)

Done.

---

client.go, line 137 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if e is an err, call it so. And then return that error back. Do not convert error to a bool.

Also, prefer returning errors to bool.

Done.

---

txn.go, line 93 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

You can just call this ctx. No need to assign it a different var.

Done.

---

txn.go, line 95 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if isAuthError(err) {
ctx, err = txn.dg.retryLogin(ctx)
}

Also, is there a reason for ctx being used here and not ctxWithJwt?

Done.

[manishrjain]

Reviewed 1 of 4 files at r3, 2 of 2 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @codexnull, @gitlw, and @srfrog)

---

examples_test.go, line 33 at r5 (raw file):

type CancelFunc func()

func GetDgraphClient() (*dgo.Dgraph, CancelFunc) {

make it private getDgraphClient