After having a few endpoints, before continuing, it might be best to stop and look at ways in which the code can be a bit more efficient with less repetition. For now, I refactored only the user_auth and courses endpoints, as these two had tests and the tests can be used to verify the code refactor.
To begin with, the view functions were handling too many exceptions. While it is necessary to produce explicit error messages along with appropriate status codes so that the frontend does not have to do much processing, I found several except blocks producing the same error. For example, in the user register view POST method:
def post(self, *args, **kwargs):
'''Create new user from API request'''
try:
user = RegisterUserSerializer(data=self.request.data)
except Exception as e:
logger.critical(
'Error in registering new user - {error}'.format(error=str(e))
)
return Response(
data=DEFAULT_ERROR_RESPONSE,
status=status.HTTP_400_BAD_REQUEST
)
new_user = None
if user.is_valid():
try:
new_user = user.save()
send_verification_link_email(new_user)
logger.info('New user {} created'.format(new_user.username))
return Response(user.data, status=status.HTTP_201_CREATED)
except Http400Error as e:
logger.error(
'Error in registering new user {username} - {error}'.format(
username=user.data['username'],
error=str(e)
)
)
return Response(
data=str(e),
status=status.HTTP_400_BAD_REQUEST
)
except Exception as e:
logger.critical(
'Error in registering new user - {error}'.format(
error=str(e)
)
)
return Response(
data=DEFAULT_ERROR_RESPONSE,
status=status.HTTP_400_BAD_REQUEST
)
else:
return serializer_error_response(user, 'Could not register user')
I am returning a 400 error response in four different places - the first in case the RegisterUserSerializer fails maybe due to corrupt request object, the second when there is a problem with the user model instance creation, the third is if there is an unexpected error and the last is if there is a serializer error such as missing username or password etc. All I need is two errors - the first is if there is a missing field or wrong API request and the second is if there is an unexpected error.
So, a better way to write this would be:
def post(self, *args, **kwargs):
'''Create new user from API request'''
try:
user = RegisterUserSerializer(data=self.request.data)
new_user = user.save()
send_verification_link_email(new_user)
logger.info('New user {} created'.format(new_user.username))
return Response(user.data, status=status.HTTP_201_CREATED)
except Http400Error as e:
logger.error(
'Error in registering new user {}'.format(str(e))
)
return Response(
data=str(e),
status=status.HTTP_400_BAD_REQUEST
)
except Exception as e:
logger.critical(
'Error in registering new user - {}'.format(str(e))
)
return Response(
data=DEFAULT_ERROR_RESPONSE,
status=status.HTTP_400_BAD_REQUEST
)
This would mean that some of the exception handling would move into the serializer as the .save() method which was returning a serializer error will now have to return a custom Http400Error. For this reason, the .save() method will need to be overloaded:
def save(self):
if self.is_valid():
return super().save()
else:
raise Http400Error(
extract_serializer_error(self.errors)
)
The is_valid() which was before in the view method is now in the serializer method which seems a better place for this to occur. A new method extract_serializer_error has to be defined:
def extract_serializer_error(error, default_msg=DEFAULT_ERROR_RESPONSE):
'''Return error string as generic error'''
try:
err_message = [error[e][0]
for e in error][0]
except:
err_message = default_msg
return err_message
It merely extracts the error string from the serializer Validation error.
In refactoring the Course view class, there was a greater deal of complication. In the create() method which I was using since I was inheriting the CreateAPIView:
def create(self, request, *args, **kwargs):
'''Create a new course - POST request'''
try:
self.authenticate(request)
return super().create(request, *args, **kwargs)
except Http400Error as e:
logger.error('Error creating course - {}'.format(str(e)))
return Response(
data=str(e),
status=status.HTTP_400_BAD_REQUEST
)
except Http403Error as e:
logger.critical('Course creation by non admin attempted')
return Response(
data=str(e),
status=status.HTTP_403_FORBIDDEN
)
except ValidationError as e:
logger.error('Error creating course - {}'.format(str(e)))
return rest_framework_validation_error(e, 'Course could not be created')
except Exception as e:
logger.critical('Error creating course - {}'.format(str(e)))
return Response(
data=DEFAULT_ERROR_RESPONSE,
status=status.HTTP_400_BAD_REQUEST
)
Here, there is an unnecessary except block checking for ValidationError and which also returns a 400 error response. But, moving the validation into the overloaded save() method of the course serializer does not solve the problem:
def save(self, *args, **kwargs):
if self.is_valid():
return super().save(*args, **kwargs)
else:
raise Http400Error(extract_serializer_error(self.errors))
The reason this will not work is because of the way the create() method provided by the CreateAPIView works. Looking into the source code in the Django Rest Framework Github repository, I found that the create method is as:
def create(self, request, *args, **kwargs):
serializer = self.get_serializer(data=request.data)
serializer.is_valid(raise_exception=True)
self.perform_create(serializer)
headers = self.get_success_headers(serializer.data)
return Response(serializer.data, status=status.HTTP_201_CREATED, headers=headers)
The create() method runs is_valid() under the hood and therefore will not wait for the overloaded save() method of the serializer to be called. The result is that exceptions will be of type serializers.ValidationError. The only way forward is to repeat some of the code:
def post(self, request, *args, **kwargs):
'''Create a new course - POST request'''
try:
self.authenticate(request)
serializer = self.get_serializer(data=request.data)
self.perform_create(serializer)
return Response(
serializer.data,
status=status.HTTP_201_CREATED
)
except Http400Error as e:
logger.error('Error creating course - {}'.format(str(e)))
return Response(
data=str(e),
status=status.HTTP_400_BAD_REQUEST
)
except Http403Error as e:
logger.critical('Course creation by non admin attempted')
return Response(
data=str(e),
status=status.HTTP_403_FORBIDDEN
)
except Exception as e:
logger.critical('Error creating course - {}'.format(str(e)))
return Response(
data=DEFAULT_ERROR_RESPONSE,
status=status.HTTP_400_BAD_REQUEST
)
I explicitly create the serializer from the request and then call perform_create() which has not changed. Only, I do not call is_valid() and I let the perform_create() method call the save() method:
def perform_create(self, serializer):
'''Create a new course with authenticated user as instructor'''
course = serializer.save(user=self.request.user)
logger.info(
'Creating course {course} by user {user}'.format(
course=course.title,
user=self.request.user.username
)
)
With this refactor, the code repetitions have decreased. The tests are still passing which implies on the whole the code should still work.
No comments:
Post a Comment