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

Bugfix: failure of generating error-logs when a HTTP response body includes multibyte-chars #208

Merged

Conversation

expajp
Copy link
Contributor

@expajp expajp commented Jul 4, 2020

I found a bug that Encoding::CompatibilityError is raised on Rsolr::Error::SolrContext#to_s and fixed it.

The error occurs during generating error-logs after Rsolr::Client#execute is received a response whose status is 4xx and body includes multibyte-characters(cf: Japanese).

     Failure/Error: m << p
     
     Encoding::CompatibilityError:
       incompatible character encodings: ASCII-8BIT and UTF-8
     # ./lib/rsolr/error.rb:22:in `to_s'

@jrochkind
Copy link
Contributor

I'm just trying to review some of these old ones. If formerly Rsolr::Error::SolrContext#to_s raising is the problem, is there a feasible way to put in a test that would have failed before this PR with a raise, but pass after it?

I'm personally having trouble reproducing that.

Can anyone think of any reason this might cause backwards incompatibility probelms?

@bess bess merged commit 5ea30b8 into rsolr:master Dec 15, 2021
jrochkind added a commit that referenced this pull request Dec 15, 2021
@jrochkind
Copy link
Contributor

jrochkind commented Dec 15, 2021

I'm afraid merging this caused a CI failure in main branch that I don't understand. See green again on the revert PR #225.

I guess CI never actually was running on this PR, there's no record of CI passing for it. (Even if it had been, it's last run may have been on an old main branch of course).

I'm going to revert it, then try to re-open it, possibly merged/rebased on master. If I can figure out the relevant git commands to do that.

@jrochkind
Copy link
Contributor

what I think we REALLY need to do is rewrite the whole Rsolr::Error thing, which is a weird mess of code nobody would write today.

jrochkind added a commit that referenced this pull request Dec 15, 2021
Apparently response can be nil in some cases. Fix to what was in #208 to avoid that introducing a regression in cases where response is nil.
@jrochkind
Copy link
Contributor

jrochkind commented Dec 15, 2021

OK fixed with a simple nil guard in #226 This will still be in release 2.4.

One case where response can be nil is a response for a timeout error. Our recently introduced test for timeout error handling caught a bug that was in this here PR regardless, thankfully!

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

Successfully merging this pull request may close these issues.

None yet

3 participants