Skip to content
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

Handle empty search result set #1076

Merged
merged 3 commits into from Feb 12, 2018
Merged

Handle empty search result set #1076

merged 3 commits into from Feb 12, 2018

Conversation

@zeke
Copy link
Member

@zeke zeke commented Feb 7, 2018

The website went down for a bit today. To fix it, I rolled back to a previous version on Heroku.

This is an exploratory PR to help diagnose if something is wrong with the master branch.

@zeke zeke temporarily deployed to electron-website-pr-1076 Feb 7, 2018 Inactive
@zeke
Copy link
Member Author

@zeke zeke commented Feb 7, 2018

This PR doesn't differ from master and the deployment on Heroku is running just fine... 🤔

@codebytere
Copy link
Member

@codebytere codebytere commented Feb 12, 2018

there was a crash again for about an hour last night too, around 8:30pm

@zeke
Copy link
Member Author

@zeke zeke commented Feb 12, 2018

@vanessayuenn tracked down a probable cause:

Feb 07 19:14:46 electron-website app/web.1: TypeError: Cannot read property 'length' of null 
Feb 07 19:14:46 electron-website app/web.1:     at Promise.all.searchOps.map (/app/routes/search/index.js:35:47) 
Feb 07 19:14:46 electron-website app/web.1:     at <anonymous> 
Feb 07 19:14:46 electron-website app/web.1: npm ERR! code ELIFECYCLE 
Feb 07 19:14:46 electron-website app/web.1: npm ERR! errno 1 
Feb 07 19:14:46 electron-website app/web.1: npm ERR! electronjs.org@2.0.0 start: `cross-env NODE_PATH=. NODE_ENV=production node server.js` 
Feb 07 19:14:46 electron-website app/web.1: npm ERR! Exit status 1 
Feb 07 19:14:46 electron-website app/web.1: npm ERR!  
@zeke
Copy link
Member Author

@zeke zeke commented Feb 12, 2018

This is a query that kills it:

/search?q=*
@MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Feb 12, 2018

This patch should handle that error, it looks like we handle missing results for the results key but not for the resultsNum key

diff --git a/routes/search/index.js b/routes/search/index.js
index 46ab389c..9b615c39 100644
--- a/routes/search/index.js
+++ b/routes/search/index.js
@@ -32,7 +32,7 @@ module.exports = (req, res) => {
     return {
       searchOp,
       // FIXME: use real pagination
-      resultsNum: results.data[searchOp.name].length,
+      resultsNum: results.data[searchOp.name] ? results.data[searchOp.name].length : 0,
       results: (req.params.searchIn || req.query.json !== undefined)
                 ? results.data[searchOp.name] : results.data[searchOp.name].slice(0, 5)
     }
@zeke zeke temporarily deployed to electron-website-pr-1076 Feb 12, 2018 Inactive
@zeke zeke changed the title [WIP] Why did the website crash? Handle empty search result set Feb 12, 2018
@zeke
Copy link
Member Author

@zeke zeke commented Feb 12, 2018

Thanks for sleuthing, @codebytere, @vanessayuenn, and @MarshallOfSound. I've committed a fix here.

@zeke zeke merged commit 864b6c3 into master Feb 12, 2018
4 checks passed
4 checks passed
AccessLint Review complete
WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@zeke zeke deleted the why-did-the-website-crash branch Feb 12, 2018
@vanessayuenn
Copy link
Contributor

@vanessayuenn vanessayuenn commented Feb 13, 2018

I could still reproduce crash with the same query 😢. I have added another patch at #1094.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants