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

Strip non-alpha numeric or whitespace chars out of search query #205

Merged
merged 1 commit into from Jun 29, 2015

Conversation

Projects
None yet
2 participants
@theresaanna
Contributor

theresaanna commented Jun 29, 2015

This morning, in production, we had an issue where requests were timing out. After consulting New Relic, it looks like the app errored when a user submitted a couple search terms that included parentheses.

This pull requests makes it so that, when users search using the default setting, "Contains words", non-query-friendly characters are stripped out of the query, leaving just alphanumeric characters and whitespaces.

@cmc333333

This comment has been minimized.

Contributor

cmc333333 commented Jun 29, 2015

This tests that things don't explode, which is good. It'd be a little better if there was an explicit test on the transformation in convert_to_tsquery (i.e. that those chars were stripped). Important PR, though, so I'll merge after travis.

As we discussed, this can be alleviated by removing the raw=True.

@theresaanna

This comment has been minimized.

Contributor

theresaanna commented Jun 29, 2015

@cmc333333 Agreed. I wanted to think it through more before deviating from the patterns established in the tests.

I'm looking at the need to use convert_to_tsquery and raw=True when performing the search, and, from my testing and the documentation you so nicely found, it looks as though we may not need to use either. That paired with the knowledge that our search functionality is changing means I did indeed just want to get this PR through so that prod doesn't choke again.

Good suggestion.

@cmc333333

This comment has been minimized.

Contributor

cmc333333 commented Jun 29, 2015

Not sure what's up with travis, but the push branch should be the same as the PR. So 1/2 works for me.

cmc333333 added a commit that referenced this pull request Jun 29, 2015

Merge pull request #205 from 18F/search-bug
Strip non-alpha numeric or whitespace chars out of search query

@cmc333333 cmc333333 merged commit b9263a7 into master Jun 29, 2015

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@cmc333333 cmc333333 deleted the search-bug branch Jun 29, 2015

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