Properly throw MsalServiceError exception

msal/exceptions.py

@@ -27,12 +27,26 @@
 
 class MsalError(Exception):
     # Define the template in Unicode to accommodate possible Unicode variables
-    msg = u'An unspecified error'
+    msg = u'An unspecified error'  # Keeping for backward compatibility
 
-    def __init__(self, *args, **kwargs):
-        super(MsalError, self).__init__(self.msg.format(**kwargs), *args)
-        self.kwargs = kwargs
 
 class MsalServiceError(MsalError):
-    msg = u"{error}: {error_description}"
+    msg = u"{error}: {error_description}"  # Keeping for backward compatibility
+    def __init__(
+        self,
+        *args,
+        error: str, error_description: str,  # Historically required, keeping them for now
+            # 1. We can't simply remove them, or else it will be a breaking change
+            # 2. We may change them to optional without breaking anyone. However,
+            #    such a change will be a one-way change, because once being optional,
+            #    we will never be able to change them (back) to be required.
+            # 3. Since they were required and already exist anyway,
+            #    now we just keep them required "for now",
+            #    just in case that we would use them again.
+            # There is no plan to do #1; and we keep option #2 open; we go with #3.
+        **kwargs,
+    ):
+        super().__init__(*args, **kwargs)
+        self._error = error
+        self._error_description = error_description
 

msal/throttled_http_client.py

@@ -66,7 +66,10 @@ def __init__(self, raw_response):
     # self.raise_for_status = raw_response.raise_for_status
     def raise_for_status(self):
         if self.status_code >= 400:
-            raise MsalServiceError("HTTP Error: {}".format(self.status_code))
+            raise MsalServiceError(
+                "HTTP Error: {}".format(self.status_code),
+                error=None, error_description=None,  #  Historically required, keeping them for now
+            )
 
 
 class ThrottledHttpClientBase(object):

tests/test_throttled_http_client.py

@@ -6,6 +6,7 @@
 
 from msal.throttled_http_client import (
     ThrottledHttpClientBase, ThrottledHttpClient, NormalizedResponse)
+from msal.exceptions import MsalServiceError
 
 from tests import unittest
 from tests.http_client import MinimalResponse as _MinimalResponse
@@ -65,6 +66,26 @@ class CloseMethodCalled(Exception):
     pass
 
 
+class NormalizedResponseTestCase(unittest.TestCase):
+    def test_pickled_minimal_response_should_contain_signature(self):
+        self.assertIn(MinimalResponse.SIGNATURE, pickle.dumps(MinimalResponse(
+            status_code=200, headers={}, text="foo")))
+
+    def test_normalized_response_should_not_contain_signature(self):
+        response = NormalizedResponse(MinimalResponse(
+            status_code=200, headers={}, text="foo"))
+        self.assertNotIn(
+            MinimalResponse.SIGNATURE, pickle.dumps(response),
+            "A pickled object should not contain undesirable data")
+        self.assertEqual(response.text, "foo", "Should return the same response text")
+
+    def test_normalized_response_raise_for_status_should_raise(self):
+        response = NormalizedResponse(MinimalResponse(
+            status_code=400, headers={}, text="foo"))
+        with self.assertRaises(MsalServiceError):
+            response.raise_for_status()
+
+
 class ThrottledHttpClientBaseTestCase(unittest.TestCase):
 
     def assertCleanPickle(self, obj):
@@ -77,10 +98,6 @@ def assertValidResponse(self, response):
         self.assertIsInstance(response, NormalizedResponse)
         self.assertCleanPickle(response)
 
-    def test_pickled_minimal_response_should_contain_signature(self):
-        self.assertIn(MinimalResponse.SIGNATURE, pickle.dumps(MinimalResponse(
-            status_code=200, headers={}, text="foo")))
-
     def test_throttled_http_client_base_response_should_tolerate_headerless_response(self):
         http_client = ThrottledHttpClientBase(DummyHttpClientWithoutResponseHeaders(
             status_code=200, response_text="foo"))