Overhaul serializers to eliminate manually defined fields

[dhruvkb]

Description

To further streamline the process of adding new media, this PR updates serializers to almost eliminate manually defined serializer fields.

This PR

• moves help_text from serializers to models
• drops fields from serializers that can be inferred from models

Testing Instructions

Being a refactoring PR, the differences at the surface level should be minimal. To test, visit each endpoint once (sorry for the big ask) and see that the server never raises any errors or sends a 500 status code.

You can also see the changes to api/catalog/api/examples/image_responses.py and api/catalog/api/examples/audio_responses.py to see the final output changes.

Checklist

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[github-actions]

API Developer Docs Preview: Ready

https://wordpress.github.io/openverse-api/_preview/696

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

[sarayourfriend]

The clean up so far looks really nice but I wonder if we could wait to do this until we have unit tested the serializers in #557?

[dhruvkb]

@sarayourfriend since the output is not being changed, I'd recommend WordPress/openverse#724 proceed with the tests based on serializers as on main and we can update this PR to include those tests when ready.

Also there will be changes to the tests needed when #684 gets merged.

[sarayourfriend]

Okay! I was mostly trying to get around at least three of us going through each API endpoint and verifying it works locally 😅

[sarayourfriend]

Why are there new fields being returned/present on the image respones?

[sarayourfriend]

What is "accuracy"?

[obulat]

Accuracy is present on old tags that have clarifai as provider. The docstrings:

"The accuracy of a machine-generated tag. Human-generated tags do not have an accuracy field."

https://github.com/cc-archive/cccatalog-api/blob/731074ee543d50edac9aacb9aa0362d03a6bec41/cccatalog-api/cccatalog/api/serializers/image_serializers.py#L252

[sarayourfriend]

Huh. Could we clarify what accuracy means here then? Maybe such as accuracy in the case of clarifai provided tags or something? Or just not call out a specific field at all, if that's too cumbersome.

[sarayourfriend]

What is the goal of this type of comment? It seems like it just duplicates the information that's already documented declaratively in the class definition below it.

[dhruvkb]

Yes, this is helpful when you're reading a class and hover over the name of its mixins to see what fields they're adding to the model.

[sarayourfriend]

Gotcha. My concern is that because it is essentially a 1:1 mirror of the implementation that it will easily go out of date quickly (as it already had, with this line needing to be move from one serializer to another without any changes on the actual implementation of either one).

If our base mixin classes are so abstract that it's not easy to remember (or learn) what fields they provide based on the names and reading the declarative code inside their definitions, maybe it's a sign that things are too complex?

It could be I'm over thinking this though. I'm just wary of whenever we start to hand write and hand-maintain documentation that says in literal terms what the code immediately below it says.

[dhruvkb]

I don't like this either, and I'm 100% in agreement with you that the mixins are super-confusing and can be improved. If we did spend a lot of time in the API models (which we don't), we'd soon remember what field was where.

But at the moment, it's a bit hard to see a class inheriting from 4-5 mixins and having to guess which fields are coming from which mixin (or going to each of their definitions to check).

[dhruvkb]

^ That was an accident, sorry.

[dhruvkb]

Why are there new fields being returned/present on the image responses?

The goal was to ensure that the search results and the details endpoint should follow as close a schema as possible by avoiding the use of required=False. Extra fields can be ignored by the client but a serializer choosing to omit fields if they're null can be chaotic.

[sarayourfriend]

The goal was to ensure that the search results and the details endpoint should follow as close a schema as possible by avoiding the use of required=False. Extra fields can be ignored by the client but a serializer choosing to omit fields if they're null can be chaotic.

Makes sense, thanks for the explanation.

What's the testing strategy for this? I saw that the "visit each endpoint and verify it" is crossed out. But the sample responses aren't automatically generated are they? Is anything comparing those against the actual data coming from the API this PR produces?

[obulat]

I've tested the API locally and see that the issue that we saw in production is gone. It's a huge improvement, thank you for adding all the documentation in the docstrings!

[dhruvkb]

@sarayourfriend the example responses are compared by actually making a request to the API and checking if the JSON matches exactly. It tests the API by checking both search results and detail pages.

[dhruvkb]

I'm merging this to unblock API deployment. We can surely fix any smaller issues that come up in hotfixes.

[sarayourfriend]

Sounds good @dhruvkb 👍