refactor(backend): add `getOrCreate` method for `grantReferenceService` #787

raducristianpopa · 2022-11-24T14:58:05Z

Changes proposed in this pull request

Add getOrCreate method for grantReferenceService

Context

fixes Add grantReferenceService.getOrCreate #703

Checklist

Related issues linked using fixes #number
Tests added/updated
Documentation added
Make sure that all checks pass

method getOrCreate was added for `grantReferenceService`

mkurapov

Looks solid, just a few small comments. Would test this by

mkurapov · 2022-11-24T16:41:43Z

packages/backend/src/open_payments/auth/middleware.test.ts

+    await grantReferenceService.create({
+      id: grant.grant,
+      clientId: uuid()
+    })


I don't think you need this create

mkurapov · 2022-11-24T16:42:24Z

packages/backend/src/open_payments/grantReference/service.test.ts

@@ -79,4 +80,60 @@ describe('Grant Reference Service', (): void => {
      )
    })
  })
+
+  describe('Get or Create Grant Reference', (): void => {
+    test('cannot fetch a non-existing grant reference', async (): Promise<void> => {


Suggested change

test('cannot fetch a non-existing grant reference', async (): Promise<void> => {

test('returns undefined for a non-existing grant reference', async (): Promise<void> => {

mkurapov · 2022-11-24T16:53:29Z

packages/backend/src/open_payments/grantReference/service.test.ts

+      expect(retrievedRef).toEqual(existingRef)
+    })
+
+    test('create a grant reference', async (): Promise<void> => {


Suggested change

test('create a grant reference', async (): Promise<void> => {

test('creates a grant reference', async (): Promise<void> => {

nitpick, generally naming the test as though it's doing the action: the test creates, returns, throws etc

mkurapov · 2022-11-24T16:54:45Z

packages/backend/src/open_payments/auth/middleware.test.ts

@@ -263,6 +263,42 @@ describe('Auth Middleware', (): void => {
    scope.done()
  })

+  test('if getOrCreateGrantReference throws, return 500', async (): Promise<void> => {


Suggested change

test('if getOrCreateGrantReference throws, return 500', async (): Promise<void> => {

test('returns 500 if getOrCreateGrantReference throws', async (): Promise<void> => {

the same as comments below

mkurapov · 2022-11-24T17:09:37Z

packages/backend/src/open_payments/auth/middleware.ts

+      } catch (e) {
+        const errInfo = e && typeof e === 'object' && e.stack ? e.stack : e
+        logger.debug(errInfo)


Suggested change

} catch (e) {

const errInfo = e && typeof e === 'object' && e.stack ? e.stack : e

logger.debug(errInfo)

} catch (error) {

const errorMessage = error?.message ?? 'Failed to get or create grant reference'

logger.error({ error }, errorMessage)

would prefer something like this, without needing to check the type etc

mkurapov · 2022-11-24T17:10:30Z

packages/backend/src/open_payments/grantReference/service.test.ts

+            { id, clientId: uuid() },
+            AccessAction.List
+          )
+      ).rejects.toThrowError('does not match internal reference clientId')


I thought we would have needed a RegEx here, but it seems to work?

Suggested change

).rejects.toThrowError('does not match internal reference clientId')

).rejects.toThrowError(/does not match internal reference clientId/)

I was thinking the same, but based on Jest docs, RegEx and strings are equivalent in this case.

mkurapov

Looks great!
In order to properly test this we need to:

Create or seed an initial grant in the auth server
Make a request (e.g. get incoming payment) with the token related to the created grant, and verify that the grant reference was created in backend's database during token introspection
Make another request (e.g. duplicate get incoming payment) with the token related to the created grant, and this time verify the grant reference was fetched correctly from backend's database.

Easiest way would be to test this with the Create Incoming Payment request in the Postman P2P payment transfer example collection, after my PR is merged in. Once that is merged, I can walk you through the flow

wilsonianb · 2022-11-28T13:48:24Z

packages/backend/src/open_payments/grantReference/service.ts

+      }
+
+      return grantRef
+    } else if (action === AccessAction.Create) {


I think this check should be moved out to the caller.

wilsonianb · 2022-11-28T14:00:52Z

packages/backend/src/open_payments/auth/middleware.ts

+        await grantReferenceService.getOrCreate(
+          { id: grant.grant, clientId: grant.clientId },
+          action
+        )


Suggested change

await grantReferenceService.getOrCreate(

{ id: grant.grant, clientId: grant.clientId },

action

)

if (action === AccessAction.Create) {

await grantReferenceService.getOrCreate({

id: grant.grant,

clientId: grant.clientId

})

} else {

await grantReferenceService.get(grant.grant)

}

By moving the action check to the caller, the clientId check

rafiki/packages/backend/src/open_payments/grantReference/service.ts

Lines 58 to 63 in c6eaaa9

if (grantRef) {

if (grantRef.clientId !== options.clientId) {

throw new Error(

`clientID ${options.clientId} for grant ${options.id} does not match internal reference clientId ${grantRef.clientId}.`

)

}

will probably need to be moved to the getGrantRerefence method

rafiki/packages/backend/src/open_payments/grantReference/service.ts

Lines 27 to 29 in c6eaaa9

async function getGrantReference(grantId: string, trx: Transaction) {

return await GrantReference.query(trx).findById(grantId)

}

and will have to take both arguments (id and clientId)

async function getGrantReference( options: CreateGrantReferenceOptions, trx: Transaction ) { // ... }

otherwise there will be no validation for the clientId when calling grantReferenceService.get

I'm leaning towards not passing clientId to grantReferenceService.get (even though this is currently the only place calling it).
What if we moved the clientId check out here?

Suggested change

await grantReferenceService.getOrCreate(

{ id: grant.grant, clientId: grant.clientId },

action

)

if (action === AccessAction.Create) {

await grantReferenceService.getOrCreate({

id: grant.grant,

clientId: grant.clientId

})

} else {

const grantRef = await grantReferenceService.get(grant.grant)

if (grantRef && grantRef.clientId !== grant.clientId) {

throw new Error(

`clientID ${grant.clientId} for grant ${grant.id} does not match internal reference clientId ${grantRef.clientId}.`

}

}

}

🤔 that looks a lot like how it was before...

Is there an argument for not actually using getOrCreate?
It looks like the AccessAction.Create check was added after this issue was opened:

fix: only store grant and client IDs on create routes #676

wilsonianb · 2022-11-30T00:47:38Z

This will completely remove grantReferenceService (if we decide we want to):

chore(backend): replace GrantReference with OutgoingPaymentGrant #800

raducristianpopa · 2022-12-05T05:48:49Z

Closing this PR since #800 got merged in.

raducristianpopa added 8 commits November 23, 2022 11:58

refactor(backend): add getOrCreate method

e6bfa1a

method getOrCreate was added for `grantReferenceService`

refactor(backend): return grantRef or undefined

ba6afb5

Merge branch 'main' into radup--703

577595d

refactor(backend): no empty return for getOrCreate

39054c9

chore(backend): add tests for getOrCreate method

6129c0d

chore(backend): fix lint warnings

04bd508

chore(backend): add middleware test

a5a56c6

chore(backend): fix middleware test

28860c8

github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related labels Nov 24, 2022

mkurapov reviewed Nov 24, 2022

View reviewed changes

raducristianpopa added 3 commits November 25, 2022 09:20

chore(backend): fix test names

9f63dd9

chore(backend): remove grantRef creation from test

84a2e18

fix(backend): fix error catching

6378e3f

raducristianpopa requested a review from mkurapov November 25, 2022 08:22

mkurapov approved these changes Nov 25, 2022

View reviewed changes

wilsonianb reviewed Nov 28, 2022

View reviewed changes

Merge branch 'main' into radup--703

c6eaaa9

raducristianpopa closed this Dec 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(backend): add `getOrCreate` method for `grantReferenceService` #787

refactor(backend): add `getOrCreate` method for `grantReferenceService` #787

raducristianpopa commented Nov 24, 2022 •

edited

Loading

mkurapov left a comment

mkurapov Nov 24, 2022

mkurapov Nov 24, 2022

mkurapov Nov 24, 2022

mkurapov Nov 24, 2022

mkurapov Nov 24, 2022

mkurapov Nov 24, 2022

raducristianpopa Nov 25, 2022

mkurapov left a comment

wilsonianb Nov 28, 2022

wilsonianb Nov 28, 2022

raducristianpopa Nov 29, 2022

wilsonianb Nov 29, 2022

wilsonianb commented Nov 30, 2022

raducristianpopa commented Dec 5, 2022

	test('cannot fetch a non-existing grant reference', async (): Promise<void> => {
	test('returns undefined for a non-existing grant reference', async (): Promise<void> => {

	test('create a grant reference', async (): Promise<void> => {
	test('creates a grant reference', async (): Promise<void> => {

	test('if getOrCreateGrantReference throws, return 500', async (): Promise<void> => {
	test('returns 500 if getOrCreateGrantReference throws', async (): Promise<void> => {

	).rejects.toThrowError('does not match internal reference clientId')
	).rejects.toThrowError(/does not match internal reference clientId/)

	if (grantRef) {
	if (grantRef.clientId !== options.clientId) {
	throw new Error(
	`clientID ${options.clientId} for grant ${options.id} does not match internal reference clientId ${grantRef.clientId}.`
	)
	}

	async function getGrantReference(grantId: string, trx: Transaction) {
	return await GrantReference.query(trx).findById(grantId)
	}

-        await grantReferenceService.getOrCreate(
-          { id: grant.grant, clientId: grant.clientId },
-          action
-        )
+        if (action === AccessAction.Create) {
+          await grantReferenceService.getOrCreate({
+            id: grant.grant,
+            clientId: grant.clientId
+          })
+        } else {
+          const grantRef = await grantReferenceService.get(grant.grant)
+          if (grantRef && grantRef.clientId !== grant.clientId) {
+            throw new Error(
+              `clientID ${grant.clientId} for grant ${grant.id} does not match internal reference clientId ${grantRef.clientId}.`
+            }
+          }
+        }

refactor(backend): add getOrCreate method for grantReferenceService #787

refactor(backend): add getOrCreate method for grantReferenceService #787

Conversation

raducristianpopa commented Nov 24, 2022 • edited Loading

Changes proposed in this pull request

Context

Checklist

mkurapov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonianb commented Nov 30, 2022

raducristianpopa commented Dec 5, 2022

refactor(backend): add `getOrCreate` method for `grantReferenceService` #787

refactor(backend): add `getOrCreate` method for `grantReferenceService` #787

raducristianpopa commented Nov 24, 2022 •

edited

Loading