Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rest-auth helpers #125

Merged
merged 6 commits into from
Oct 13, 2020
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
1 change: 1 addition & 0 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ Features
- Schema serving with ``SpectacularAPIView`` (Redoc and Swagger-UI views are also available)
- Optional input/output serializer component split
- Included support for:
- `django-rest-auth <https://github.com/Tivix/django-rest-auth>`_
- `django-polymorphic <https://github.com/django-polymorphic/django-polymorphic>`_ / `django-rest-polymorphic <https://github.com/apirobot/django-rest-polymorphic>`_
- `SimpleJWT <https://github.com/SimpleJWT/django-rest-framework-simplejwt>`_
- `DjangoOAuthToolkit <https://github.com/jazzband/django-oauth-toolkit>`_
Expand Down
1 change: 1 addition & 0 deletions drf_spectacular/contrib/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
__all__ = [
'django_oauth_toolkit',
'djangorestframework_camel_case',
'rest_auth',
'rest_polymorphic',
'rest_framework_jwt',
'rest_framework_simplejwt'
Expand Down
121 changes: 121 additions & 0 deletions drf_spectacular/contrib/rest_auth.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
from django.conf import settings as global_settings
from rest_framework import serializers

try:
from allauth.account.app_settings import EMAIL_VERIFICATION, EmailVerificationMethod
Copy link
Owner

Choose a reason for hiding this comment

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

please do local import and setup in the extensions instead (for contrib stuff). duplication seem minimal from a quick glance. python, basic django&drf and spectaculuar imports can stay of course.

i only did those fallbacks out of necessity in the tests when running with the packaging (missing-contrib) option. not really necessary here.

except ImportError:
EMAIL_VERIFICATION, EmailVerificationMethod = None, None

try:
from rest_auth.app_settings import JWTSerializer, TokenSerializer, UserDetailsSerializer
from rest_auth.registration.app_settings import RegisterSerializer
except ImportError:
JWTSerializer, UserDetailsSerializer, TokenSerializer, RegisterSerializer = (None, ) * 4

from drf_spectacular.extensions import OpenApiViewExtension
from drf_spectacular.utils import extend_schema

if getattr(global_settings, 'REST_USE_JWT', False):
AuthTokenSerializer = JWTSerializer
else:
AuthTokenSerializer = TokenSerializer


class DRFDefaultDetailResponseSerializer(serializers.Serializer):
detail = serializers.CharField(read_only=True, required=False)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the responses for #101

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that is would be highly desirable to fix #101 first! This is a major bug in the current schemas emitted by drf-spectacular.

Doing that first will cement in the serializer name, and thus the component name, which will be in the schema, and then it can be used there and elsewhere.

Copy link
Owner

Choose a reason for hiding this comment

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

i'm currently trying out some ideas for #101 but it is a bit tricker that one would expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tfranzel , please comment here. This is the most important part, and you ignored it.

Copy link
Owner

Choose a reason for hiding this comment

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

i have not ignored it. my time is also limited and i wanted to give a well thought out review. will comment on it very soon.



if EmailVerificationMethod and EMAIL_VERIFICATION == EmailVerificationMethod.MANDATORY:
class RegisterResponseSerializer(DRFDefaultDetailResponseSerializer): # type: ignore
pass

elif AuthTokenSerializer:
class RegisterResponseSerializer(serializers.Serializer): # type: ignore
user = UserDetailsSerializer(read_only=True)
token = AuthTokenSerializer(read_only=True)
else:
class RegisterResponseSerializer: # type: ignore
pass


class RestAuthDefaultResponseView(OpenApiViewExtension):

def view_replacement(self):

class Fixed(self.target_class):

@extend_schema(responses=DRFDefaultDetailResponseSerializer)
def post(self, request, *args, **kwargs):
pass

return Fixed


class RestAuthLoginView(OpenApiViewExtension):
target_class = 'rest_auth.views.LoginView'

def view_replacement(self):

class Fixed(self.target_class):

@extend_schema(responses=AuthTokenSerializer)
def post(self, request, *args, **kwargs):
pass

return Fixed


class RestAuthLogoutView(OpenApiViewExtension):
target_class = 'rest_auth.views.LogoutView'

def view_replacement(self):

class Fixed(self.target_class):

if getattr(global_settings, 'ACCOUNT_LOGOUT_ON_GET', None):
@extend_schema(request=None, responses=DRFDefaultDetailResponseSerializer)
def get(self, request, *args, **kwargs):
pass
else:
@extend_schema(exclude=True)
def get(self, request, *args, **kwargs):
pass

@extend_schema(request=None, responses=DRFDefaultDetailResponseSerializer)
def post(self, request, *args, **kwargs):
pass

return Fixed


class RestAuthPasswordChangeView(RestAuthDefaultResponseView):
target_class = 'rest_auth.views.PasswordChangeView'


class RestAuthPasswordResetView(RestAuthDefaultResponseView):
target_class = 'rest_auth.views.PasswordResetView'


class RestAuthPasswordResetConfirmView(RestAuthDefaultResponseView):
target_class = 'rest_auth.views.PasswordResetConfirmView'


class RestAuthRegisterView(OpenApiViewExtension):
target_class = 'rest_auth.registration.views.RegisterView'

def view_replacement(self):

class Fixed(self.target_class):

@extend_schema(
request=RegisterSerializer,
responses=RegisterResponseSerializer,
)
def post(self, request, *args, **kwargs):
pass

return Fixed


class RestAuthVerifyEmailView(RestAuthDefaultResponseView):
target_class = 'rest_auth.registration.views.VerifyEmailView'
2 changes: 2 additions & 0 deletions requirements/optionals.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
django-allauth
Copy link
Owner

Choose a reason for hiding this comment

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

please add working min versions

drf-jwt>=0.13.0
django-rest-auth
djangorestframework-simplejwt>=4.4.0
django-polymorphic>=2.1
django-rest-polymorphic>=0.1.8
Expand Down
3 changes: 2 additions & 1 deletion requirements/testing.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
diff-match-patch
Copy link
Owner

Choose a reason for hiding this comment

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

we can remove it (see other comment)

pytest>=5.3.5
pytest-django>=3.8.0
pytest-cov>=2.8.1
flake8>=3.7.9
mypy>=0.770
djangorestframework-stubs>=1.1.0
isort>=5.0.4
isort>=5.0.4
52 changes: 48 additions & 4 deletions tests/__init__.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,62 @@
import os
import sys

from diff_match_patch import diff_match_patch

from drf_spectacular.validation import validate_schema


def assert_schema(schema, reference_file):
def assert_schema(schema, reference_filename, transforms=None):
from drf_spectacular.renderers import OpenApiJsonRenderer, OpenApiYamlRenderer

schema_yml = OpenApiYamlRenderer().render(schema, renderer_context={})
# render also a json and provoke serialization issues
OpenApiJsonRenderer().render(schema, renderer_context={})

with open(reference_file.replace('.yml', '_out.yml'), 'wb') as fh:
if not os.path.exists(reference_filename):
generated_filename = reference_filename
else:
generated_filename = reference_filename.replace('.yml', '_out.yml')

with open(generated_filename, 'wb') as fh:
fh.write(schema_yml)

with open(reference_file) as fh:
assert schema_yml.decode() == fh.read()
if generated_filename == reference_filename:
raise RuntimeError(
f'{reference_filename} was not found. '
f'It has been created with the generated schema. '
f'Review carefully, as it is the baseline for subsequent checks.')
Copy link
Owner

Choose a reason for hiding this comment

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

closing parenthesis on a new line please


generated = schema_yml.decode()

with open(reference_filename) as fh:
expected = fh.read()

# This only does each transform, however it may be necessary to do all
# combinations of transforms.
# Force to be a list, and insert identity
if transforms:
tfranzel marked this conversation as resolved.
Show resolved Hide resolved
transforms = [lambda x: x] + list(transforms)
else:
transforms = [lambda x: x]

results = []
for result in (transformer(generated) for transformer in transforms):
if result not in results:
results.append(result)
if result == expected:
break
else:
dmp = diff_match_patch()
diff = dmp.diff_main(expected, generated)
dmp.diff_cleanupSemantic(diff)
patch = dmp.patch_toText(dmp.patch_make(diff))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This library is better than nothing, and quite good for short textual differences, especially within a single line, but the current results are far from perfect for large chunks of multiline differences.

The outstanding rest-polymorphic difference here is not exactly easy to grok.

Maybe the invocation can be improved by tweaking some of its options.

Doing a plain unified diff is also an option; that at least leverages existing brain circuitry for people familiar with diffs of YAML.

Copy link
Owner

Choose a reason for hiding this comment

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

i just had a quick look if python's included batteries are good enough. looks good to me. i think we can refrain from adding a new dependency here.

    diff = difflib.unified_diff(
        schema_ref.splitlines(True),
        schema_yml.decode().splitlines(True),
    )
    diff = ''.join(diff)
    assert not diff, diff

assert patch, f'Failed to generate patch from "{expected}" to "{generated}"'
msg = f"Patch from {reference_filename} to {generated_filename}: {patch}"
if len(transforms) and len(results) != len(transforms):
msg = f'{len(transforms) - 1} transformers ineffective: {msg}'
print(msg, file=sys.stderr)
assert expected == generated, msg

# this is more a less a sanity check as checked-in schemas should be valid anyhow
validate_schema(schema)
Expand Down
21 changes: 19 additions & 2 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ def pytest_configure(config):
)

contrib_apps = [
'rest_auth',
'rest_auth.registration',
Copy link
Owner

Choose a reason for hiding this comment

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

why not also 'allauth' here, since it is an optional contrib package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this PR is adding rest-auth, not allauth. It is testing only one scenario of configuration for rest-auth. allauth is much bigger, and I will need an extension for it also, but one step at a time.

I have of course run local Django project with each major scenario, and gone through the code of these apps to check that the drf-spectacular extension covers all of the branching that is required.

This means there is code which handles scenarios which are not tested in CI. That seems on par with the other auth extensions. Having multiple tox testenvs for things in 'contrib' seems overboard. If you want to each 100% test coverage, there are much lower hanging fruit outside of contrib, which should be addressed before applying that criteria to contrib extensions.

Copy link
Owner

Choose a reason for hiding this comment

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

i'm not talking about coverage or not having tests for allauth. that is ok for now. merely saying that it is an optional package and thus should only be included, if the module is available. that is the purpose of contrib_apps. you would potentially break all tests for --skip-missing-contrib if the package is not there.

'rest_framework_jwt',
'oauth2_provider',
# this is not strictly required and when added django-polymorphic
Expand Down Expand Up @@ -69,6 +71,8 @@ def pytest_configure(config):
'django.contrib.staticfiles',
'rest_framework',
'rest_framework.authtoken',
'allauth',
'allauth.account',
*[app for app in contrib_apps if module_available(app)],
'drf_spectacular',
'tests',
Expand Down Expand Up @@ -120,10 +124,23 @@ def pytest_collection_modifyitems(config, items):
item.add_marker(allow_contrib_fail)


@pytest.hookimpl(hookwrapper=True, tryfirst=True)
def pytest_runtest_makereport(item, call):
""" store outcome result in request """
outcome = yield
rep = outcome.get_result()
setattr(item, "rep_" + rep.when, rep)
return rep


@pytest.fixture()
def no_warnings(capsys):
""" make sure test emits no warnings """
def no_warnings(capsys, request):
""" make sure successful test emits no warnings """
yield capsys

if request.node.rep_call.failed:
Copy link
Owner

Choose a reason for hiding this comment

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

what is the rational behind this? why is it needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See commit messages please.

return

captured = capsys.readouterr()
assert not captured.out
assert not captured.err
Expand Down
40 changes: 40 additions & 0 deletions tests/contrib/test_rest_auth.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import re

import pytest
from django.urls import include, path

from drf_spectacular.generators import SchemaGenerator
from tests import assert_schema

transforms = [
# User model first_name differences
lambda x: re.sub(r'(first_name:\n *type: string\n *maxLength:) 150', r'\g<1> 30', x, re.M),
Copy link
Owner

Choose a reason for hiding this comment

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

afais those transforms do nothing. test pass without them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are required on different versions of django and drf. This is explained in the commit messages.

Copy link
Owner

Choose a reason for hiding this comment

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

ah that makes sense. now i see. can you extend the comment with something like (changed in Django 3.1) to make it more obvious.

# User model username validator tail
lambda x: x.replace(r'+$', r'+\Z'),
]


@pytest.mark.contrib('rest_auth')
Copy link
Owner

Choose a reason for hiding this comment

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

you can list multiple packages on contrib. prob should include the `allauth`` package since its optional

def test_rest_auth(no_warnings):
urlpatterns = [
path('rest-auth/', include('rest_auth.urls')),
]

generator = SchemaGenerator(patterns=urlpatterns)
schema = generator.get_schema(request=None, public=True)

assert_schema(schema, 'tests/contrib/test_rest_auth.yml',
Copy link
Owner

Choose a reason for hiding this comment

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

i prefer

assert_schema(
    schema, 'tests/contrib/test_rest_auth.yml', transforms=transforms
)

or

assert_schema(
    schema, 
    'tests/contrib/test_rest_auth.yml', 
    transforms=transforms
)

depending on line length. i try not going over ~95 in general

transforms=transforms)


@pytest.mark.contrib('rest_auth')
def test_rest_auth_registration(no_warnings):
Copy link
Owner

Choose a reason for hiding this comment

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

imho you can merge those two tests together with one yaml.

urlpatterns = [
path('rest-auth/registration/', include('rest_auth.registration.urls')),
]

generator = SchemaGenerator(patterns=urlpatterns)
schema = generator.get_schema(request=None, public=True)

assert_schema(schema, 'tests/contrib/test_rest_auth_registration.yml',
transforms=transforms)
Loading