Fix APIs response status and workflow; adding Dev container #17
Conversation
| // TODO: Make common responce preparation and creation | ||
| response.json({ token: token.encode() }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Did you rename or move this file so those changes are shown as deleted/added even if you didn't changed anything beside the type check? can you revert this change and include only the actual changes?
| @@ -108,6 +108,15 @@ paths: | |||
| application/json: | |||
| schema: | |||
| $ref: '#/components/schemas/RequestDescription' | |||
| "404": | |||
There was a problem hiding this comment.
I don't think that this is an appropriate case for this status code, since this status code is used in case request-URI is not matched with any of application routes (reference 10.4.5) but we clearly have /credential-issuance/request static route without any '{slugs}' (in case with not found slugs in route path - 404 is an appropriate status code). In general - this is looks like 400 status code and should be handled by already existed and used validation mechanism - oas3 definition to have single source of truth. Please refer to oas3 enums from here.
| @@ -129,6 +138,15 @@ paths: | |||
| application/json: | |||
| schema: | |||
| $ref: '#/components/schemas/RequestDescription' | |||
| "404": | |||
There was a problem hiding this comment.
Please check previous comment.
| const requestDescription = await this.requestDescriptionFactory.create(token) | ||
| response.json(requestDescription.toJSON()) | ||
| } catch (error) { | ||
| if (error instanceof InvalidArgumentException) { |
There was a problem hiding this comment.
This type of exception was created for the general purpose and shouldn't be used in this way I guess, since it can occur in any type of arguments assertion for example and the behavior could be unpredictable/wrong. I guess if you'd like to intercept some error and handle it in some specific way - just create specific exception and catch/handle it in this specific case... but regarding to the comments related to 404 status code above - changes in this file are not needed.
| @@ -25,7 +27,7 @@ export class CredentialController { | |||
| private readonly staticClaimsMetadataProvider: StaticClaimsMetadataProvider, | |||
| private readonly staticCredentialOfferProvider: StaticCredentialOfferProvider, | |||
| private readonly credentialOfferFactory: CredentialOfferFactory | |||
| ) {} | |||
| ) { } | |||
There was a problem hiding this comment.
Please control useless code-style changes like this. I think we should create the tasks to configure static analyzing tools and ci to prevent such type of corrections.
|
|
||
| response.json(requestDescription.toJSON()) | ||
| } catch (error) { | ||
| if (error instanceof InvalidArgumentException) { |
There was a problem hiding this comment.
Please check this comment.
| try { | ||
| await agent.findInteraction(request.body.token) | ||
| } catch (error) { | ||
| if (error instanceof Error && error.message === ErrorCode.NoSuchInteraction) { |
There was a problem hiding this comment.
If you want properly handle this error - shouldn't we do something if it's not Error type error? in this case we still get 500 on attempt to execute lines out of try/catch I think. So IMO - cleaner way could be if ((error as Error).message === ErrorCode.NoSuchInteraction) { what you think?
Just related to this question thread.
| # RUN su node -c "source /usr/local/share/nvm/nvm.sh && nvm install ${EXTRA_NODE_VERSION}" | ||
|
|
||
| # [Optional] Uncomment if you want to install more global node packages | ||
| # RUN su node -c "npm install -g <your-package-list -here>" |
There was a problem hiding this comment.
IMO it's not the best solution to put under the VCS something specific to some concrete development tool like IDE and deny users of other ones. The best way is to create common definition on base of some common image version without any sort of additional IDE related plugins etc. just raw image with working app within it and if required - add some sort of configurations to be executed depends on environment or chosen IDE. But for now I guess the easiest way - just create some distinction by creation of folders structure like docker/.dev/vscode/... just to put, if required, docker related things under the docker dir...
| // Comment out connect as root instead. More info: https://aka.ms/vscode-remote/containers/non-root. | ||
| "remoteUser": "node" | ||
|
|
||
| } |
There was a problem hiding this comment.
Please check previous comment and fix code style.
|
Closing this PR and open a new one because it seems there are some issues with my machine regarding the indent and format. |
Description
This change adds Dev Container to make the developer experience easier and it fixes some issues in APIs.
Fixes:
#14 - Adding handler for invalid type in the payload
Enhancment:
#16 - Adding Dev container