Skip to content

Fix for OAuth2::Error trying to encode non-encodable objects

Created by: jhmoore

We are pretty interested in seeing 2.0.0 released so we can unfork ourselves -- I was looking down the list of remaining items in https://github.com/oauth-xx/oauth2/milestone/1 and thought I might try to pitch in.

This is a replacement for https://github.com/oauth-xx/oauth2/pull/261 which looks dead. The issue is trying to call encode on something which does not have an encode method. However in trying to test (drive!) a fix for it, it seemed like there were no tests present at all for the OAuth2::Error class. I created a new spec file and did my best to backfill specs for the already-existing functionality (of which there is quite a bit tacked on to the inherited StandardError behavior).

The GitHub diff of the error class itself looks kind of weird because of indentation, but a brief rundown of changes:

  • Made #error_message private. Nothing outside this file talks to this method, and it sure seems like a "generate a formatted string for the error message" internal thing.
  • Renamed some locals to make me less nervous (e.g. error_message inside of def error_message, message inside a class descending from StandardError).
  • Made #initialize not rely on a local being defined only inside an if branch. (Haha I know it works because Ruby is maaaagic, but y'know)
  • If error_description is present but not error, avoid displaying an errant ": ".
  • If a formatted error description is not nil but is blank, avoid displaying a blank newline.
  • Backfilled specs for encoding and multi-line message generation.

Also: I was unable to determine why @code and @description are set and made into attr_readers -- nothing outside the file seems to talk to those things either. However, folks using this error in their implementations certainly could be relying on being able to call error.code or whatever. I elected to leave them there (and test that they are there), but open to suggestions on that front.

Merge request reports

Loading