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.

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

fix: use parameter level decorators for openapi params #940

Merged
merged 2 commits into from
Feb 5, 2018
Merged

fix: use parameter level decorators for openapi params #940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d2ebf6
fix: use parameter level decorators for openapi params
raymondfeng Jan 31, 2018
4dcd8b9
fix(metadata): add more tests
raymondfeng Feb 5, 2018
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
32 changes: 19 additions & 13 deletions packages/example-getting-started/src/controllers/todo.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ export class TodoController {
@repository(TodoRepository.name) protected todoRepo: TodoRepository,
) {}
@post('/todo')
@param.body('todo', TodoSchema)
async createTodo(todo: Todo) {
Copy link
Member

Choose a reason for hiding this comment

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

Allowing parameter-level decorators at method level was an intentional decision we made early on, so that we can support the following pattern:

class MyController() {
  @param.body('todo', TodoSchema) {
  async createTodo(...args) {
   return await this.todoRepo.create(...args);
  }   
}

As long as the example above keeps working (we keep it covered by our automated tests), I think the changes you are proposing here are a matter of subjective preference and can be landed if we all agree that's the style we want to promote.

I personally like more the old (method-level) style:

  • All REST decorators are specified in a single place (before the method)
  • The method signature is easier to read, because it's not cluttered by decorators

Compare:

// now
@get('/todo/{id}')
@param.path.number('id')
@param.query.boolean('items')
async findTodoById(id: number, items?: boolean): Promise<Todo> {
  // ...
}

// proposed
@get('/todo/{id}')
async findTodoById(
  @param.path.number('id') id: number, 
  @param.query.boolean('items') items?: boolean,
): Promise<Todo> {
  // ...
}

Having said that, the difference is not super important to me, so if there are more people in favor of your proposed style, then I am ok with that.

@strongloop/sq-lb-apex @strongloop/loopback-next opinions?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bajtos I think the method level decorators promote a top-down approach since the intended use for them seems to be is to have the users provide a schema, but I'm not so sure anymore. Currently, type inference is only possible with the parameter-level decorators and it seems like that's what we should promote since it is better for the UX, but I think it's also possible to infer the parameter types through method-level decorators, so I guess the change isn't strictly needed if we plan on adding in support for method-level decorators' parameter type inferrence.

Copy link
Contributor

@jannyHou jannyHou Feb 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Update: the following old comment is not a concern now. The new decorator requestBody allows overriding the generated schema by decorator's input.

@bajtos Interesting, I also come across a decision making about "method decorator" or "parameter decorator", or both.

The benefit of "parameter decorator" is it guarantees the consistency between openapi spec and controller function's inputs. But it may also have limitation:
Take the openapi3 requestBody spec as an example, one requestBody can have multiple content-type, if they don't share a common schema, we need to apply @param.body, which will equivalent to @requestBody in openapi3, to multiple parameters, but ideally one function only has one input for body data.

I started a discussion in #753 (comment)
And post my concern in #753 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I love the parameter decorator, even given my concern above, I would still like to try adjust the decorator code to support the multiple contents.

async createTodo(
@param.body('todo', TodoSchema)
todo: Todo,
) {
// TODO(bajtos) This should be handled by the framework
// See https://github.com/strongloop/loopback-next/issues/118
if (!todo.title) {
Expand All @@ -23,9 +25,10 @@ export class TodoController {
}

@get('/todo/{id}')
@param.path.number('id')
@param.query.boolean('items')
async findTodoById(id: number, items?: boolean): Promise<Todo> {
async findTodoById(
@param.path.number('id') id: number,
@param.query.boolean('items') items?: boolean,
): Promise<Todo> {
return await this.todoRepo.findById(id);
}

Expand All @@ -35,22 +38,25 @@ export class TodoController {
}

@put('/todo/{id}')
@param.path.number('id')
@param.body('todo', TodoSchema)
async replaceTodo(id: number, todo: Todo): Promise<boolean> {
async replaceTodo(
@param.path.number('id') id: number,
@param.body('todo', TodoSchema)
todo: Todo,
): Promise<boolean> {
return await this.todoRepo.replaceById(id, todo);
}

@patch('/todo/{id}')
@param.path.number('id')
@param.body('todo', TodoSchema)
async updateTodo(id: number, todo: Todo): Promise<boolean> {
async updateTodo(
@param.path.number('id') id: number,
@param.body('todo', TodoSchema)
todo: Todo,
): Promise<boolean> {
return await this.todoRepo.updateById(id, todo);
}

@del('/todo/{id}')
@param.path.number('id')
async deleteTodo(id: number): Promise<boolean> {
async deleteTodo(@param.path.number('id') id: number): Promise<boolean> {
return await this.todoRepo.deleteById(id);
}
}
4 changes: 2 additions & 2 deletions packages/example-getting-started/test/acceptance/application.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import {createClientForHandler, expect, supertest} from '@loopback/testlab';
import {RestServer} from '@loopback/rest';
import {TodoApplication} from '../../src/application';
import {TodoRepository} from '../../src/repositories/index';
import {TodoRepository} from '../../src/repositories/';
import {givenTodo} from '../helpers';
import {Todo} from '../../src/models/index';
import {Todo} from '../../src/models/';

describe('Application', () => {
let app: TodoApplication;
Expand Down
2 changes: 2 additions & 0 deletions packages/metadata/src/decorator-factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,12 +284,14 @@ export class DecoratorFactory<
Reflector.getMetadata(this.key, target),
);
meta = this.mergeWithInherited(meta, target, member, descriptorOrIndex);
/* istanbul ignore if */
if (debug.enabled) {
debug('%s: %j', targetName, meta);
}
Reflector.defineMetadata(this.key, meta, target);
} else {
meta = this.mergeWithOwn(meta, target, member, descriptorOrIndex);
/* istanbul ignore if */
if (debug.enabled) {
debug('%s: %j', targetName, meta);
}
Expand Down
32 changes: 32 additions & 0 deletions packages/metadata/test/unit/decorator-factory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,38 @@ describe('ClassDecoratorFactory', () => {
});
});

describe('ClassDecoratorFactory for primitive types', () => {
/**
* Define `@classDecorator(spec)`
* @param spec
*/
function classDecorator(spec: number): ClassDecorator {
return ClassDecoratorFactory.createDecorator('test', spec);
}

const xSpec = 1;
@classDecorator(xSpec)
class BaseController {}

@classDecorator(2)
class SubController extends BaseController {}

it('applies metadata to a class', () => {
const meta = Reflector.getOwnMetadata('test', BaseController);
expect(meta).to.equal(xSpec);
});

it('merges with base class metadata', () => {
const meta = Reflector.getOwnMetadata('test', SubController);
expect(meta).to.equal(2);
});

it('does not mutate base class metadata', () => {
const meta = Reflector.getOwnMetadata('test', BaseController);
expect(meta).to.equal(1);
});
});

describe('ClassDecoratorFactory with create', () => {
interface MySpec {
x?: number;
Expand Down