Implement parser/unmarshaller #295
Conversation
packages/core/src/parser.ts
Outdated
| if (!match) { | ||
| debug(' -> next (path mismatch)'); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
IMO, these two if checks belong to the router. The parser should receive only the Endpoint matching the request.
packages/core/src/parser.ts
Outdated
| pathParams[key.name] = match[matchIndex]; | ||
| } | ||
|
|
||
| const operationName = endPoint._spec['x-operation-name']; |
There was a problem hiding this comment.
operationName looks unused to me, can you remove it please?
packages/core/src/parser.ts
Outdated
| const debug = require('debug')('loopback:request-parser'); | ||
| import * as Promise from 'bluebird'; | ||
| import * as url from 'url'; | ||
| import * as pathToRegexp from 'path-to-regexp'; |
There was a problem hiding this comment.
Some of these imports are unused, e.g. pathToRegexp. It would be great to clean them up.
packages/core/src/parser.ts
Outdated
| status?: number; | ||
| } | ||
|
|
||
| function createHttpError(statusCode: number, message: string) { |
There was a problem hiding this comment.
As we have discussed elsewhere (sorry, don't have a link), creating an HTTP error object is so frequent case that LoopBack should provide a public API for that. One option is to leverage http-errors module, as we are already doing in CodeHub's UserController.
Having said that, it's probably best to leave this improvement out of this initial pull request.
packages/core/src/parser.ts
Outdated
| // As per the earlier design discussion, parser() was suppose to take request and swagger spec | ||
| // as arguments. However, this parsing/unmarshalling logic needs an instance of an Endpoint which is already created | ||
| // in the router prior to parser(..) being called. Router would have mounted controllers and created endpoints | ||
| // before parser() gets called. Hence Swagger spec is not needed here. |
There was a problem hiding this comment.
IIUC, the only reason why we need the Endpoint instance is to access the arguments mapped to path parameters. IMO, this is something that should be handled by the router.
The signature of the parser function then can become something like the following:
export function parser(
request: Request, pathParameters: StringToStringMap, spec: OperationObject)
: OperationArgs
|
@rashmihunt I added one more commit where I addressed my comments and integrated the new parse with SwaggerRouter. PTAL. |
|
@bajtos LGTM. I am ok with you going ahead and merging this PR. This way we can do smaller, step by step decompose changes into master. I will work on unmarshaller/write next. |
rashmihunt
changed the title
|
@rashmihunt sounds good. I have added at least few basic unit tests to make it easier to follow TDD in the future - see 4f506c9 |
|
I have also renamed the function |
@bajtos @raymondfeng Feel free to provide initial feedback.