Fix/72 many searches causing 500s #73
Conversation
custom/search.js
Outdated
| if (nextPageToken) { | ||
| options.nextPageToken = nextPageToken | ||
| } |
There was a problem hiding this comment.
this all doesn't seem right to me...
files.list takes pageToken, not nextPageToken:
https://developers.google.com/drive/api/v3/reference/files/list#parameters
There was a problem hiding this comment.
yeah I found some threads talking about how the docs were wrong
There was a problem hiding this comment.
I can revert the getAllFolders changes, they're just hitching a ride
There was a problem hiding this comment.
I'm... not convinced. can you provide any sources? I'm poking around the internals of the Google API node module and don't see anything indicating that.
I'm also curious how that explains why it worked sometimes and failed other times.
There was a problem hiding this comment.
I have some time next week to open this up again. At first glance it seemed as though there was a bug in how pagination works (or that it's just straight up broken -- I don't believe we actually have that as a feature), probably why the page limit is set to 1000 results (that's the max). I'll need to set aside some real time to get you the "why" of this.
There was a problem hiding this comment.
Here's a thread talking about a similar issue: https://issuetracker.google.com/issues/196413673?pli=1
Thread about docs being out of date: https://stackoverflow.com/questions/47402545/google-drive-js-api-nextpagetoken-invalid
Another thread about docs being out of date googleapis/google-api-nodejs-client#1885
There was a problem hiding this comment.
@zenkimoto added you here for review. There are some comments here for background. I'm still poking around to answer the "why this works" question, but if you have any insights it would be much appreciated!
I'm not exactly sure what the problem is exactly and I'm not sure if it's related to pagination. I just pulled from main and tested it appears any of the following will also fail:
accessibility, accessible, access, acce.
the and that will fail too probably because of too many results...
What I get back from the API is:
message: 'Invalid Value',
stack: 'Error: Invalid Value\n' +
' at Gaxios._request (/Users/laihoyip/Development/nytimes-library/node_modules/gaxios/build/src/gaxios.js:129:23)\n' +
' at runMicrotasks (<anonymous>)\n' +
' at processTicksAndRejections (node:internal/process/task_queues:96:5)\n' +
' at async JWT.requestAsync (/Users/laihoyip/Development/nytimes-library/node_modules/google-auth-library/build/src/auth/oauth2client.js:343:18)\n' +
' at async fullSearch (/Users/laihoyip/Development/nytimes-library/custom/search.js:45:18)\n' +
' at async Object.exports.run (/Users/laihoyip/Development/nytimes-library/custom/search.js:25:17)',
The query seems to be formulated correctly:
q: `fullText contains "access" AND mimeType != 'application/vnd.google-apps.folder' AND trashed = false`,
Also, I see that the pageSize that we're sending is 1000. Perhaps if we lower that, it won't error out. I'm not entirely sure here.
There was a problem hiding this comment.
I also just adjusted the pageSize in list.js and it also doesn't appear to help. I'm not quite sure what's going on here, @timwright12. I'll have to dig a little further when I have some time. Sorry, I'm not sure if I was much help...
There was a problem hiding this comment.
No worries at all! It's a weird one for sure. I'm going to continue to fiddle as time allows
custom/search.js
Outdated
| } | ||
|
|
||
| const {data} = await drive.files.list(options) | ||
|
|
||
| const {files, nextPageToken} = data | ||
| const {files} = data |
There was a problem hiding this comment.
so if we're deleting this, how are we getting the next page token from the response?
see: https://developers.google.com/drive/api/v3/reference/files/list#response
custom/search.js
Outdated
| } | ||
|
|
||
| const {data} = await drive.files.list(options) | ||
|
|
||
| const {files, nextPageToken} = data | ||
| const {files} = data | ||
| const combined = foldersSoFar.concat(files) | ||
|
|
||
| if (nextPageToken) { |
There was a problem hiding this comment.
this looks like you might trigger infinite recursion now --- if you pass in nextPageToken to getAllFolders, then continue calling getAllFolders again?
There was a problem hiding this comment.
I can look into it, but none of the logic really changed here
There was a problem hiding this comment.
wdym "none of the logic really changed here"?
line 74 checks if there's a nextPageToken and then calls again to get the next page of data. but if we're no longer pulling nextPageToken from the response, then how does it work?
|
@zenkimoto added you here for review. There are some comments here for background. I'm still poking around to answer the "why this works" question, but if you have any insights it would be much appreciated! |
|
This seems like it stopped working, digging back in |
Closes #72
Updated a variable name that was causing issues with the drive API
Verify changes at: https://content-library-dev.adhoc.pizza/search?q=accessibility&types=
Acceptance criteria validation
No new tests were added, current tests all pass