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

API Endpoints refactoring #359

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
Open

Conversation

blu3eee
Copy link
Contributor

@blu3eee blu3eee commented Jun 14, 2024

Addresses #358

What I Did

  • Refactored redundant endpoints into one with search queries (/profiles/:account_id?fields= & account-settings/:account_id?fields=)
  • Removed re_path routes, which is returning a template index.html on every request to the endpoint that doesn't have a handler, which is causing append slash behavior to not working (more details below)
  • Adjusted test cases to fit with the refactoring

Removal of re_path(r'.*', ) in mesh/urls.py

  • This removal is to allow the appending slash behavior to work properly
  • About appending slash behavior: with the current Django setup, Django is setting APPEND_SLASH = true as a default behavior. This allows the Django app to "append" slash on requests that don't have a trailing slash by permanently redirecting the request to the correct one. For example, we have an API endpoint at /example/helloworld/, when requesting to /example/helloworld, the Django app is expected to permanently redirect the request to /example/helloworld/, acting as an "append slash" behavior. But said re_path in mesh/urls.py is causing this redirect not to take place when a request to /example/helloworld is sent to the app, because the re_path takes priority and returns the user a template HTML file as a response instead of redirecting to /example/helloworld/.
  • By removing said re_path, the "append" slash behavior is working properly, allowing users to send requests to an endpoint with or without a trailing slash.

Testing with and without trailing slash

  • As mentioned above, the appending slash behavior, under the hood, is actually permanently redirecting requests to the correct one. So when we testing a request without the trailing slash, it will return a response with a status code 301
image
  • In test cases:

❌ Incorrect way of sending test request (without trailing slash):

from django.test import TestCase, Client
class YourTestClass(TestCase):
    def setUp(self):
        self.client = Client()
    def your_test_case(self):
        response = self.client.get("/accounts")
        self.assertEqual(response.status_code, 200) # this will not pass ❌

This will causing failures as the response.status_code is 301 instead of 200, because the app is permanently redirect.

✅ Correct way of sending test requests (without trailing slash):

from django.test import TestCase, Client
class YourTestClass(TestCase):
    def setUp(self):
        self.client = Client()
    def your_test_case(self):
        response = self.client.get("/accounts", follow=True)
        self.assertEqual(response.status_code, 200) # this will pass ✅
        response = self.client.get("/accounts/")
        self.assertEqual(response.status_code, 200) # this will pass ✅

…358)

this removal is actually helping the django app's setting on appending slash to take effect
```
APPEND_SLASH = True # this is default behavior
```
By default, APPEND_SLASH is set to True so the api will accepts api requests with or without trailing slash, but adding re_path(r'.*') is affecting said behavior to not "append" slash at the end of requests.
@blu3eee blu3eee requested review from misslame and shotoh June 14, 2024 21:38
@blu3eee blu3eee changed the base branch from main to dev June 14, 2024 21:38
Copy link
Contributor

@shotoh shotoh left a comment

Choose a reason for hiding this comment

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

looks really good

mesh/exampleapi/urls.py Show resolved Hide resolved
@blu3eee blu3eee requested a review from JRSiwiecki June 18, 2024 18:36
Copy link
Collaborator

@misslame misslame left a comment

Choose a reason for hiding this comment

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

Just one question

mesh/exampleapi/views.py Show resolved Hide resolved
@blu3eee blu3eee linked an issue Jul 28, 2024 that may be closed by this pull request
Comment on lines +103 to +110
fields = request.GET.get('fields')
if fields:
fields = [field for field in fields.split(',') if field in self.valid_fields]
else:
fields = self.valid_fields
if len(fields) == 0:
return JsonResponse({'error': 'Invalid fields'}, status=400)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious... wasn't the validate_json_and_required_fields made for this or is this one a special case that requires us to not use the util function?

Copy link
Contributor Author

@blu3eee blu3eee Aug 27, 2024

Choose a reason for hiding this comment

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

tdlr: validate_json_and_required_fields was not made for this. the two use cases are different.

  • validate_json_and_required_fields is to validate the request body. It takes in a json object, check if the request body has the required fields for the action (i.e. creating account will need enough information) and returns an indicator saying if the request body is valid for the action.

  • The fields checking in this particular case is getting the fields param from the request, then parse it into an array and double-check it with valid fields to see if the requested fields are matched with return object available fields. (Kindof mimicking graphQL behavior in a sense)

    • For example, the account settings have these fields: twoFactorEnabled, theme, and notificationSetting. The fields in the request body will indicate how much information the backend is returning, in some cases, we only need partial information. So, instead of returning the whole accountSetting object from the database, we only return the requested fields. If in the request body, fields="theme,twoFactorEnabled", only those information is returned in the response.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay that makes sense, is there no need to split this into its own function as well or does this seem like it'd be a one-off operation?

Copy link
Contributor

@JRSiwiecki JRSiwiecki left a comment

Choose a reason for hiding this comment

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

Small questions, ping me on discord when you take a look at them 🍡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REFACTOR] RESTful API endpoints
4 participants