Repository navigation
Conversation
The Stream screen sent the expression as a GET query-string parameter, so a sufficiently large expression could be rejected by the server before ever reaching the stream handler - and the UI had no error handling for a failed request, so it just hung with no feedback. Added a POST-based action to the Query service (form-encoded body, same request shape otherwise) and switched the Stream screen to use it. Both the success and error paths now go through one response handler that falls back to showing the raw body if it isn't JSON, instead of letting an unguarded JSON.parse throw uncaught.
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
Can we use the more appropriate QUERY verb instead? Does that even work?
…uirk /simplify pass on the previous commit: "query" and "queryPost" had an identical transformResponse copy-pasted between them - extracted to a shared wrapRawResponse helper. Also added a comment on stream.js's showResult() explaining why it must handle non-JSON bodies even on the "success" path: app.js's global interceptor routes most failures for doNotIntercept requests through that callback too.
Very in favor of QUERY conceptually, but googling indicates it was added to Jetty 13. I think we are still on 12. |
|
This was a fun overveiw read.. https://medium.com/@bhushan.darandale/http-just-got-a-new-verb-and-it-fixes-a-problem-youve-been-working-around-for-years-9f71bf2a270e. Though looking at jetty/jetty.project#15316 makes me worry we won't get to use it for a while... |
|
I found a fly in the ointment.... streaming expressions is used for updates... So it's not a fit for QUERY. However, our SQL capablity is Read Only! |
|
I am opening up a fresh pR the demonstrates QUERY. I'll cross post here when I get it done. |
|
Aside from the QUERY question, does the rest look okay? |
Good point, though an interesting future feature might be to accept QUERY, but throw an error for any state modifying operation. (requiring POST for those) |
3 participants
https://issues.apache.org/jira/browse/SOLR-9759
Summary
#/<collection>/stream) sent the streaming expression as a GET query-string parameter. A sufficiently large expression could be rejected by the server (URL/header length limits) before ever reaching the stream handler, and the UI had no error handling for a failed request, so it just hung with no feedback - matching the original 2016 report exactly.queryPostaction to theQueryAngular service: same request shape as the existingqueryaction, but sent as a form-encoded (application/x-www-form-urlencoded) POST body instead of a GET query string.StreamHandler(and Solr's generic request dispatch) already support this transparently - no server-side changes needed.stream.js'sdoStream()to usequeryPost. Both the success and error paths now go through one shared response handler that falls back to showing the raw response body if it isn't valid JSON, instead of letting an unguardedJSON.parsethrow uncaught - this is what actually produced the original "UI hangs silently" symptom whenever the server's error body wasn't parseable JSON.AdminUiStreamScreenTest#testLargeExpressionSucceedsViaUi: a >16KB expression that a GET request would have failed on. Verified it fails against the pre-fix GET-based code and passes with the POST fix. AddedtestFailedRequestShowsErrorInsteadOfHangingto cover a failed request surfacing an error instead of a blank screen.Test plan
AdminUiStreamScreenTest(3 tests) passes with the fix.testLargeExpressionSucceedsViaUiconfirmed to fail against the original GET-basedstream.jsand pass with the POST-based fix (regression-tested both ways).