search

dchester / epilogue

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

Only validate field type for *likesearch operators #113

Closed chaunax closed 8 years ago

chaunax commented 8 years ago

I think validation of the field type is only relevant for "like" type search operators. This change allows other field types to be usable with searches by validating Sequelize.TEXT and Sequelize.STRING only if the search operator is in the "like" family.

mbroadst commented 8 years ago

@chaunax thanks for the contribution, I think the idea is right but I have a single question in its execution. I'm ready to merge if you can fix that, as well as the test you've broken in the process: using '$eq' as a custom search operator (maybe add a new test to ensure this behavior still works?)

chaunax commented 8 years ago

Thanks, changing it to a regex is a no-brainer thanks to your perf test.

I'm confused why travis is failing the tests; they've been running fine locally. I've tested in both 4.2.1 and 0.12.7. Any pointers?

mbroadst commented 8 years ago

@chaunax afaict when looking this morning the actual test is using an $eq which probably used to fall into the bucket of "skip this thing" which it is no longer doing - that's my guess having not touched the code at all.

On another note the tests really could use some sprucing up to actually print out the error when something goes wrong (they are generally making requests to a url and asserting that there is no error without printing something). That probably would have helped you a lot :smile:

mbroadst commented 8 years ago

@chaunax you could probably also optimize the regex, I just put in all values but it seems like you could get away with just the lowercase versions and making it case insensitive or something. Also a note in the code for why you're doing that would go a long way for future developers

chaunax commented 8 years ago

I looked into optimizing the regex, but chrome seems to like the simple OR'd statement best, and in firefox it's negligible. Since it's a lot more readable, I'd like to keep it as is--the operators should be case specific anyway, right? http://jsperf.com/regex-suffix-match/2

I really don't know what's going on with Travis. I enabled it on my fork, and like my local env, all builds there are running just fine: https://travis-ci.org/chaunax/epilogue/builds/89548713

I don't know why something in the PR process is causing the tests to not pass, but I'm pretty new to this. I'm at a loss about what I can do if I can't repro the test failure from my end.

mbroadst commented 8 years ago

@chaunax hey sorry that was my fault, we've had travis caching the node_modules folder which it shouldn't have been doing. I manually cherry-picked your changes into master, did a little cleanup and published a new version 0.6.4. Thanks for the contribution!