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

chore(backend): match subresources by payment pointer #647

Merged
merged 7 commits into from
Oct 25, 2022

Conversation

wilsonianb
Copy link
Contributor

@wilsonianb wilsonianb commented Sep 29, 2022

Changes proposed in this pull request

  • match subresources by payment pointer on GET requests
  • store grantId on quote
  • distinguish read/read-all (and list/list-all) in auth middleware instead of routes
  • add abstract PaymentPointerSubresource model.
  • add shared subresource tests

Context

Checklist

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

Add abstract PaymentPointerSubresource model.
Add SubresourceQueryBuilder custom query builder.
Add shared subresource tests.
Replace grant.includesAccess with findAccess.
Store filtering clientId on request context.
Add shared subresource GET route tests
@github-actions github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related labels Sep 29, 2022
@wilsonianb wilsonianb marked this pull request as ready for review September 29, 2022 22:19
@wilsonianb
Copy link
Contributor Author

What if we moved the following into the packages/backend/src/open_payments/payment_pointer directory
-packages/backend/src/open_payments/payment/incoming
-packages/backend/src/open_payments/payment/incoming
-packages/backend/src/open_payments/quote
to enforce the idea of them as payment pointer sub-resources?

(What if we then moved everything out of the packages/backend/src/open_payments directory?)

@matdehaast matdehaast self-requested a review October 3, 2022 16:28
@@ -84,6 +84,7 @@ export type ClientKeysContext = Context<AppRequest<'keyId'>>
export interface PaymentPointerContext extends AppContext {
paymentPointer: PaymentPointer
grant?: Grant
clientId?: string
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that also a property of grant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but ctx.clientId is now being defined in the auth middleware if the request has limited access to client-specific resources.
(instead of checking the grant in every relevant route)
Maybe it'd be clearer if we replace grant here with grantId, but also add a limits?: AccessLimits (to be used in outgoing payment creation)?

Copy link
Member

@sabineschaller sabineschaller Oct 19, 2022

Choose a reason for hiding this comment

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

Maybe it'd be clearer if we replace grant here with grantId, but also add a limits?: AccessLimits (to be used in outgoing payment creation)?

Let's do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started making this change but it seems like it'll be a bit extensive, so I'd prefer addressing it as a separate issue:

Comment on lines +130 to +138
if (paymentPointerId) {
this.where(
`${this.modelClass().tableName}.paymentPointerId`,
paymentPointerId
)
}
if (clientId) {
this.byClientId(clientId)
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this doing two queries if paymentPointerId and clientId are given?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is all a single query builder that isn't executed until we await
https://vincit.github.io/objection.js/api/query-builder/#class-querybuilder

Copy link
Member

@sabineschaller sabineschaller left a comment

Choose a reason for hiding this comment

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

Let's just get it in and fix potential issues later.

@wilsonianb wilsonianb merged commit 02920fd into main Oct 25, 2022
@wilsonianb wilsonianb deleted the bw-quote-grant branch October 25, 2022 14:47
@wilsonianb
Copy link
Contributor Author

Apparently I should have merged main and re-run checks (despite no conflicts), because builds on main are now failing.
This branch removed packages/backend/src/shared/routes.test.ts from which an import has since been added

I'll resolve this as a part of:

omertoast pushed a commit that referenced this pull request Oct 28, 2022
* chore(backend): store grantId on quote

Add abstract PaymentPointerSubresource model.

* chore(backend): match subresources by payment pointer

Add SubresourceQueryBuilder custom query builder.
Add shared subresource tests.

* chore(backend): distinguish read/read-all in auth middleware

Replace grant.includesAccess with findAccess.
Store filtering clientId on request context.
Add shared subresource GET route tests

* chore(backend): add list query to SubresourceQueryBuilder

Add PaymentPointerSubresourceService interface.

* chore(backend): add list to shared subresource tests

* chore(backend): run pagination tests via shared subresource tests

Create fewer instances in shared pagination tests.

* chore(backend): abstract list routes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
2 participants