New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Boolean mode option to node.search() #3282

Merged
merged 11 commits into from Sep 10, 2018

Conversation

Projects
4 participants
@milaaraujo
Collaborator

milaaraujo commented Sep 1, 2018

1 - Update node.search to add Boolean mode: Done
2 - Update /search to use node.search(): Done

Moved from #2547

@plotsbot

This comment has been minimized.

Collaborator

plotsbot commented Sep 1, 2018

1 Message
📖 @milaaraujo Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

@milaaraujo milaaraujo changed the title from Update Search endpoints to use node.search() (WIP) to Update /search to use node.search() (WIP) Sep 1, 2018

@milaaraujo milaaraujo requested a review from jywarren Sep 1, 2018

@milaaraujo milaaraujo changed the title from Update /search to use node.search() (WIP) to Update /search to use node.search() Sep 1, 2018

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Sep 1, 2018

Hey Jeff, could you help us to review this PR?

We were thinking that you could also help us to use the unstable branch to compare the results returned by the different search types (boolean, natural...). Depending on the results we may decide to use the natural search as default. What do you think?

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 4, 2018

Hi! Is it possible to offer different types of search as parameter settable options, like here:

https://publiclab.org/search/community?order=likes

Then, you can demonstrate how one type is better than others by linking to and comparing actual results on production. But also acknowledging that for SOME uses, people may choose a non-default sort. Does this make sense? Sorry, just catching up here! It was a holiday in the US yesterday 🏖

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 4, 2018

image

@milaaraujo milaaraujo added this to Being reviewed in RGSoC 2018 Sep 4, 2018

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Sep 4, 2018

Hey @jywarren! No problem, yesterday was a holiday here in Canada too. Don't you think that we should open a new issue/PR to compare the results? So maybe we can review #3282 first and then compare results to see which mode we will keep as default... Or do you think we should do the comparison here?

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Sep 4, 2018

And we are also working on the view, so the search will be able to offer different types of search as parameter settable options. But again, we were planning on doing this in a different PR.

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 4, 2018

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Sep 5, 2018

Sorry I was not so clear! 😬
And about this PR? What do you think?

@milaaraujo milaaraujo added the review-me label Sep 5, 2018

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 5, 2018

Hi, this looks great, i love the ability to toggle boolean vs. natural language. However, I wasn't sure the title of the PR seemed accurate-- isn't /search already running off of node.search() and this PR more accurately adds the Boolean option plus making it settable via parameter?

In any case it looks great.

However it is warning in CodeClimate against the possibility of an sql injection. Is there a way to sanitize the string query so this cannot happen? I think you can do:

.where('MATCH(node_revisions.body, node_revisions.title) AGAINST("? MODE)', query.to_s + mode)

See if that works? It should then sanitize the string if I have it right. But there may also be a quotation mark typo?

https://github.com/publiclab/plots2/pull/3282/files#diff-16d89387de9be2711872969affbc5b57R42

@milaaraujo milaaraujo changed the title from Update /search to use node.search() to Adding Boolean mode option to node.search() Sep 5, 2018

milaaraujo added some commits Sep 5, 2018

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Sep 5, 2018

Hi, this looks great, i love the ability to toggle boolean vs. natural language. However, I wasn't sure the title of the PR seemed accurate-- isn't /search already running off of node.search() and this PR more accurately adds the Boolean option plus making it settable via parameter?

You are right! I changed the PR's title. And I'm trying to fix the possibility of a sql injection.

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 5, 2018

milaaraujo added some commits Sep 5, 2018

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Sep 6, 2018

Hey Jeff, I tried my best to make code climate happy, but it seems an impossible mission! 😆

I've followed your sugestion here (and it worked):

.where('MATCH(node_revisions.body, node_revisions.title) AGAINST(? IN NATURAL LANGUAGE MODE)', query.to_s)

But using the same strategy with the select does not work. For some reason the '?' is not replaced by the query:

ActiveRecord::StatementInvalid (Mysql2::Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '? IN NATURAL LANGUAGE MODE) AS score, test FROM node_revisions WHERE (MATCH(no' at line 1: SELECT node_revisions.nid, node_revisions.body, node_revisions.title, MATCH(node_revisions.body, node_revisions.title) AGAINST(? IN NATURAL LANGUAGE MODE) AS score, test FROM node_revisions WHERE (MATCH(node_revisions.body, node_revisions.title) AGAINST('test' IN NATURAL LANGUAGE MODE)))

Anyway the code that is in the master now also concatenates the string in the query, like I'm doing now, so I do not think it should be big problem!

https://github.com/publiclab/plots2/blob/master/app/models/node.rb#L38

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Sep 6, 2018

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Sep 8, 2018

Hey @jywarren, finally the possible SQL injection is fixed! Code climate is still complaining because of the number of lines, but can we just leave it the way it is? I don't think refactoring this method should be a priority right now.

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 10, 2018

Super! Congratulations and agreed re: codeClimate. Merging now!

@jywarren jywarren merged commit 312a2ef into publiclab:master Sep 10, 2018

3 checks passed

codeclimate Approved by Jeffrey Warren.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
danger/danger All green. Well done.
Details

RGSoC 2018 automation moved this from Being reviewed to Done Sep 10, 2018

@wafflebot wafflebot bot removed the in progress label Sep 10, 2018

@milaaraujo milaaraujo deleted the milaaraujo:boolean-mode-fulltext branch Sep 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment