Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

663/mk/open payments validate #679

Merged
merged 40 commits into from
Oct 25, 2022
Merged

663/mk/open payments validate #679

merged 40 commits into from
Oct 25, 2022

Conversation

mkurapov
Copy link
Contributor

@mkurapov mkurapov commented Oct 18, 2022

Changes proposed in this pull request

  • Adding Open API validation for response messages when making Open Payments API calls (using the sibling open-api package)
  • Refactoring folder structure

Context

The related issues are #663, and #662 and the parent roadmap item is interledger/roadmap#20.

Checklist

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

@github-actions github-actions bot added type: source Changes business logic type: tests Testing related pkg: open-payments labels Oct 18, 2022
@@ -0,0 +1,37 @@
/* eslint-disable @typescript-eslint/no-empty-function */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure how important these tests in particular are, but I thought it was worth making sure we are adding the correct path for the response validator

import { components } from './generated/types'
import { components, paths as Paths } from './generated/types'

export const getPath = <P extends keyof Paths>(path: P): string =>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function gives autocomplete functionality to all of the possible (generated) Open payments API paths we have

args?: CreateOpenPaymentClientArgs
): Promise<OpenPaymentsClient> => {
const axios = createAxiosInstance(args)
const openApi = await createOpenAPI(config.OPEN_PAYMENTS_OPEN_API_URL)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using the same version to validate the Open API as was used for generating the types in the first place

@github-actions github-actions bot added pkg: auth Changes in the GNAP auth package. pkg: backend Changes in the backend package. type: ci Changes to the CI type: documentation labels Oct 21, 2022
@github-actions github-actions bot added the type: ci Changes to the CI label Oct 24, 2022
@mkurapov mkurapov marked this pull request as ready for review October 24, 2022 12:17
Base automatically changed from 663/mk/op-client-1 to main October 24, 2022 13:40
axiosInstance,
{
url: `${baseUrl}/incoming-payment`,
accessToken: 'accessToken'

Check failure

Code scanning / CodeQL

Hard-coded credentials

The hard-coded value "accessToken" is used as [authorization header](1).
return get(
axios,
args,
openApi.createResponseValidator({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response validator library uses Ajv, which recommends instantiating once at initialization (vs once per request here)
https://github.com/kogosoftwarellc/open-api/blob/9b58193f17f69a3f6b018a17c79a8c938b7628a2/packages/openapi-response-validator/index.ts#L79-L85
https://ajv.js.org/guide/managing-schemas.html#compiling-during-initialization
(We should probably document this in our openapi library...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, updated to create the validators just once. Will also update the docs.

packages/openapi/README.md Outdated Show resolved Hide resolved
packages/openapi/README.md Outdated Show resolved Hide resolved
packages/openapi/README.md Outdated Show resolved Hide resolved
Comment on lines 31 to 44
console.log(errorMessage, {
url,
data: JSON.stringify(data)
})

throw new Error(errorMessage)
}

return data
} catch (error) {
console.log('Error when making Open Payments GET request', {
errorMessage: error?.message,
url
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this either not log or should open-payments support a configurable logger like pino?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, added pino support. wanted to do it as a follow up but it's not too big

Copy link
Contributor

@wilsonianb wilsonianb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need to merge/rebase on main to resolve backend build failure

packages/open-payments/src/client/index.ts Outdated Show resolved Hide resolved
packages/open-payments/src/client/requests.ts Outdated Show resolved Hide resolved
@mkurapov mkurapov merged commit 9fc3182 into main Oct 25, 2022
@mkurapov mkurapov deleted the 663/mk/open-payments-validate branch October 25, 2022 23:25
omertoast pushed a commit that referenced this pull request Oct 28, 2022
* feat(open-payments-client): initial POC of open-payments-client

* feat(open-payments-client): cleanup

* feat(open-payments): remove open-payments-client folder

* feat(open-payments): make open-payments folder and use script for type generation

* feat(open-payments): fix rootDir

* feat(open-payments): downgrading generation lib, updating how script is ran

* feat(open-payments): update default config usage

* feat(open-payments): update pnpm-lock.yaml

* feat(open-payments): update pnpm-lock.yaml

* feat(open-payments): adding tests

* feat(open-payments): update pnpm-lock.yaml

* feat(open-payments): simplify test

* feat(open-payments): updating workflows

* feat(open-payments): pin the open api spec to the most recent commit

* feat(open-payments): adding openapi validation

* feat(open-payments): adding openapi validation

* feat(open-payments): adding tests

* feat(open-payments): correcting test

* feat(open-payments): use updated RS spec

* feat(open-payments): build open api package on build step

* feat(open-payments): building open-api package during workflow

* Revert "feat(open-payments): building open-api package during workflow"

This reverts commit e11ec65.

* feat(open-payments): building open-api package during workflow

* feat(open-payments): update naming

* feat(open-payments): instantiate validator functions once

* chore(openapi): add docs for usage

* chore(openapi): update docs

* chore(openapi): prettify docs

* chore(openapi): update docs

* feat(open-payments): adding logger as dependency

* feat(open-payments): updating logging & error logic

* feat(open-payments): remove unnecessary imports

* feat(open-payments): update error handling & test
@huijing huijing added the type: documentation (archived) Improvements or additions to documentation label Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: ci Changes to the CI type: documentation (archived) Improvements or additions to documentation type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants