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

Fix APIs response status and workflow; adding Dev container #17

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .devcontainer/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# See here for image contents: https://github.com/microsoft/vscode-dev-containers/tree/v0.194.0/containers/typescript-node/.devcontainer/base.Dockerfile

# [Choice] Node.js version: 16, 14, 12
ARG VARIANT="16-buster"
FROM mcr.microsoft.com/vscode/devcontainers/typescript-node:0-${VARIANT}

# [Optional] Uncomment this section to install additional OS packages.
# RUN apt-get update && export DEBIAN_FRONTEND=noninteractive \
# && apt-get -y install --no-install-recommends <your-package-list-here>

# [Optional] Uncomment if you want to install an additional version of node using nvm
# ARG EXTRA_NODE_VERSION=10
# 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>"
Copy link
Contributor

@serg-temchenko serg-temchenko Sep 13, 2021

Choose a reason for hiding this comment

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

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...

31 changes: 31 additions & 0 deletions .devcontainer/devcontainer.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// For format details, see https://aka.ms/devcontainer.json. For config options, see the README at:
// https://github.com/microsoft/vscode-dev-containers/tree/v0.194.0/containers/typescript-node
{
"name": "Node.js & TypeScript",
"build": {
"dockerfile": "Dockerfile",
// Update 'VARIANT' to pick a Node version: 12, 14, 16
"args": {
"VARIANT": "14"
}
},

// Set *default* container specific settings.json values on container create.
"settings": {},


// Add the IDs of extensions you want installed when the container is created.
"extensions": [
"dbaeumer.vscode-eslint"
],

// Use 'forwardPorts' to make a list of ports inside the container available locally.
"forwardPorts": [9000],

// Use 'postCreateCommand' to run commands after the container is created.
// "postCreateCommand": "yarn install",

// Comment out connect as root instead. More info: https://aka.ms/vscode-remote/containers/non-root.
"remoteUser": "node"

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check previous comment and fix code style.

18 changes: 18 additions & 0 deletions src/api/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,15 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/RequestDescription'
"404":
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

description: Returns when interaction for provided type(s) was not found.
content:
application/json:
schema:
type: object
properties:
message:
type: string
x-swagger-router-controller: CredentialController
operationId: requestPost
/credential-issuance/offer:
Expand All @@ -129,6 +138,15 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/RequestDescription'
"404":
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check previous comment.

description: Returns when interaction for provided type(s) was not found.
content:
application/json:
schema:
type: object
properties:
message:
type: string
x-swagger-router-controller: CredentialController
operationId: offerPost
/credential-issuance/offer/custom:
Expand Down
96 changes: 48 additions & 48 deletions src/controller/callbackController.ts
Original file line number Diff line number Diff line change
@@ -1,48 +1,48 @@
import { Request, Response } from 'express'
import { SdkAgentProvider } from '../sdk/sdkAgentProvider'
import { ErrorCode } from '@jolocom/sdk'
import { StatusCodes } from 'http-status-codes'
import { InteractionRequestHandler } from '../interaction/interactionRequestHandler'
import { injectable } from 'inversify'
// @ts-ignore
import { FlowType } from '@jolocom/sdk/js/interactionManager/types'

/**
* The controller to handle all interactions callback requests.
*/
@injectable()
export class CallbackController {
constructor(
private readonly agentProvider: SdkAgentProvider,
private readonly interactionRequestHandler: InteractionRequestHandler,
) {}

/**
* An action method to process interactions callback request for all {@link FlowType} types.
* In response will be received encoded processed interaction message jwt.
*
* @param request The {@link Request} object representation.
* @param response The {@link Response} object representation.
* @return {Promise<void>}
*/
public async callbackPost(request: Request, response: Response) {
const agent = await this.agentProvider.provide()

try {
await agent.findInteraction(request.body.token)
} catch (error) {
if (error.message === ErrorCode.NoSuchInteraction) {
response.status(StatusCodes.NOT_FOUND).json({
message: `Interaction with token '${request.body.token}' not found.`
})
}

throw (error)
}

const token = await this.interactionRequestHandler.handle(request.body.token, agent)

// TODO: Make common responce preparation and creation
response.json({ token: token.encode() })
}
}
import { Request, Response } from 'express'
import { SdkAgentProvider } from '../sdk/sdkAgentProvider'
import { ErrorCode } from '@jolocom/sdk'
import { StatusCodes } from 'http-status-codes'
import { InteractionRequestHandler } from '../interaction/interactionRequestHandler'
import { injectable } from 'inversify'
// @ts-ignore
import { FlowType } from '@jolocom/sdk/js/interactionManager/types'
/**
* The controller to handle all interactions callback requests.
*/
@injectable()
export class CallbackController {
constructor(
private readonly agentProvider: SdkAgentProvider,
private readonly interactionRequestHandler: InteractionRequestHandler,
) {}
/**
* An action method to process interactions callback request for all {@link FlowType} types.
* In response will be received encoded processed interaction message jwt.
*
* @param request The {@link Request} object representation.
* @param response The {@link Response} object representation.
* @return {Promise<void>}
*/
public async callbackPost(request: Request, response: Response) {
const agent = await this.agentProvider.provide()
try {
await agent.findInteraction(request.body.token)
} catch (error) {
if (error instanceof Error && error.message === ErrorCode.NoSuchInteraction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

response.status(StatusCodes.NOT_FOUND).json({
message: `Interaction with token '${request.body.token}' not found.`
})
}
throw (error)
}
const token = await this.interactionRequestHandler.handle(request.body.token, agent)
// TODO: Make common responce preparation and creation
response.json({ token: token.encode() })
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

69 changes: 46 additions & 23 deletions src/controller/credentialController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import { CredentialOfferFactory } from '../credential/offer/credentialOfferFacto
import { ICredentialRequest } from '@jolocom/protocol-ts/dist/lib/interactionTokens'
// @ts-ignore
import { FlowType } from '@jolocom/sdk/js/interactionManager/types'
import { InvalidArgumentException } from '../exception/invalidArgumentException'
import { StatusCodes } from 'http-status-codes'

/**
* The controller to handle all requests related to the
Expand All @@ -25,7 +27,7 @@ export class CredentialController {
private readonly staticClaimsMetadataProvider: StaticClaimsMetadataProvider,
private readonly staticCredentialOfferProvider: StaticCredentialOfferProvider,
private readonly credentialOfferFactory: CredentialOfferFactory
) {}
) { }
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


/**
* An action method to receive the resource containing credential request information.
Expand All @@ -36,19 +38,29 @@ export class CredentialController {
*/
public async requestPost(request: Request, response: Response) {
// TODO: Refactor in favor of strategy pattern usage
const credentialRequirements: ICredentialRequest[] = request.body.types.map((type: string) => ({
type: this.staticClaimsMetadataProvider.getByType(type).type,
// TODO: Define constraints definition place and provide
constraints: [],
}))
const agent = await this.agentProvider.provide()
const token = await agent.credRequestToken({
credentialRequirements,
callbackURL: this.appConfig.sdk.callbackUrl,
})
const requestDescription = await this.requestDescriptionFactory.create(token)
try {
const credentialRequirements: ICredentialRequest[] = request.body.types.map((type: string) => ({
type: this.staticClaimsMetadataProvider.getByType(type).type,
// TODO: Define constraints definition place and provide
constraints: [],
}))

response.json(requestDescription.toJSON())
const agent = await this.agentProvider.provide()
const token = await agent.credRequestToken({
credentialRequirements,
callbackURL: this.appConfig.sdk.callbackUrl,
})

const requestDescription = await this.requestDescriptionFactory.create(token)
response.json(requestDescription.toJSON())
} catch (error) {
if (error instanceof InvalidArgumentException) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

response.status(StatusCodes.NOT_FOUND).json({
message: error.message
})
}
throw error;
}
}

/**
Expand All @@ -60,17 +72,27 @@ export class CredentialController {
*/
public async offerPost(request: Request, response: Response) {
// TODO: Refactor in favor of strategy pattern usage
const offeredCredentials: CredentialOfferRequest[] = request.body.types.map(
(type: string) => this.staticCredentialOfferProvider.getByType(type)
)
const agent = await this.agentProvider.provide()
const token = await agent.credOfferToken({
offeredCredentials,
callbackURL: this.appConfig.sdk.callbackUrl
})
const requestDescription = await this.requestDescriptionFactory.create(token)
try {
const offeredCredentials: CredentialOfferRequest[] = request.body.types.map(
(type: string) => this.staticCredentialOfferProvider.getByType(type)
)
const agent = await this.agentProvider.provide()
const token = await agent.credOfferToken({
offeredCredentials,
callbackURL: this.appConfig.sdk.callbackUrl
})
const requestDescription = await this.requestDescriptionFactory.create(token)

response.json(requestDescription.toJSON())
} catch (error) {
if (error instanceof InvalidArgumentException) {
Copy link
Contributor

@serg-temchenko serg-temchenko Sep 13, 2021

Choose a reason for hiding this comment

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

Please check this comment.

response.status(StatusCodes.NOT_FOUND).json({
message: error.message
})
}
throw error;
}

response.json(requestDescription.toJSON())
}

/**
Expand All @@ -93,5 +115,6 @@ export class CredentialController {
const requestDescription = await this.requestDescriptionFactory.create(token)

response.json(requestDescription.toJSON())

}
}
3 changes: 2 additions & 1 deletion src/credential/offer/staticCredentialOfferProvider.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { injectable, multiInject } from 'inversify'
import { TYPES } from '../../types'
import { CredentialOffer } from '@jolocom/protocol-ts'
import { InvalidArgumentException } from '../../exception/invalidArgumentException'

/**
* This implementation is registry of all predefined (static) {@link CredentialOffer} instances.
Expand All @@ -23,7 +24,7 @@ export class StaticCredentialOfferProvider {

private assertExists(type: string) {
if (!this.credentialOffers.some(credentialOffer => credentialOffer.type === type)) {
throw new Error(`Credential offer with type '${type}' not found.`)
throw new InvalidArgumentException(`Credential offer with type '${type}' not found.`)
}
}
}
3 changes: 2 additions & 1 deletion src/credential/staticClaimsMetadataProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { inject, injectable } from 'inversify'
import { ClaimsMetadataMap } from './claimsMetadataMap'
import { TYPES } from '../types'
import { BaseMetadata } from 'cred-types-jolocom-core/types'
import { InvalidArgumentException } from '../exception/invalidArgumentException'

/**
* This implementation is providing service of {@link BaseMetadata}.
Expand All @@ -24,7 +25,7 @@ export class StaticClaimsMetadataProvider {

private assertExists(type: string) {
if (!(type in this.claimsMetadataMap)) {
throw new Error(`Claims metadata with type '${type}' not found.`)
throw new InvalidArgumentException(`Claims metadata with type '${type}' not found.`)
}
}
}