-
Notifications
You must be signed in to change notification settings - Fork 274
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
Changes from all commits
1794cde
b70fef2
a3c7b45
5db8ddd
ccf6ca2
5970984
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is one of the responses for #101 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
django-allauth | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,8 @@ | ||
diff-match-patch | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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.') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
|
||
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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ def pytest_configure(config): | |
) | ||
|
||
contrib_apps = [ | ||
'rest_auth', | ||
'rest_auth.registration', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not also There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm not talking about coverage or not having tests for |
||
'rest_framework_jwt', | ||
'oauth2_provider', | ||
# this is not strictly required and when added django-polymorphic | ||
|
@@ -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', | ||
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the rational behind this? why is it needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. afais those transforms do nothing. test pass without them. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
# User model username validator tail | ||
lambda x: x.replace(r'+$', r'+\Z'), | ||
] | ||
|
||
|
||
@pytest.mark.contrib('rest_auth') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can list multiple packages on |
||
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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i prefer
or
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
There was a problem hiding this comment.
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.