Convert to new URL scheme. #73
Conversation
|
This is only the first version. At minimum, the docs and examples need to be fixed to match the new url scheme. Also, I want to add some query string validation. |
| } | ||
| var errorString = 'Invalid realtime action: ' + req.params.action; | ||
| req.log.error(errorString); | ||
| return next(new restify.InternalError(errorString)); |
There was a problem hiding this comment.
I think a BadRequestError(400) might be more appropriate here.
| // `/request` handles request/response services. There is one operation on these services, namely | ||
| // "get me some data". In some sense this ought to be a GET but because the request data is fairly | ||
| // large we use a POST. | ||
| server.post('/request', | ||
| requestHandler.onRequest); |
There was a problem hiding this comment.
nit: Since this can be joined with the previous line, I would join them together. However, I understand why it it is aligned on its own line because of the statements that follow.
There was a problem hiding this comment.
Yeah, that's why I left it, but now that I've added comments, I think this doesn't need to align with the following section, so I'll join 'em.
| @@ -191,11 +192,23 @@ export function onRequest(req: Interface.IOurRequest, | |||
| return next(new restify.InternalError('Error not find blpSession.')); | |||
| } | |||
|
|
|||
| // Validate the query parameters | |||
There was a problem hiding this comment.
I will advocate moving these checks into an util middleware
|
I rebased from bloomberg/develop to pick up more examples before fixing the examples, so that's why the whole "added some commits" thing happened. |
| @@ -0,0 +1,24 @@ | |||
| // Type definitions for adding validator to restify | |||
There was a problem hiding this comment.
I think we should put this typing file alongside with the express-validator since it is not a restify-validator.
There was a problem hiding this comment.
The main thing is that I want it to be a separate file (so that we can grab updates to express-validator.d.ts without thinking too hard), but if what you're suggesting is to put it in the same directory, I'm ok with that. Suggestions for names? express-to-restify-validator? :)
|
lgtm - please squash and ensure that the commit has an appropriate summary line and body message that describes at a high-level the changes being landed. Thanks! |
| requestHandler.onSubscribe); | ||
| server.post('/unsubscribe', | ||
|
|
||
| // `/realtime` handles subscription services. There are two things you can do with subscriptions: |
The paths now reflect the operation type (i.e. request/response or subscription), and any additional info is in the query params or body. Details of how the URLs changed: - Subscribe, unsubscribe, and poll are now one path, and we distinguish between them based on POST vs. GET and query params. - Requests use query params rather than having everything in the path. - /request query params now match the BLPAPI values exactly rather than omitting the "Request" at the end of the type. This change also adds a request validator that is used for the new query params, but which also can be used for any future additions to what's in the request. Closes #59.
|
Amended yet again! :) |
|
lgtm. @ericlu88? |
|
LGTM. I will merge. |
Closes #59.