Skip to content

Commit

Permalink
Use warnings.warn rather than log warning in app log
Browse files Browse the repository at this point in the history
  • Loading branch information
lafrech committed Oct 19, 2020
1 parent e06325d commit e2e04bc
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 76 deletions.
5 changes: 2 additions & 3 deletions docs/etag.rst
Original file line number Diff line number Diff line change
Expand Up @@ -201,9 +201,8 @@ It is up to the developer to call
:meth:`Blueprint.check_etag <Blueprint.check_etag>` in the view function. It
can't be automatic.

If ETag is enabled and :meth:`check_etag <Blueprint.check_etag>` is not called,
a warning is logged at runtime. When in `DEBUG` or `TESTING` mode, an exception
is raised.
A warning is issued if ETag is enabled and
:meth:`check_etag <Blueprint.check_etag>` is not called.

Include Headers Content in ETag
-------------------------------
Expand Down
35 changes: 16 additions & 19 deletions flask_smorest/etag.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@
from functools import wraps
from copy import deepcopy
import http
import warnings

import hashlib

from marshmallow import Schema
from flask import request, current_app, json

from .exceptions import (
CheckEtagNotCalledError,
PreconditionRequired, PreconditionFailed, NotModified)
from .utils import deepupdate, get_appcontext

Expand Down Expand Up @@ -150,15 +150,16 @@ def check_etag(self, etag_data, etag_schema=None):
Must be called from resource code to check ETag.
Unfortunately, there is no way to call it automatically. It is the
developer's responsability to do it. However, a warning is logged at
developer's responsability to do it. However, a warning is issued at
runtime if this function was not called.
Logs a warning if called in a method other than one of
PUT, PATCH, DELETE.
Issues a warning if called in a method other than PUT, PATCH, or
DELETE.
"""
if request.method not in self.METHODS_NEEDING_CHECK_ETAG:
current_app.logger.warning(
'ETag cannot be checked on {} request.'.format(request.method))
warnings.warn(
'ETag cannot be checked on {} request.'.format(request.method)
)
if _is_etag_enabled():
etag_schema = etag_schema or _get_etag_ctx().get('etag_schema')
new_etag = self._generate_etag(etag_data, etag_schema)
Expand All @@ -169,23 +170,18 @@ def check_etag(self, etag_data, etag_schema=None):
def _verify_check_etag(self):
"""Verify check_etag was called in resource code
Log a warning if ETag is enabled but check_etag was not called in
Issues a warning if ETag is enabled but check_etag was not called in
resource code in a PUT, PATCH or DELETE method.
Raise CheckEtagNotCalledError when in debug or testing mode.
This is called automatically. It is meant to warn the developer about
an issue in his ETag management.
"""
if request.method in self.METHODS_NEEDING_CHECK_ETAG:
if not _get_etag_ctx().get('etag_checked'):
message = (
warnings.warn(
'ETag not checked in endpoint {} on {} request.'
.format(request.endpoint, request.method))
app = current_app
app.logger.warning(message)
if app.debug or app.testing:
raise CheckEtagNotCalledError(message)
.format(request.endpoint, request.method)
)

def _check_not_modified(self, etag):
"""Raise NotModified if etag is in If-None-Match header
Expand All @@ -207,12 +203,13 @@ def set_etag(self, etag_data, etag_schema=None):
decorated with the ``response`` decorator, in which case the ETag is
computed by default from response data if ``set_etag`` is not called.
Logs a warning if called in a method other than one of
GET, HEAD, POST, PUT, PATCH.
Issues a warning if called in a method other than GET, HEAD, POST, PUT
or PATCH.
"""
if request.method not in self.METHODS_ALLOWING_SET_ETAG:
current_app.logger.warning(
'ETag cannot be set on {} request.'.format(request.method))
warnings.warn(
'ETag cannot be set on {} request.'.format(request.method)
)
if _is_etag_enabled():
etag_schema = etag_schema or _get_etag_ctx().get('etag_schema')
new_etag = self._generate_etag(etag_data, etag_schema)
Expand Down
4 changes: 0 additions & 4 deletions flask_smorest/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ class MissingAPIParameterError(FlaskSmorestError):
"""Missing API parameter"""


class CheckEtagNotCalledError(FlaskSmorestError):
"""ETag enabled on resource but check_etag not called"""


class NotModified(wexc.HTTPException, FlaskSmorestError):
"""Resource was not modified (Etag is unchanged)
Expand Down
8 changes: 4 additions & 4 deletions flask_smorest/pagination.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@
- Post-pagination: the resource returns an iterator (typically a DB cursor) and
a pager is provided to paginate the data and get the total number of items.
"""

from copy import deepcopy
from collections import OrderedDict
from functools import wraps
import http
import json
import warnings

from flask import request, current_app
from flask import request

import marshmallow as ma
from webargs.flaskparser import FlaskParser
Expand Down Expand Up @@ -199,8 +199,8 @@ def wrapper(*args, **kwargs):
# Set pagination metadata in response
if self.PAGINATION_HEADER_FIELD_NAME is not None:
if page_params.item_count is None:
current_app.logger.warning(
'item_count not set in endpoint {}'
warnings.warn(
'item_count not set in endpoint {}.'
.format(request.endpoint)
)
else:
Expand Down
67 changes: 31 additions & 36 deletions tests/test_etag.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,23 @@
from collections import OrderedDict
import json
import hashlib
from unittest import mock

import pytest

from flask import jsonify, Response
from flask import jsonify, Response, request as f_request
from flask.views import MethodView

from flask_smorest import Api, Blueprint, abort
from flask_smorest.etag import _get_etag_ctx
from flask_smorest.exceptions import (
CheckEtagNotCalledError,
NotModified, PreconditionRequired, PreconditionFailed)
from flask_smorest.utils import get_appcontext

from .mocks import ItemNotFound


HTTP_METHODS = ['OPTIONS', 'HEAD', 'GET', 'POST', 'PUT', 'PATCH', 'DELETE']
HTTP_METHODS_ALLOWING_SET_ETAG = ['GET', 'HEAD', 'POST', 'PUT', 'PATCH']


@pytest.fixture(params=[True, False])
Expand Down Expand Up @@ -251,7 +250,7 @@ def test_etag_check_etag_wrong_method_warning(
app.config['ETAG_DISABLED'] = etag_disabled
blp = Blueprint('test', __name__)

with mock.patch.object(app.logger, 'warning') as mock_warning:
with pytest.warns(None) as record:
with app.test_request_context(
'/',
method=method,
Expand All @@ -263,52 +262,43 @@ def test_etag_check_etag_wrong_method_warning(
except PreconditionFailed:
pass
if method in ['PUT', 'PATCH', 'DELETE']:
assert not mock_warning.called
assert not record
else:
assert mock_warning.called
assert len(record) == 1
assert record[0].category == UserWarning
assert str(record[0].message) == (
'ETag cannot be checked on {} request.'
.format(method)
)

@pytest.mark.parametrize('method', HTTP_METHODS)
def test_etag_verify_check_etag_warning(self, app, method):
blp = Blueprint('test', __name__)
old_item = {'item_id': 1, 'db_field': 0}
old_etag = blp._generate_etag(old_item)

with mock.patch.object(app.logger, 'warning') as mock_warning:
with pytest.warns(None) as record:
with app.test_request_context(
'/',
method=method,
headers={'If-Match': old_etag},
):
blp._verify_check_etag()
if method in ['PUT', 'PATCH', 'DELETE']:
assert mock_warning.called
assert len(record) == 1
assert record[0].category == UserWarning
assert str(record[0].message) == (
'ETag not checked in endpoint {} on {} request.'
.format(f_request.endpoint, method)
)
else:
assert not mock_warning.called
assert not record
blp.check_etag(old_item)
mock_warning.reset_mock()
record.clear()
blp._verify_check_etag()
assert not mock_warning.called
assert not record

@pytest.mark.parametrize('method', HTTP_METHODS)
@pytest.mark.parametrize('debug', (True, False))
@pytest.mark.parametrize('testing', (True, False))
def test_etag_verify_check_etag_exception(
self, app, method, debug, testing):
app.config['DEBUG'] = debug
app.config['TESTING'] = testing
blp = Blueprint('test', __name__)

with app.test_request_context('/', method=method):
if (debug or testing) and method in ['PUT', 'PATCH', 'DELETE']:
with pytest.raises(
CheckEtagNotCalledError,
match='ETag not checked in endpoint'
):
blp._verify_check_etag()
else:
blp._verify_check_etag()

@pytest.mark.parametrize('method', HTTP_METHODS)
@pytest.mark.parametrize('method', HTTP_METHODS_ALLOWING_SET_ETAG)
@pytest.mark.parametrize('etag_disabled', (True, False))
def test_etag_set_etag(self, app, schemas, method, etag_disabled):
app.config['ETAG_DISABLED'] = etag_disabled
Expand Down Expand Up @@ -369,18 +359,23 @@ def test_etag_set_etag(self, app, schemas, method, etag_disabled):

@pytest.mark.parametrize('etag_disabled', (True, False))
@pytest.mark.parametrize('method', HTTP_METHODS)
def test_set_etag_method_not_allowed_warning(
def test_etag_set_etag_method_not_allowed_warning(
self, app, method, etag_disabled):
app.config['ETAG_DISABLED'] = etag_disabled
blp = Blueprint('test', __name__)

with mock.patch.object(app.logger, 'warning') as mock_warning:
with pytest.warns(None) as record:
with app.test_request_context('/', method=method):
blp.set_etag(None)
if method in ['GET', 'HEAD', 'POST', 'PUT', 'PATCH']:
assert not mock_warning.called
if method in HTTP_METHODS_ALLOWING_SET_ETAG:
assert not record
else:
assert mock_warning.called
assert len(record) == 1
assert record[0].category == UserWarning
assert str(record[0].message) == (
'ETag cannot be set on {} request.'
.format(method)
)

@pytest.mark.parametrize('paginate', (True, False))
def test_etag_set_etag_in_response(self, app, schemas, paginate):
Expand Down
21 changes: 11 additions & 10 deletions tests/test_pagination.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
from itertools import product
from collections import namedtuple
import json
from unittest import mock

import pytest

Expand Down Expand Up @@ -196,16 +195,18 @@ def func(pagination_parameters):
api.register_blueprint(blp)
client = app.test_client()

with mock.patch.object(app.logger, 'warning') as mock_warning:
with pytest.warns(None) as record:
response = client.get('/test/')
assert response.status_code == 200
assert 'X-Pagination' not in response.headers
if header_name is None:
assert mock_warning.call_count == 0
else:
assert mock_warning.call_count == 1
assert mock_warning.call_args == (
('item_count not set in endpoint test.func',), )
if header_name is None:
assert not record
else:
assert len(record) == 1
assert record[0].category == UserWarning
assert str(record[0].message) == (
'item_count not set in endpoint test.func.'
)
assert response.status_code == 200
assert 'X-Pagination' not in response.headers

@pytest.mark.parametrize('collection', [1000, ], indirect=True)
def test_pagination_parameters(self, app_fixture):
Expand Down

0 comments on commit e2e04bc

Please sign in to comment.