Skip to content

[don't merge]poc: customize statusCode using response spec #1260

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.

Sign up for GitHub

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

Closed
jannyHou wants to merge 1 commit into from
Closed

[don't merge]poc: customize statusCode using response spec #1260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions packages/openapi-spec-builder/src/openapi-spec-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,18 @@ export class OpenApiSpecBuilder extends BuilderBase<OpenApiSpec> {

return this.withOperation(verb, path, spec);
}

withOperationReturningStringWithStatusCode(
Copy link
Contributor

@virkt25 virkt25 Apr 24, 2018

Choose a reason for hiding this comment

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

Instead of adding more functions can we just add statusCode as an optional parameter for the existing functions?

verb: string,
path: string,
statusCode: number,
operationName?: string,
): this {
const spec = anOperationSpec().withStringResponse(statusCode);
if (operationName) spec.withOperationName(operationName);

return this.withOperation(verb, path, spec);
}
}

/**
Expand Down
6 changes: 5 additions & 1 deletion packages/rest/src/internal-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,11 @@ export type InvokeMethod = (
* @param response The response the response to send to.
* @param result The operation result to send.
*/
export type Send = (response: ServerResponse, result: OperationRetval) => void;
export type Send = (
response: ServerResponse,
result: OperationRetval,
route?: ResolvedRoute,
) => void;

/**
* Reject the request with an error.
Expand Down
2 changes: 1 addition & 1 deletion packages/rest/src/sequence.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export class DefaultSequence implements SequenceHandler {
const result = await this.invoke(route, args);

debug('%s result -', route.describe(), result);
this.send(res, result);
this.send(res, result, route);
} catch (err) {
this.reject(res, req, err);
}
Expand Down
10 changes: 10 additions & 0 deletions packages/rest/src/writer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import {ServerResponse as Response} from 'http';
import {OperationRetval} from './internal-types';
import {HttpError} from 'http-errors';
import {ResolvedRoute} from './router/routing-table';
import {Readable} from 'stream';

/**
Expand All @@ -20,6 +21,7 @@ export function writeResultToResponse(
response: Response,
// result returned back from invoking controller method
result: OperationRetval,
route?: ResolvedRoute,
): void {
if (result) {
if (result instanceof Readable || typeof result.pipe === 'function') {
Expand Down Expand Up @@ -48,6 +50,14 @@ export function writeResultToResponse(
result = result.toString();
break;
}
if (route) {
const spec = route.spec;
const responsesCode = Object.keys(spec.responses || {});
if (responsesCode.length >= 1) {
const successStatusCode = responsesCode[0];
Copy link
Contributor Author

@jannyHou jannyHou Apr 16, 2018

Choose a reason for hiding this comment

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

The assumption here is risky.

Copy link
Contributor

@raymondfeng raymondfeng Apr 16, 2018

Choose a reason for hiding this comment

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

We might need to allow x-default-status-code.

Copy link
Member

@bajtos bajtos Apr 17, 2018

Choose a reason for hiding this comment

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

What are the risks?

What if we filtered responsesCode to find the first "success" code in the range 2xx, would that mitigate the risks?

Copy link
Contributor Author

@jannyHou jannyHou Apr 23, 2018

Choose a reason for hiding this comment

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

What if we filtered responsesCode to find the first "success" code in the range 2xx, would that mitigate the risks?

Much better than my approach here!

response.statusCode = parseInt(successStatusCode);
}
}
response.write(result);
}
response.end();
Expand Down
14 changes: 14 additions & 0 deletions packages/rest/test/integration/http-handler.integration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ describe('HttpHandler', () => {
.withOperationReturningString('get', '/hello', 'hello')
.withOperationReturningString('get', '/bye', 'bye')
.withOperationReturningString('post', '/hello', 'postHello')
.withOperationReturningStringWithStatusCode(
Copy link
Member

@bajtos bajtos Apr 17, 2018

Choose a reason for hiding this comment

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

Not needed for this spike, but required for the real implementation:

Please create a new describe block the 201 scenario. This set of tests is focused on testing different paths and verbs, not status codes.

'post',
'/createHello',
201,
'createHello',
)
.build();

class HelloController {
Expand All @@ -72,11 +78,19 @@ describe('HttpHandler', () => {
public async postHello(): Promise<string> {
return 'hello posted';
}

public async createHello(): Promise<string> {
return 'hello created';
Copy link
Contributor

@raymondfeng raymondfeng Apr 16, 2018

Choose a reason for hiding this comment

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

We should be able to allow a method to return an instance of HttpResponse or Promise<HttpResponse>, for example:

return HttpResponse.statusCode(201).header('Location', '...').body(...);

Copy link
Member

@bajtos bajtos Apr 17, 2018

Choose a reason for hiding this comment

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

I have a similar proposal in #788 (comment):

@post('/products')
  async create(@requestBody() obj: Product)
    : Promise<ControllerResponse<Product>> {
    const model = await this.productRepository.create(obj);
    return {
      statusCode: 201,
      // FIXME: Do not hard-code `model.id`, find the actual PK property name.
      // FIXME: Can Location contain a path only, with no hostname and scheme?
      headers: {location: `/products/${model.id}`},
      body: model,
    }
  }

IIUC, @jannyHou is exploring a different approach in this spike pull request, one where the status code is automatically inferred from the OpenAPI spec.

}
}

givenControllerClass(HelloController, spec);
});

it('returns 201 for "POST /createHello"', () => {
return client.post('/createHello').expect(201, 'hello created');
});

it('executes hello() for "GET /hello"', () => {
return client.get('/hello').expect('hello');
});
Expand Down