mulesoft / osprey

Generate Node.JS API middleware from a RAML definition
Other
431 stars 66 forks source link

UriParameters not passed to request params. #86

Closed YvanDaSilva closed 8 years ago

YvanDaSilva commented 8 years ago

Hello, I am having an issue parsing the uri parameters when using osprey router.

Test case: RAML route

/data:
-------
  /{data_id}:
    ........
    uriParameters:
      data_id:

The osprey router config: this.router.get('/data/{data_id}', (req: express.Request, res: express.Response)=> { console.log(req)} I have tested many different options notably using "/data/:data_id" but this wouldn't recognise the route. The problem: req.params is empty, meaning the id of the resource is not retrieve.

Am I doing something wrong, is this expected ? I saw that you are depending on https://github.com/blakeembrey/router/ , is there something specific to this router in order to extract url parameters ?

Thank you in advance. Best regards.

Yvan

blakeembrey commented 8 years ago

@YvanDaSilva Currently there's no support for the propagation of param types, so when they aren't set they are omitted. I can't remember the full breadth, but I did try to implement this at one point - I'd have to retry to remember by I aborted doing it.

To make the parameters come through, you can do .get('/data/{data_id}', { data_id: { type: 'string' } }). Most likely, a workaround can be used to avoid removing undefined properties and keep the existing type they were parsed at earlier.

YvanDaSilva commented 8 years ago

Hello @blakeembrey, thank you for your reply. I see, wouldn't it be simpler to let the function handle the type of the id further down? And maybe map the url to something like '/data/:data_id' in order to be compatible with other routers structure, like the express one ?

I didn't investigate the repercussion of my proposal at all, so please let me know if this is a good approach or not and why. In case you would need some help to do this, please do not hesitate to open a new issue (if you can, detail as possible what to do) and I'll gladly help as I can.

Best, Yvan

blakeembrey commented 8 years ago

I don't understand the proposal you've made, but I can definitely see what I can do about fixing the issue.

And maybe map the url to something like '/data/:data_id' in order to be compatible with other routers structure, like the express one ?

The RAML URLs are different from Express URLs. If you want to use Express URLs, you can use the Express router. The issue isn't about mapping at all, it's actually about merging of params (the router currently omits them when not defined, they are originally defined by RAML so we could try to merge the params at each step - there was an issue doing this and I can't recall what it was).

I see, wouldn't it be simpler to let the function handle the type of the id further down?

I don't know if I answered this already, but I can't quite understand the question. The type of the ID is verified by Osprey middleware already, what do you mean by "further down"?

YvanDaSilva commented 8 years ago

@blakeembrey It was a misunderstanding from my part. I didn't understand that the router was ditching the param if it was not defined as you mentioned above. So yes it is answered already. Thank you for your reply.

blakeembrey commented 8 years ago

Ok, trying to come up with a solution and the pains are all coming back to me. The likely solution will be adding the ability to "merge" or re-use uriParameters that have already been defined. There are possible edge cases, but I think it's a net good and this is definitely a bit of a pain point.

blakeembrey commented 8 years ago

https://github.com/mulesoft/osprey/releases/tag/v0.2.0

YvanDaSilva commented 8 years ago

Nice, I saw your changes to the osprey-router. Thank you for your work.