Search Solr using POST instead of GET #129
Conversation
* Switched search to post for super big queries (e.g. anything over 2048 chars)
* `get_max` => `get_max_request_entity_size` * Removed local test core from the config (cherry picked from commit ba9fa70)
|
Added feedback from @nicolasembleton on #121 and cleaned up testing config, so unit tests pass once more. |
|
Sorry for the delay. I actually really like what you did. I've came up with something very similar myself, but your code is better/cleaner. There's 1 last change I'd like to make to make sure we're taking into account this comment: #121 (comment) I'm actually thinking that to avoid this pull request to be a breaking change (since it's automatically switching to POST), we probably should default it to false as well. I'll make 2 comments in the code, please update this and I'll make sure that gets pulled in because I think it's an important change. ( @lbdremy if you have a minute I'd also like some feedback that this is the right direction ) |
| @@ -82,7 +82,8 @@ function Client(options){ | |||
| path : options.path || '/solr', | |||
| agent : options.agent, | |||
| secure : options.secure || false, | |||
| bigint : options.bigint || false | |||
| bigint : options.bigint || false, | |||
| get_max_request_entity_size: options.get_max_request_entity_size || 2048 | |||
There was a problem hiding this comment.
Default it to false, so that this is not a breaking change
get_max_request_entity_size: options.get_max_request_entity_size || false
|
@kfitzgerald I've created a 'Doing' card in Trello here: https://trello.com/c/PhoU0gUh/1-search-solr-using-post-instead-of-get If you want to add yourself as a member and take it over. (it's not mandatory, just if you're willing to). We should consider adding tests too (but I don't think it's mandatory for the PR to happen). |
|
Hey @nicolasembleton ! Looks like the Trello board is for org members or invite only. Add me maybe? (
I'm inclined to leave this on by default, only because backwards-compatibility isn't much of an issue, since existing users wont be able to complete a large search request anyway. Also, by defaulting to false, the user would then have to be aware of what the actual maximum URL length the Solr stack can support actually is. If this is the intended direction, I'd be happy to make the change! Thanks again, |
|
Thanks for the feedback @kfitzgerald. I think that for this coming release, I'd like to make it non-breaking (hence off by default) but I'd happily add a comment in the code saying that next major release will turn it on by default at 2048. I'll add a Trello card for this specific purpose. Works for you? |
* Can opt-in to auto-posting by setting `get_max_request_entity_size` to a number. (e.g. 2048) * Changed URL length check to be UTF8-friendly
|
@nicolasembleton Works for me! Incorporated changes in 85b502b, ready for review. Only other last-minute addition was a quick change from character length to byte length, just to be better stewards of internationalization. Should prevent a super narrow edge case that could happen if the request is teetering on the edge of the length limit and contains multi-byte characters. |
|
Ha! Very smart change for multi-byte chars. The PR lgtm for mergining. One last call to the guys and I'll merge it before this Sunday if no one objects: |
Search Solr using POST instead of GET (configurable auto-switch on request size, disabled by default)
Greetings,
I've made a modification to the search method to use HTTP POST instead of GET. The reasoning is that for large queries, GET will fail with 413 - Request Entity Too Large.
Since there's no real performance difference, I just simply swapped out the functionality to use POST instead of making it configurable.
I tried running unit tests, however nine fail against solr 5.1 even without the changes, so it appears like all is well.
Thanks,
-Kevin