Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

fix: have webpush router handle ClientErrors #1141

Merged
merged 1 commit into from
Mar 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 7 additions & 12 deletions autopush/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,8 @@
Factory,
Attribute)

from boto.dynamodb2.exceptions import (
ItemNotFound,
)
import boto3
import botocore
from boto.dynamodb2.table import Table # noqa
from boto3.resources.base import ServiceResource # noqa
from boto3.dynamodb.conditions import Key
from boto3.exceptions import Boto3Error
Expand All @@ -77,9 +73,8 @@

import autopush.metrics
from autopush import constants
from autopush.exceptions import AutopushException
from autopush.exceptions import AutopushException, ItemNotFound
from autopush.metrics import IMetrics # noqa
from autopush.types import ItemLike # noqa
from autopush.utils import (
generate_hash,
normalize_id,
Expand All @@ -95,7 +90,7 @@

# Typing
T = TypeVar('T') # noqa
TableFunc = Callable[[str, int, int, ServiceResource], Table]
TableFunc = Callable[[str, int, int, ServiceResource], Any]

key_hash = ""
TRACK_DB_CALLS = False
Expand Down Expand Up @@ -149,7 +144,7 @@ def create_rotating_message_table(
write_throughput=5, # type: int
boto_resource=None # type: DynamoDBResource
):
# type: (...) -> Table # noqa
# type: (...) -> Any # noqa
"""Create a new message table for webpush style message storage"""
tablename = make_rotating_tablename(prefix, delta, date)

Expand Down Expand Up @@ -227,7 +222,7 @@ def get_rotating_message_tablename(
def create_router_table(tablename="router", read_throughput=5,
write_throughput=5,
boto_resource=None):
# type: (str, int, int, DynamoDBResource) -> Table
# type: (str, int, int, DynamoDBResource) -> Any
"""Create a new router table

The last_connect index is a value used to determine the last month a user
Expand Down Expand Up @@ -332,7 +327,7 @@ def _expiry(ttl):

def get_router_table(tablename="router", read_throughput=5,
write_throughput=5, boto_resource=None):
# type: (str, int, int, DynamoDBResource) -> Table
# type: (str, int, int, DynamoDBResource) -> Any
"""Get the main router table object

Creates the table if it doesn't already exist, otherwise returns the
Expand Down Expand Up @@ -401,7 +396,7 @@ def wrapper(self, *args, **kwargs):


def has_connected_this_month(item):
# type: (ItemLike) -> bool
# type: (Dict[str, Any]) -> bool
"""Whether or not a router item has connected this month"""
last_connect = item.get("last_connect")
if not last_connect:
Expand Down Expand Up @@ -790,7 +785,7 @@ def get_uaid(self, uaid):

@track_provisioned
def register_user(self, data):
# type: (ItemLike) -> Tuple[bool, Dict[str, Any]]
# type: (Dict[str, Any]) -> Tuple[bool, Dict[str, Any]]
"""Register this user

If a record exists with a newer ``connected_at``, then the user will
Expand Down
4 changes: 4 additions & 0 deletions autopush/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,7 @@ class LogCheckError(Exception):

class InvalidConfig(Exception):
"""Error in initialization of AutopushConfig"""


class ItemNotFound(Exception):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😞 cause boto3 no longer provides such a useful thing....

"""Signals missing DynamoDB Item data"""
11 changes: 5 additions & 6 deletions autopush/router/webpush.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
from StringIO import StringIO
from typing import Any # noqa

from boto.dynamodb2.exceptions import ItemNotFound
from boto.exception import JSONResponseError
from botocore.exceptions import ClientError
from twisted.internet.threads import deferToThread
from twisted.web.client import FileBodyProducer
from twisted.internet.defer import (
Expand All @@ -27,7 +26,7 @@
from twisted.logger import Logger
from twisted.web._newclient import ResponseFailed

from autopush.exceptions import RouterException
from autopush.exceptions import ItemNotFound, RouterException
from autopush.metrics import make_tags
from autopush.protocol import IgnoreBody
from autopush.router.interface import RouterResponse
Expand Down Expand Up @@ -101,7 +100,7 @@ def route_notification(self, notification, uaid_data):
# - Error (db error): Done, return 503
try:
yield self._save_notification(uaid_data, notification)
except JSONResponseError:
except ClientError:
raise RouterException("Error saving to database",
status_code=503,
response_body="Retry Request",
Expand All @@ -118,7 +117,7 @@ def route_notification(self, notification, uaid_data):
# - Error (no client) : Done, return 404
try:
uaid_data = yield deferToThread(router.get_uaid, uaid)
except JSONResponseError:
except ClientError:
returnValue(self.stored_response(notification))
except ItemNotFound:
self.metrics.increment("updates.client.deleted")
Expand Down Expand Up @@ -240,4 +239,4 @@ def _save_notification(self, uaid_data, notification):
#############################################################
def _eat_db_err(self, fail):
"""errBack for ignoring provisioned throughput errors"""
fail.trap(JSONResponseError)
fail.trap(ClientError)
8 changes: 1 addition & 7 deletions autopush/tests/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import signal
import subprocess

import boto
import psutil
from twisted.internet import reactor

Expand All @@ -19,7 +18,7 @@


def setUp():
for name in ('boto', 'boto3', 'botocore'):
for name in ('boto3', 'botocore'):
logging.getLogger(name).setLevel(logging.CRITICAL)
global ddb_process, boto_resource
cmd = " ".join([
Expand Down Expand Up @@ -54,11 +53,6 @@ def tearDown():
os.kill(p.pid, signal.SIGTERM)
ddb_process.wait()

# Clear out the boto config that was loaded so the rest of the tests run
# fine
for section in boto.config.sections(): # pragma: nocover
boto.config.remove_section(section)


_multiprocess_shared_ = True

Expand Down
5 changes: 1 addition & 4 deletions autopush/tests/test_db.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@
from datetime import datetime, timedelta

from autopush.websocket import ms_time
from boto.dynamodb2.exceptions import (
ItemNotFound,
)
from botocore.exceptions import ClientError
from mock import Mock, patch
import pytest
Expand All @@ -27,7 +24,7 @@
DatabaseManager,
DynamoDBResource
)
from autopush.exceptions import AutopushException
from autopush.exceptions import AutopushException, ItemNotFound
from autopush.metrics import SinkMetrics
from autopush.utils import WebPushNotification

Expand Down
2 changes: 1 addition & 1 deletion autopush/tests/test_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
has_connected_this_month,
Message,
)
from autopush.exceptions import ItemNotFound
from autopush.logging import begin_or_register
from autopush.main import ConnectionApplication, EndpointApplication
from autopush.utils import base64url_encode, normalize_id
Expand Down Expand Up @@ -454,7 +455,6 @@ def test_webpush_data_delivery_to_connected_client(self):

@inlineCallbacks
def test_webpush_data_delivery_to_connected_client_uaid_fail(self):
from boto.dynamodb2.exceptions import ItemNotFound
client = yield self.quick_register()
self.conn.db.router.get_uaid = Mock(side_effect=ItemNotFound)
assert client.channels
Expand Down
22 changes: 14 additions & 8 deletions autopush/tests/test_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import ssl

import pytest
from botocore.exceptions import ClientError
from mock import Mock, PropertyMock, patch
from twisted.trial import unittest
from twisted.internet.error import ConnectionRefusedError
Expand All @@ -24,7 +25,7 @@
from autopush.db import (
Message
)
from autopush.exceptions import RouterException
from autopush.exceptions import ItemNotFound, RouterException
from autopush.metrics import SinkMetrics
from autopush.router import (
APNSRouter,
Expand Down Expand Up @@ -1116,10 +1117,12 @@ def test_amend(self):
assert resp == expected

def test_route_to_busy_node_save_throws_db_error(self):
from boto.dynamodb2.exceptions import JSONResponseError

def throw():
raise JSONResponseError(500, "Whoops")
raise ClientError(
{'Error': {'Code': 'InternalServerError'}},
'mock_store_message'
)

self.agent_mock.request.return_value = response_mock = Mock()
response_mock.code = 202
Expand All @@ -1139,10 +1142,12 @@ def verify_deliver(fail):
return d

def test_route_lookup_uaid_fails(self):
from boto.dynamodb2.exceptions import JSONResponseError

def throw():
raise JSONResponseError(500, "Whoops")
raise ClientError(
{'Error': {'Code': 'InternalServerError'}},
'mock_get_uaid'
)

self.message_mock.store_message.return_value = True
self.db.message_table = Mock(return_value=self.message_mock)
Expand All @@ -1161,7 +1166,6 @@ def verify_deliver(status):
return d

def test_route_lookup_uaid_not_found(self):
from boto.dynamodb2.exceptions import ItemNotFound

def throw():
raise ItemNotFound()
Expand Down Expand Up @@ -1198,7 +1202,6 @@ def verify_deliver(status):
return d

def test_route_and_clear_failure(self):
from boto.dynamodb2.exceptions import JSONResponseError
self.agent_mock.request = Mock(side_effect=ConnectionRefusedError)
self.message_mock.store_message.return_value = True
self.message_mock.all_channels.return_value = (True, [dummy_chid])
Expand All @@ -1208,7 +1211,10 @@ def test_route_and_clear_failure(self):
self.router_mock.get_uaid.return_value = router_data

def throw():
raise JSONResponseError(500, "Whoops")
raise ClientError(
{'Error': {'Code': 'InternalServerError'}},
'mock_clear_node'
)

self.router_mock.clear_node.side_effect = MockAssist([throw])
self.router.message_id = uuid.uuid4().hex
Expand Down
9 changes: 5 additions & 4 deletions autopush/tests/test_web_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@
from hashlib import sha256

import ecdsa
from boto.dynamodb2.exceptions import (
ItemNotFound,
)
from cryptography.fernet import InvalidToken
from cryptography.exceptions import InvalidSignature
from jose import jws
Expand All @@ -17,7 +14,11 @@
from twisted.trial import unittest

from autopush.metrics import SinkMetrics
from autopush.exceptions import InvalidRequest, InvalidTokenException
from autopush.exceptions import (
InvalidRequest,
InvalidTokenException,
ItemNotFound
)
from autopush.tests.support import test_db
import autopush.utils as utils

Expand Down
2 changes: 1 addition & 1 deletion autopush/tests/test_webpush_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import attr
import factory
from boto.dynamodb2.exceptions import ItemNotFound
from mock import Mock
from twisted.logger import globalLogPublisher
import pytest
Expand All @@ -19,6 +18,7 @@
)
from autopush.metrics import SinkMetrics
from autopush.config import AutopushConfig
from autopush.exceptions import ItemNotFound
from autopush.logging import begin_or_register
from autopush.tests.support import TestingLogObserver
from autopush.utils import WebPushNotification, ns_time
Expand Down
9 changes: 1 addition & 8 deletions autopush/types.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
"""Common types"""
from boto.dynamodb2.items import Item
from typing import (
Any,
Dict,
Union
)
from typing import Any, Dict


# no mypy reucrsive types yet:
# https://github.com/python/mypy/issues/731
JSONDict = Dict[str, Any]

ItemLike = Union[Item, Dict[str, Any]]
3 changes: 1 addition & 2 deletions autopush/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@

from autopush.exceptions import (InvalidTokenException, VapidAuthException)
from autopush.jwt import repad, VerifyJWT
from autopush.types import ItemLike # noqa
from autopush.web.base import AUTH_SCHEMES


Expand Down Expand Up @@ -469,7 +468,7 @@ def expired(self, at_time=None):

@classmethod
def from_message_table(cls, uaid, item):
# type: (uuid.UUID, ItemLike) -> WebPushNotification
# type: (uuid.UUID, Dict[str, Any]) -> WebPushNotification
"""Create a WebPushNotification from a message table item"""
key_info = cls.parse_sort_key(item["chidmessageid"])
if key_info["api_ver"] in ["01", "02"]:
Expand Down
6 changes: 4 additions & 2 deletions autopush/web/health.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""Health Check HTTP Handler"""

import cyclone.web
from boto.dynamodb2.exceptions import InternalServerError
from botocore.exceptions import ClientError
from twisted.internet.defer import DeferredList
from twisted.internet.threads import deferToThread

Expand Down Expand Up @@ -61,7 +61,9 @@ def _check_error(self, failure, name):
cause = self._health_checks[name] = {"status": "NOT OK"}
if failure.check(MissingTableException):
cause["error"] = failure.value.message
elif failure.check(InternalServerError): # pragma nocover
elif (failure.check(ClientError) and
failure.value.response["Error"]["Code"] ==
"InternalServerError"): # pragma nocover
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is becoming a reoccurring pattern. I wonder if we should create a function that just does this combined check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally, I almost created one in this diff but figured I'd keep these short/readable. Logged #1142

cause["error"] = "Server error"
else:
cause["error"] = "Internal error" # pragma nocover
Expand Down
3 changes: 1 addition & 2 deletions autopush/web/registration.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
)

import simplejson as json
from boto.dynamodb2.exceptions import ItemNotFound
from cryptography.hazmat.primitives import constant_time
from marshmallow import (
Schema,
Expand All @@ -25,7 +24,7 @@
from twisted.internet.threads import deferToThread

from autopush.db import generate_last_connect, hasher
from autopush.exceptions import InvalidRequest, RouterException
from autopush.exceptions import InvalidRequest, ItemNotFound, RouterException
from autopush.types import JSONDict # noqa
from autopush.utils import generate_hash, ms_time
from autopush.web.base import (
Expand Down
2 changes: 1 addition & 1 deletion autopush/web/webpush.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import re
import time

from boto.dynamodb2.exceptions import ItemNotFound
from cryptography.fernet import InvalidToken
from cryptography.exceptions import InvalidSignature
from marshmallow import (
Expand Down Expand Up @@ -32,6 +31,7 @@
from autopush.exceptions import (
InvalidRequest,
InvalidTokenException,
ItemNotFound,
VapidAuthException,
)
from autopush.types import JSONDict # noqa
Expand Down
Loading