Repository: praw-dev/praw

Issue:
KeyError from corrupted image uploads
On behalf of u/Smoking-Snake (https://www.reddit.com/r/redditdev/comments/g7s1yx/praw_keyerror_redirect_when_trying_to_upload/)

**Describe the bug**
<!-- A clear and concise description of what the bug is. -->Invalid media uploads return an empty payload but a type of failed. PRAW expects a payload with a redirect, and since there is no payload, a KeyError is thrown.

**Expected behavior**
<!-- A clear and concise description of what you expected to happen. -->
An exception that the media is valid should be raised.

**Code/Logs**
<!-- include your code, without the Reddit() initialization, so as to not leak private credentials. -->

**System Info**
 - OS: ?
 - Python: ?
 - PRAW Version: 7.0.0


Discussion:
Pyprohly: I don’t recommend trying to shoehorn every API exception you find into the `RedditAPIException` class. This class is a concrete class that should only be used for the API errors that look like “`{'json': {'errors': [['NO_TEXT', 'we need something here', 'title’]]}}`”. Anything that deviates from this format even slightly deserves its own class.

Even if there were a `BaseAPIException` I wouldn’t make `MediaPostFailed` a subclass of it since it’s not a part of the regular REST API and uses a different protocol.

bboe: > Even if there were a BaseAPIException I wouldn’t make MediaPostFailed a subclass of it since it’s not a part of the regular REST API and uses a different protocol.

That's an interesting point. All such exceptions should be under the umbrella of a PRAW API type exception. Users of the library shouldn't care which API PRAW is talking to in order to distinguish between what exceptions might be thrown. But under that, we could distinguish between classes of exceptions.
PythonCoderAS: > I don’t recommend trying to shoehorn every API exception you find into the `RedditAPIException` class. This class is a concrete class that should only be used for the API errors that look like “`{'json': {'errors': [['NO_TEXT', 'we need something here', 'title’]]}}`”. Anything that deviates from this format even slightly deserves its own class.
> 
> Even if there were a `BaseAPIException` I wouldn’t make `MediaPostFailed` a subclass of it since it’s not a part of the regular REST API and uses a different protocol.

Do you think it would be better to subclass this from `WebsocketException`, but then we would need to rework that as well. We could split `WebsocketException` into `WebsocketException` and `WebsocketConnectionException`, maybe. 
Pyprohly: Ah yes, there’s a `WebSocketException` exception. I do think it’s better have `MediaPostFailed` inherit from that instead.

Why would `WebSocketException` need reworking though?
PythonCoderAS: > Ah yes, there’s a `WebSocketException` exception. I do think it’s better have `MediaPostFailed` inherit from that instead.
> 
> Why would `WebSocketException` need reworking though?

The main reason is that `WebSocketException` currently has it's own set message and basically wraps an existing exception with a custom message.
Pyprohly: Yea, right. That `.original_exception` attribute shouldn’t be there.

Never wrap exceptions, it’s better to chain them: `raise MyException from exc`. Fix it.

PythonCoderAS: > Yea, right. That `.original_exception` attribute shouldn’t be there.
> 
> Never wrap exceptions, it’s better to chain them: `raise MyException from exc`. Fix it.

We can't do that now, it's been part of PRAW 7. It needs to be deprecated. 

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 1aa4382c..bc9621d4 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -25,6 +25,10 @@ Unreleased
 * :meth:`.SubredditRemovalReasons.__getitem__` now takes integers and slices to
   get removal reasons from the list of reasons as returned by Reddit.
 
+**Deprecated**
+
+* :attr:`.WebSocketException.original_exception` is deprecated and slated for removal in PRAW 8.0.
+
 **Fixed**
 
 * An issue where certain subreddit settings could not be set through
@@ -34,6 +38,9 @@ Unreleased
   update request.
 * Instances of ``BadRequest``\ s captured by PRAW that do not contain any
   detailed JSON data are re-raised as the original ``BadRequest``.
+* :meth:`.submit_image` and :meth:`.submit_video` will throw
+  :class:`.MediaPostFailed` when Reddit fails to post an image or video post.
+
 
 7.0.0 (2020/04/24)
 ------------------
diff --git a/praw/exceptions.py b/praw/exceptions.py
index 8791d0f2..3a05e0e3 100644
--- a/praw/exceptions.py
+++ b/praw/exceptions.py
@@ -244,11 +244,47 @@ class TooLargeMediaException(ClientException):
 class WebSocketException(ClientException):
     """Indicate exceptions caused by use of WebSockets."""
 
-    def __init__(self, message: str, exception: Exception):
+    @property
+    def original_exception(self) -> Exception:
+        """Access the original_exception attribute (now deprecated)."""
+        warn(
+            "Accessing the attribute original_exception is deprecated. Please"
+            " rewrite your code in such a way that this attribute does not"
+            " need to be used. It will be removed in PRAW 8.0.",
+            category=DeprecationWarning,
+            stacklevel=2,
+        )
+        return self._original_exception
+
+    @original_exception.setter
+    def original_exception(self, value: Exception):
+        self._original_exception = value
+
+    @original_exception.deleter
+    def original_exception(self):
+        del self._original_exception
+
+    def __init__(self, message: str, exception: Optional[Exception]):
         """Initialize a WebSocketException.
 
         :param message: The exception message.
         :param exception: The exception thrown by the websocket library.
+
+            .. note:: This parameter is deprecated. It will be removed in PRAW
+                8.0.
         """
         super().__init__(message)
-        self.original_exception = exception
+        self._original_exception = exception
+
+
+class MediaPostFailed(WebSocketException):
+    """Indicate exceptions where media uploads failed.."""
+
+    def __init__(self):
+        """Instantiate MediaPostFailed."""
+        super().__init__(
+            "The attempted media upload action has failed. Possible causes"
+            " include the corruption of media files. Check that the media "
+            "file can be opened on your local machine.",
+            None,
+        )
diff --git a/praw/models/reddit/subreddit.py b/praw/models/reddit/subreddit.py
index 52e0beba..6fdce155 100644
--- a/praw/models/reddit/subreddit.py
+++ b/praw/models/reddit/subreddit.py
@@ -18,6 +18,7 @@ from ...const import API_PATH, JPEG_HEADER
 from ...exceptions import (
     ClientException,
     InvalidFlairTemplateID,
+    MediaPostFailed,
     RedditAPIException,
     TooLargeMediaException,
     WebSocketException,
@@ -612,6 +613,8 @@ class Subreddit(
                 "the original exception.",
                 ws_exception,
             )
+        if ws_update.get("type") == "failed":
+            raise MediaPostFailed
         url = ws_update["payload"]["redirect"]
         return self._reddit.submission(url=url)
 


Test patch:
diff --git a/tests/unit/models/reddit/test_subreddit.py b/tests/unit/models/reddit/test_subreddit.py
index b51d8055..28b4e7a2 100644
--- a/tests/unit/models/reddit/test_subreddit.py
+++ b/tests/unit/models/reddit/test_subreddit.py
@@ -1,7 +1,9 @@
+import json
 import pickle
-
 import pytest
+from unittest import mock
 
+from praw.exceptions import MediaPostFailed
 from praw.models import Subreddit, WikiPage
 from praw.models.reddit.subreddit import SubredditFlairTemplates
 
@@ -61,6 +63,21 @@ class TestSubreddit(UnitTest):
         assert hash(subreddit2) != hash(subreddit3)
         assert hash(subreddit1) != hash(subreddit3)
 
+    @mock.patch(
+        "praw.Reddit.post",
+        return_value={"json": {"data": {"websocket_url": ""}}},
+    )
+    @mock.patch("praw.models.Subreddit._upload_media", return_value="")
+    @mock.patch("websocket.create_connection")
+    def test_invalid_media(
+        self, connection_mock, _mock_upload_media, _mock_post
+    ):
+        connection_mock().recv.return_value = json.dumps(
+            {"payload": {}, "type": "failed"}
+        )
+        with pytest.raises(MediaPostFailed):
+            self.reddit.subreddit("test").submit_image("Test", "dummy path")
+
     def test_pickle(self):
         subreddit = Subreddit(
             self.reddit, _data={"display_name": "name", "id": "dummy"}
diff --git a/tests/unit/test_deprecations.py b/tests/unit/test_deprecations.py
index 49b9e654..57d3f7da 100644
--- a/tests/unit/test_deprecations.py
+++ b/tests/unit/test_deprecations.py
@@ -2,7 +2,7 @@
 
 import pytest
 
-from praw.exceptions import APIException
+from praw.exceptions import APIException, WebSocketException
 
 from . import UnitTest
 
@@ -39,3 +39,14 @@ class TestDeprecation(UnitTest):
             "/other/subredditrules.html#praw.models.reddit.rules."
             "SubredditRules.__call__)."
         )
+
+    def test_web_socket_exception_attribute(self):
+        exc = WebSocketException("Test", Exception("Test"))
+        with pytest.raises(DeprecationWarning) as excinfo:
+            _ = exc.original_exception
+        assert (
+            excinfo.value.args[0]
+            == "Accessing the attribute original_exception is deprecated."
+            " Please rewrite your code in such a way that this attribute does "
+            "not need to be used. It will be removed in PRAW 8.0."
+        )
diff --git a/tests/unit/test_exceptions.py b/tests/unit/test_exceptions.py
index c6a422fa..0822945f 100644
--- a/tests/unit/test_exceptions.py
+++ b/tests/unit/test_exceptions.py
@@ -8,6 +8,7 @@ from praw.exceptions import (
     InvalidFlairTemplateID,
     InvalidImplicitAuth,
     InvalidURL,
+    MediaPostFailed,
     MissingRequiredAttributeException,
     PRAWException,
     RedditAPIException,
@@ -178,10 +179,29 @@ class TestWebSocketException:
             str(WebSocketException("error message", None)) == "error message"
         )
 
+    @pytest.mark.filterwarnings("ignore", category=DeprecationWarning)
     def test_exception_attr(self):
-        assert WebSocketException(None, None).original_exception is None
+        exc = WebSocketException(None, None)
+        assert exc.original_exception is None
         assert isinstance(WebSocketException(None, Exception()), Exception)
         assert (
             str(WebSocketException(None, Exception("test")).original_exception)
             == "test"
         )
+        exc.original_exception = Exception()
+        assert isinstance(exc.original_exception, Exception)
+        del exc.original_exception
+        assert "_original_exception" not in vars(exc)
+
+
+class TestMediaPostFailed:
+    def test_inheritance(self):
+        assert issubclass(MediaPostFailed, WebSocketException)
+
+    def test_message(self):
+        assert (
+            str(MediaPostFailed())
+            == "The attempted media upload action has failed. Possible causes"
+            " include the corruption of media files. Check that the media "
+            "file can be opened on your local machine."
+        )