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

Updates outputs from the backend to camelCase for easy ingestion #1004

Merged
merged 11 commits into from
Nov 13, 2024

Conversation

andrewtavis
Copy link
Member

Contributor checklist


Description

WIP attempt to switch the backend output case to camelCase so that it can easily be ingested by the frontend type classes.

Related issue

  • Hydration issues

@andrewtavis andrewtavis requested a review from to-sta November 7, 2024 00:29
Copy link

netlify bot commented Nov 7, 2024

Deploy Preview for activist-org ready!

Name Link
🔨 Latest commit 64724fa
🔍 Latest deploy log https://app.netlify.com/sites/activist-org/deploys/6734f99ad824a70008e580d1
😎 Deploy Preview https://deploy-preview-1004--activist-org.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

github-actions bot commented Nov 7, 2024

Thank you for the pull request!

The activist team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you!

Maintainer checklist

  • The TypeScript and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The Playwright end to end and Zap penetration tests have been ran and are passing (if necessary)

  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

"djangorestframework_camel_case.render.CamelCaseJSONRenderer",
"djangorestframework_camel_case.render.CamelCaseBrowsableAPIRenderer",
),
"DEFAULT_PARSER_CLASSES": (
Copy link
Member Author

Choose a reason for hiding this comment

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

@to-sta, this is where I have the parser classes being defined as in the djangorestframework-camel-case docs, but we're still getting snake_case in responses in the frontend.

@@ -40,7 +40,7 @@ class Meta:


class OrganizationTextSerializer(serializers.ModelSerializer[OrganizationText]):
orgID = serializers.StringRelatedField(source="org_id.id")
org_id = serializers.StringRelatedField(source="org_id.id")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I needed to ignore a mypy type warning here in my PR

"VERSION": "0.1.0",
"SERVE_INCLUDE_SCHEMA": False,
"CAMELIZE_NAMES": False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see another comment where you noted you were still getting snake_case responses. Could this have something to do with it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good check :) This is specifically for the table names within the Swagger UI, not the results of the backend itself. Keeping the table names snake_case makes sense to me as they should have consistent identifiers.

@@ -10,13 +10,13 @@
<img
v-if="$colorMode.value == 'light'"
class="mb-4 h-36 sm:h-44 lg:hidden"
:src="imageURL + '_light.png'"
:src="imgUrl + '_light.png'"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think at some point it would be good to refactor conditional image fetching like this into a composable, but that's probably out of scope for this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want to make an issue for this, @cquinn540?

@@ -133,7 +133,7 @@ const props = defineProps<{

const aboveMediumBP = useBreakpoint("md");

const { linkURL } = useLinkURL(props);
const { linkUrl } = useLinkURL(props);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see useLinkURL() hasn't changed. Is that from a dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya I didn't want to change it in the function as it just seemed weird to me. In the last sync we chatted and there will be an issue to do a custom conversion for URL so that we can maintain the uppercase. For now I wanted to do a direct conversion so that we don't need to worry about anything else.

I'll make that issue now though 😊

Copy link
Member Author

Choose a reason for hiding this comment

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

#1007 has been made for this. Maybe you'd want to take a look at it once we're done here?

sortedOrgTextsData.push(text);
}
}
}

console.log(`Hereee ${JSON.stringify(sortedOrgTextsData)}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this for manual debugging? Does it need to stay in the PR?

Copy link
Member Author

@andrewtavis andrewtavis Nov 8, 2024

Choose a reason for hiding this comment

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

Ya that's not intentional. Will be removed. I'm debugging to try to return organizations on the /organizations route, and this check here is how I'm finding that we're still getting back snake_case.

@cquinn540
Copy link
Collaborator

Looks like the backend mypy test failed with the same issues in my PR.

You can steal my fixes if you want
91f6155

@to-sta
Copy link
Collaborator

to-sta commented Nov 8, 2024

@andrewtavis

I checked it and I also don't see it working. I have checked if it doesn't work global as a default and imported it locally and tested on endpoint but that also didn't change the output.

Also re-arranging the Middleware but it didn't change the fields to camelCase.

@andrewtavis
Copy link
Member Author

andrewtavis commented Nov 8, 2024

Thanks for checking this, @to-sta! I'm really confused here as isn't this exactly what djangorestframework-camel-case is supposed to do? The quickest route here is that we could say that within stores we can use snake_case and nowhere else. I'd prefer to actually have this work, but getting opinions on this would also be helpful :)

@to-sta
Copy link
Collaborator

to-sta commented Nov 8, 2024

Thanks for checking this, @to-sta! I'm really confused here as isn't this exactly what djangorestframework-camel-case is supposed to do? The quickest route here is that we could say that within stores we can use snake_case and nowhere else. I'd prefer to actually have this work, but getting opinions on this would also be helpful :)

I think you can deconstruct the response data and assign it properly. Could be done by a function or maybe a npm package?

@cquinn540
Copy link
Collaborator

Okay, I think I figured it out. Or at least it seems to work on my machine.

I think this is an import order issue. These lines:

from rest_framework import viewsets

django_stubs_ext.monkeypatch(extra_classes=(viewsets.ModelViewSet,))

from rest_framework.settings import api_settings

api_settings.reload()

need to be at the end of settings.py. You may not even need the api_settings.reload() anymore.

@andrewtavis
Copy link
Member Author

Amazing, @cquinn540! I'll give this a try later on 😊 Thanks so much for the investigation here!

@to-sta
Copy link
Collaborator

to-sta commented Nov 10, 2024

Looked into the api_settings at the current state and with @cquinn540 solution. A lot of the values are getting override, that also explains why some global default settings don't work properly.

Current api_settings
{
  "defaults": {
      "DEFAULT_RENDERER_CLASSES": [
          "rest_framework.renderers.JSONRenderer",
          "rest_framework.renderers.BrowsableAPIRenderer"
      ],
      "DEFAULT_PARSER_CLASSES": [
          "rest_framework.parsers.JSONParser",
          "rest_framework.parsers.FormParser",
          "rest_framework.parsers.MultiPartParser"
      ],
      "DEFAULT_AUTHENTICATION_CLASSES": [
          "rest_framework.authentication.SessionAuthentication",
          "rest_framework.authentication.BasicAuthentication"
      ],
      "DEFAULT_PERMISSION_CLASSES": [
          "rest_framework.permissions.AllowAny"
      ],
      "DEFAULT_THROTTLE_CLASSES": [],
      "DEFAULT_CONTENT_NEGOTIATION_CLASS": "rest_framework.negotiation.DefaultContentNegotiation",
      "DEFAULT_METADATA_CLASS": "rest_framework.metadata.SimpleMetadata",
      "DEFAULT_VERSIONING_CLASS": null,
      "DEFAULT_PAGINATION_CLASS": null,
      "DEFAULT_FILTER_BACKENDS": [],
      "DEFAULT_SCHEMA_CLASS": "rest_framework.schemas.openapi.AutoSchema",
      "DEFAULT_THROTTLE_RATES": {
          "user": null,
          "anon": null
      },
      "NUM_PROXIES": null,
      "PAGE_SIZE": null,
      "SEARCH_PARAM": "search",
      "ORDERING_PARAM": "ordering",
      "DEFAULT_VERSION": null,
      "ALLOWED_VERSIONS": null,
      "VERSION_PARAM": "version",
      "UNAUTHENTICATED_USER": "django.contrib.auth.models.AnonymousUser",
      "UNAUTHENTICATED_TOKEN": null,
      "VIEW_NAME_FUNCTION": "rest_framework.views.get_view_name",
      "VIEW_DESCRIPTION_FUNCTION": "rest_framework.views.get_view_description",
      "EXCEPTION_HANDLER": "rest_framework.views.exception_handler",
      "NON_FIELD_ERRORS_KEY": "non_field_errors",
      "TEST_REQUEST_RENDERER_CLASSES": [
          "rest_framework.renderers.MultiPartRenderer",
          "rest_framework.renderers.JSONRenderer"
      ],
      "TEST_REQUEST_DEFAULT_FORMAT": "multipart",
      "URL_FORMAT_OVERRIDE": "format",
      "FORMAT_SUFFIX_KWARG": "format",
      "URL_FIELD_NAME": "url",
      "DATE_FORMAT": "iso-8601",
      "DATE_INPUT_FORMATS": [
          "iso-8601"
      ],
      "DATETIME_FORMAT": "iso-8601",
      "DATETIME_INPUT_FORMATS": [
          "iso-8601"
      ],
      "TIME_FORMAT": "iso-8601",
      "TIME_INPUT_FORMATS": [
          "iso-8601"
      ],
      "UNICODE_JSON": true,
      "COMPACT_JSON": true,
      "STRICT_JSON": true,
      "COERCE_DECIMAL_TO_STRING": true,
      "UPLOADED_FILES_USE_URL": true,
      "HTML_SELECT_CUTOFF": 1000,
      "HTML_SELECT_CUTOFF_TEXT": "More than {count} items...",
      "SCHEMA_COERCE_PATH_PK": true,
      "SCHEMA_COERCE_METHOD_NAMES": {
          "retrieve": "read",
          "destroy": "delete"
      }
  },
  "import_strings": [
      "DEFAULT_RENDERER_CLASSES",
      "DEFAULT_PARSER_CLASSES",
      "DEFAULT_AUTHENTICATION_CLASSES",
      "DEFAULT_PERMISSION_CLASSES",
      "DEFAULT_THROTTLE_CLASSES",
      "DEFAULT_CONTENT_NEGOTIATION_CLASS",
      "DEFAULT_METADATA_CLASS",
      "DEFAULT_VERSIONING_CLASS",
      "DEFAULT_PAGINATION_CLASS",
      "DEFAULT_FILTER_BACKENDS",
      "DEFAULT_SCHEMA_CLASS",
      "EXCEPTION_HANDLER",
      "TEST_REQUEST_RENDERER_CLASSES",
      "UNAUTHENTICATED_USER",
      "UNAUTHENTICATED_TOKEN",
      "VIEW_NAME_FUNCTION",
      "VIEW_DESCRIPTION_FUNCTION"
  ],
  "_cached_attrs": []
}
@cquinn540 approach api_settings
{
    "defaults": {
        "DEFAULT_RENDERER_CLASSES": [
            "rest_framework.renderers.JSONRenderer",
            "rest_framework.renderers.BrowsableAPIRenderer"
        ],
        "DEFAULT_PARSER_CLASSES": [
            "rest_framework.parsers.JSONParser",
            "rest_framework.parsers.FormParser",
            "rest_framework.parsers.MultiPartParser"
        ],
        "DEFAULT_AUTHENTICATION_CLASSES": [
            "rest_framework.authentication.SessionAuthentication",
            "rest_framework.authentication.BasicAuthentication"
        ],
        "DEFAULT_PERMISSION_CLASSES": [
            "rest_framework.permissions.AllowAny"
        ],
        "DEFAULT_THROTTLE_CLASSES": [],
        "DEFAULT_CONTENT_NEGOTIATION_CLASS": "rest_framework.negotiation.DefaultContentNegotiation",
        "DEFAULT_METADATA_CLASS": "rest_framework.metadata.SimpleMetadata",
        "DEFAULT_VERSIONING_CLASS": null,
        "DEFAULT_PAGINATION_CLASS": null,
        "DEFAULT_FILTER_BACKENDS": [],
        "DEFAULT_SCHEMA_CLASS": "rest_framework.schemas.openapi.AutoSchema",
        "DEFAULT_THROTTLE_RATES": {
            "user": null,
            "anon": null
        },
        "NUM_PROXIES": null,
        "PAGE_SIZE": null,
        "SEARCH_PARAM": "search",
        "ORDERING_PARAM": "ordering",
        "DEFAULT_VERSION": null,
        "ALLOWED_VERSIONS": null,
        "VERSION_PARAM": "version",
        "UNAUTHENTICATED_USER": "django.contrib.auth.models.AnonymousUser",
        "UNAUTHENTICATED_TOKEN": null,
        "VIEW_NAME_FUNCTION": "rest_framework.views.get_view_name",
        "VIEW_DESCRIPTION_FUNCTION": "rest_framework.views.get_view_description",
        "EXCEPTION_HANDLER": "rest_framework.views.exception_handler",
        "NON_FIELD_ERRORS_KEY": "non_field_errors",
        "TEST_REQUEST_RENDERER_CLASSES": [
            "rest_framework.renderers.MultiPartRenderer",
            "rest_framework.renderers.JSONRenderer"
        ],
        "TEST_REQUEST_DEFAULT_FORMAT": "multipart",
        "URL_FORMAT_OVERRIDE": "format",
        "FORMAT_SUFFIX_KWARG": "format",
        "URL_FIELD_NAME": "url",
        "DATE_FORMAT": "iso-8601",
        "DATE_INPUT_FORMATS": [
            "iso-8601"
        ],
        "DATETIME_FORMAT": "iso-8601",
        "DATETIME_INPUT_FORMATS": [
            "iso-8601"
        ],
        "TIME_FORMAT": "iso-8601",
        "TIME_INPUT_FORMATS": [
            "iso-8601"
        ],
        "UNICODE_JSON": true,
        "COMPACT_JSON": true,
        "STRICT_JSON": true,
        "COERCE_DECIMAL_TO_STRING": true,
        "UPLOADED_FILES_USE_URL": true,
        "HTML_SELECT_CUTOFF": 1000,
        "HTML_SELECT_CUTOFF_TEXT": "More than {count} items...",
        "SCHEMA_COERCE_PATH_PK": true,
        "SCHEMA_COERCE_METHOD_NAMES": {
            "retrieve": "read",
            "destroy": "delete"
        }
    },
    "import_strings": [
        "DEFAULT_RENDERER_CLASSES",
        "DEFAULT_PARSER_CLASSES",
        "DEFAULT_AUTHENTICATION_CLASSES",
        "DEFAULT_PERMISSION_CLASSES",
        "DEFAULT_THROTTLE_CLASSES",
        "DEFAULT_CONTENT_NEGOTIATION_CLASS",
        "DEFAULT_METADATA_CLASS",
        "DEFAULT_VERSIONING_CLASS",
        "DEFAULT_PAGINATION_CLASS",
        "DEFAULT_FILTER_BACKENDS",
        "DEFAULT_SCHEMA_CLASS",
        "EXCEPTION_HANDLER",
        "TEST_REQUEST_RENDERER_CLASSES",
        "UNAUTHENTICATED_USER",
        "UNAUTHENTICATED_TOKEN",
        "VIEW_NAME_FUNCTION",
        "VIEW_DESCRIPTION_FUNCTION"
    ],
    "_cached_attrs": [
        "UNICODE_JSON",
        "DEFAULT_THROTTLE_RATES",
        "DEFAULT_PAGINATION_CLASS",
        "DEFAULT_AUTHENTICATION_CLASSES",
        "DEFAULT_VERSIONING_CLASS",
        "DEFAULT_FILTER_BACKENDS",
        "DEFAULT_CONTENT_NEGOTIATION_CLASS",
        "DEFAULT_METADATA_CLASS",
        "DEFAULT_PARSER_CLASSES",
        "STRICT_JSON",
        "PAGE_SIZE",
        "DEFAULT_PERMISSION_CLASSES",
        "DEFAULT_THROTTLE_CLASSES",
        "COMPACT_JSON",
        "DEFAULT_RENDERER_CLASSES"
    ],
    "_user_settings": {
        "DEFAULT_THROTTLE_CLASSES": [
            "rest_framework.throttling.AnonRateThrottle",
            "rest_framework.throttling.UserRateThrottle"
        ],
        "DEFAULT_THROTTLE_RATES": {
            "anon": "40/min",
            "user": "60/min"
        },
        "DEFAULT_SCHEMA_CLASS": "drf_spectacular.openapi.AutoSchema",
        "DEFAULT_PAGINATION_CLASS": "rest_framework.pagination.PageNumberPagination",
        "DEFAULT_PAGINATION_ORDERS_OBJECTS": false,
        "PAGE_SIZE": 20,
        "DEFAULT_AUTHENTICATION_CLASSES": [
            "rest_framework.authentication.TokenAuthentication"
        ],
        "EXCEPTION_HANDLER": "backend.exception_handler.bad_request_logger",
        "DEFAULT_RENDERER_CLASSES": [
            "djangorestframework_camel_case.render.CamelCaseJSONRenderer",
            "djangorestframework_camel_case.render.CamelCaseBrowsableAPIRenderer"
        ],
        "DEFAULT_PARSER_CLASSES": [
            "djangorestframework_camel_case.parser.CamelCaseFormParser",
            "djangorestframework_camel_case.parser.CamelCaseMultiPartParser",
            "djangorestframework_camel_case.parser.CamelCaseJSONParser"
        ]
    },
    "UNICODE_JSON": true,
    "COMPACT_JSON": true,
    "STRICT_JSON": true,
    "DEFAULT_AUTHENTICATION_CLASSES": [
        "rest_framework.authentication.TokenAuthentication"
    ],
    "DEFAULT_PERMISSION_CLASSES": [
        "rest_framework.permissions.AllowAny"
    ],
    "DEFAULT_RENDERER_CLASSES": [
        "djangorestframework_camel_case.render.CamelCaseJSONRenderer",
        "djangorestframework_camel_case.render.CamelCaseBrowsableAPIRenderer"
    ],
    "DEFAULT_PARSER_CLASSES": [
        "djangorestframework_camel_case.parser.CamelCaseFormParser",
        "djangorestframework_camel_case.parser.CamelCaseMultiPartParser",
        "djangorestframework_camel_case.parser.CamelCaseJSONParser"
    ],
    "DEFAULT_THROTTLE_RATES": {
        "anon": "40/min",
        "user": "60/min"
    },
    "DEFAULT_THROTTLE_CLASSES": [
        "rest_framework.throttling.AnonRateThrottle",
        "rest_framework.throttling.UserRateThrottle"
    ],
    "DEFAULT_CONTENT_NEGOTIATION_CLASS": "rest_framework.negotiation.DefaultContentNegotiation",
    "DEFAULT_METADATA_CLASS": "rest_framework.metadata.SimpleMetadata",
    "DEFAULT_VERSIONING_CLASS": null,
    "DEFAULT_FILTER_BACKENDS": [],
    "PAGE_SIZE": 20,
    "DEFAULT_PAGINATION_CLASS": "rest_framework.pagination.PageNumberPagination"
}

@cquinn540, excellent observation! It got me thinking about the reasons behind this and the implications it might have.

@andrewtavis, I’d recommend implementing that change and cleaning up some of the views. The TokenAuthentication, Pagination, and other features will seamlessly use the default settings, streamlining the configuration overall.

@cquinn540
Copy link
Collaborator

The short answer is those 4 lines cause lot of initialization code to be run before the settings in settings.py are defined.

Figuring out the exact effects of each line is pretty complicated because the settings are lazily loaded when they are first used. Since some settings are used to set class-level variables, their values are frozen at import time despite the lazy loading.

@andrewtavis
Copy link
Member Author

Alright, we're good here on my end as well now 😊 Thanks so much for the solution here, @cquinn540! 🚀

@andrewtavis andrewtavis merged commit 33b22fa into main Nov 13, 2024
6 checks passed
@andrewtavis andrewtavis deleted the backend-case-output branch November 13, 2024 20:30
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.

3 participants