tommybananas / finale

Create flexible REST endpoints and controllers from Sequelize models in your Express app
187 stars 36 forks source link

Tests failing: search included model field doesn't work on restify #58

Closed drama-ostrich closed 4 years ago

drama-ostrich commented 4 years ago

Hey my recent PR has a test that fails for restify. I noticed that providing a search query like ?model.includedmodel=value allows searching for fields in included models, but apparently that isn't true for restify.

In list.js, in the section to search for extraSearchCriteria, the param key would either be model or model.includedmodel depending on if you're using restify or not. This is due to the way Restify parses the req.query. With Restify the query would be an object with the shape of

{ model: { includedmodel: 'value' } }

and without Restify it will be

{ model.indludedmodel: 'value' }

So I think there's a question here of if this type of search query should be allowed or encouraged at all. For now I'll put up a PR to disable the test for Restify.

ShaunParsons commented 4 years ago

Looks like this is a change in restify. From the docs: http://restify.com/docs/plugins-api/#queryparser

queryParser has an option called allowDots which is apparently false by default, but I think the docs might be out of date.

So the change you need is:

server.use(restify.plugins.queryParser({ allowDots: false }));
drama-ostrich commented 4 years ago

Thanks @ShaunParsons I updated the PR. The allowDots fixes it.

tommybananas commented 4 years ago

i'll get this included asap

tommybananas commented 4 years ago

I just published 1.1.1 with this included