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

feat(rest)!: update to AFJ 0.2.0 #148

Merged
merged 99 commits into from
Sep 2, 2022

Conversation

janrtvld
Copy link
Contributor

@janrtvld janrtvld commented Aug 23, 2022

I think the switch to tsoa has improved the OpenAPI spec quite a lot. However, there are a few drawbacks (mainly related to the issues lukeautry/tsoa#1268, lukeautry/tsoa#1267 & lukeautry/tsoa#1266).

I think it is fine for now, but if there are better alternatives i'd be happy to change it. It isn't that much work if we don't combine it with a major AFJ release :).

@janrtvld janrtvld force-pushed the feat/controllers branch 3 times, most recently from 5c10bff to ea65472 Compare August 23, 2022 10:01
Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

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

Not any major notes.

I think something that we can improve upon, in a follow-up would PR would be to generalize the error handling of an apicall.

Recordnotfound will always return 404, if we don't know we return 500, etc.

It could be as simlpe as:

handleHttpErrors(() => {
  return {status: 200}
})

const handleHttpErrors = (cb: (...args: any[]) => void | Promise<void>) => {
  try {
    cb()
  } catch(e) {
    if(e instanceof RecordNotFoundError) {
      return {status: 404, message: "Record not found"}
    }
  }
}

packages/rest/samples/sampleWithApp.ts Outdated Show resolved Hide resolved
credential.offerMessage.toJSON({ useLegacyDidSovPrefix: this.agent.config.useLegacyDidSovPrefix })
)}`,
credentialRecord: credential.credentialRecord,
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not an important comment, but it would be nice if we could handle the internalservererror(500, ...) wihtout specifying it like this all the time. Will think about this.

`credential definition with credentialDefinitionId "${credentialDefinitionId}" not found.`
)
if (error instanceof IndySdkError && error.message === 'IndyError(LedgerNotFound): LedgerNotFound') {
return notFoundError(404, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we explicitly pass 404 to NotFound? Shouldn't that be handled by the function as 404 == not-found?

@@ -0,0 +1,186 @@
import type {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should look into generating this... even if it will be

{
  "did": "string",
  "isDid": "boolean"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? Generating examples instead of defining them ourselves?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, and even the generating it like this would be fine. However, we shouldn't over engineer it :).

packages/rest/src/controllers/proofs/ProofController.ts Outdated Show resolved Hide resolved
@janrtvld janrtvld changed the title feat(rest)!:update to AFJ 0.2.0 feat(rest)!: update to AFJ 0.2.0 Aug 24, 2022
@janrtvld janrtvld force-pushed the feat/controllers branch 3 times, most recently from 77bad7b to 3b8b3af Compare August 24, 2022 09:23
@janrtvld janrtvld marked this pull request as ready for review August 26, 2022 09:25
@janrtvld janrtvld requested a review from a team as a code owner August 26, 2022 09:25
@janrtvld
Copy link
Contributor Author

I looked into a cleaner way to handle the errors with a handleError function, but when you don't use @Res() notFoundError: TsoaResponse<404, { reason: string }> tsoa doesn't seem to infer the types for the OpenAPI spec.

github-actions bot and others added 17 commits August 26, 2022 13:46
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
…undation#100)

Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
…#103)

Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
…#106)

Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
…foundation#112)

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
…llet-foundation#109)

Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
…n#110)

Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: Berend Sliedrecht <berend@animo.id>

BREAKING CHANGE: PushNotificationsModule does not exist anymore. You now have to use one that is specific to your service, e.g. PushNotificationsApnsModule or PushNotificationsFcmModule

Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
…#117)

Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
…ation#113)

Signed-off-by: Łukasz Przytuła <lprzytula@gmail.com>

BREAKING CHANGE: `useConnectionByState` now needs a `DidExchangeState` state value instead of a `ConnectionState` state value.

Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
@TimoGlastra
Copy link
Contributor

TimoGlastra commented Sep 1, 2022

Some feedback from my side:

  • In the credentials API I think it would be nice to keep the endpoints for a specific record with the credentialRecordId in it. (like we have with proofs still). This is best practice when building REST APIs.
  • there are types in the swagger ui that I'm not sure will work with the current API. Maybe we can exclude those from the API for now (e.g. linkedAttachments don't think will work because of the difference in interface <-> json keys)

Also, @janrtvld Maybe we can merge this PR into a separate branch and start making incremental PRs?

Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
@janrtvld
Copy link
Contributor Author

janrtvld commented Sep 2, 2022

I have changed the credentials Api to use the credentialRecord Id in the endpoint. I have also excluded the Routing and Attachment properties from the API.

if there isn't anything else i think we can merge this. If we want to add more i'm fine working on a separate branch.

Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
@TimoGlastra
Copy link
Contributor

Great! I think there were also messages that you can pass to the oob apis. Have you tested whether that works?

@TimoGlastra
Copy link
Contributor

TimoGlastra commented Sep 2, 2022

It seems the create-offer method is missing in the API. But we can add that in a separate PR

Have opened issue for create-invitation: #156

Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
@TimoGlastra
Copy link
Contributor

Some other parameters to remove:

  • appendedAttachments from oob/create-invitation
  • routing + appendedAttachments from oob/receive-invitation
  • routing from oob/receive-invitation-url

@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2022

Codecov Report

Merging #148 (e70aa01) into main (c0351c5) will decrease coverage by 0.83%.
The diff coverage is 84.16%.

@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
- Coverage   87.07%   86.24%   -0.84%     
==========================================
  Files          45       21      -24     
  Lines         681     1163     +482     
  Branches       92      260     +168     
==========================================
+ Hits          593     1003     +410     
- Misses         76      146      +70     
- Partials       12       14       +2     
Impacted Files Coverage Δ
packages/rest/src/routes/routes.ts 86.58% <ø> (ø)
packages/rest/src/utils/util.ts 100.00% <ø> (ø)
packages/rest/src/utils/webhook.ts 100.00% <ø> (ø)
packages/rest/src/utils/agent.ts 62.96% <50.00%> (ø)
packages/rest/src/server.ts 79.59% <70.58%> (-11.84%) ⬇️
...st/src/controllers/credentials/SchemaController.ts 82.35% <77.77%> (+3.78%) ⬆️
...lers/credentials/CredentialDefinitionController.ts 87.87% <83.33%> (+3.26%) ⬆️
...rc/controllers/connections/ConnectionController.ts 83.92% <84.09%> (+3.92%) ⬆️
...rc/controllers/credentials/CredentialController.ts 87.36% <84.28%> (+5.66%) ⬆️
...ges/rest/src/controllers/proofs/ProofController.ts 83.51% <85.96%> (+2.74%) ⬆️
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
Signed-off-by: Jan <60812202+janrtvld@users.noreply.github.com>
@TimoGlastra
Copy link
Contributor

Ready to merge 👀 ???

@TimoGlastra TimoGlastra merged commit 8ec4dc4 into openwallet-foundation:main Sep 2, 2022
@janrtvld janrtvld deleted the feat/controllers branch September 8, 2022 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[rest] Get all connections with filters
6 participants