dchester / epilogue

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

Fix bug on search attributes type checking #93

Closed OlivierLeborne closed 9 years ago

OlivierLeborne commented 9 years ago

Hy,

There is a bug on search since 08683277748a1739833be4d7293ef8731f35ca06 : the check if attributes are either STRING or TEXT is done against undefined.

mbroadst commented 9 years ago

@OlivierLeborne hey, I don't think that's correct but maybe you've uncovered a related bug? I'm checking locally with my tests and Sequelize is always a valid instance, were you able to initialize epilogue without a Sequelize instance somehow?

OlivierLeborne commented 9 years ago

@mbroadst In my app, I've tested using Sequelize 3.5.1 and 3.7.0. The Sequelize in list.js is a valid instance but STRING and TEXT are undefined. When checking with Sequelize code, I found that datatypes are defined on the constructor but not replicated on instances of sequelize. However, they however set this constructor as a property on each instance explicitly for this usage: https://github.com/sequelize/sequelize/blob/master/lib/sequelize.js#L219

A reference to Sequelize constructor from sequelize. Useful for accessing DataTypes, Errors etc.

This was already the case in 3.5.1: https://github.com/sequelize/sequelize/blob/e3b9e02c41b5e4074620ffddc7df3fd3e5a8ca8b/lib/sequelize.js#L213

The Sequelize variable in the current version of list.js might be a bit confusing. At the top of the document, it is defined as require('sequelize'), which returns the constructor that has datatypes as direct properties. This would allow the validation code to work but this line is commented out. It is defined as an instance of Sequelize later in the same file.

mbroadst commented 9 years ago

@OlivierLeborne I've just run the tests for search against both 3.5.1 and 3.7.0 and they operate on valid instances of sequelize, can you please check that you are indeed passing in a valid instance to epilogue's initialize method? I do see that the resources automatically created for auto-association do not seem to maintain proper references to the top level Sequelize and that seems like a bug.

My cheap test right now is simply to add console.log(Sequelize.STRING) around line 45 in list.js and then run npm test. You'll see that all of the basic search tests run fine, and indeed print a valid STRING object.

mbroadst commented 9 years ago

@OlivierLeborne AH, this looks like a documentation issue. In the front page example of how to use epilogue I stupidly pass in an instance of Sequelize rather than the top level prototype. Can you please try doing that instead? It seems to me that the solution for this is to check during initialize whether the passed in Sequelize object has a Sequelize member (in this case you're passing in an instance) and to use that instead

OlivierLeborne commented 9 years ago

@mbroadst thank you very much, this solved my issue :+1:

mbroadst commented 9 years ago

@OlivierLeborne if you wait a minute I'm pushing a change to support both methods :)

mbroadst commented 9 years ago

@OlivierLeborne published version 0.6.1, should be fixed there using either approach

OlivierLeborne commented 9 years ago

@mbroadst thank you very much for your help and reactivity. Epilogue was already the best/simplest/most powerful package I found to expose sequelize as a REST resource and now I find out it also has a great community.

mbroadst commented 9 years ago

@OlivierLeborne make sure to give it a star on github and npmjs.org, for some reason people keep making clones :)