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

Drf JSONField usage is innacurate #3657

Open
pedro-psb opened this issue Jul 2, 2024 · 2 comments
Open

Drf JSONField usage is innacurate #3657

pedro-psb opened this issue Jul 2, 2024 · 2 comments
Labels

Comments

@pedro-psb
Copy link
Member

Django-rest-framework JSONField can be any JSON primitive. In our codebase, its used in fields where we would expect specifically a Object Type or a List Type.

I'm not sure if it makes sense to update all of these, but it may be worth doing a general review. The possible benefits of this are:

  • provide more meaningful types for the user, because those things shows in the docs.
  • catch gross errors on desserialization

Some that may be more meaningful are the config ones (on repository.py, see below), where we could go further and custom object with the config keys, so it shows nicely in the REST API docs (this may be done with drf-spectacular extend_config_field).

Context

This was originally brought to my attention because of #3639, but its not directly related anymore.

Reference

This are the places where its used in pulp_rpm/app/serlializers:

advisory.py::UpdateCollectionSerializer   module = serializers.JSONField(help_text=_("Collection modular NSVCA."), allow_null=True)

comps.py::PackageGroupSerializer          packages = serializers.JSONField(help_text=_("PackageGroup package list."), allow_null=True)
comps.py::PackageGroupSerializer          desc_by_lang = serializers.JSONField(
comps.py::PackageGroupSerializer          name_by_lang = serializers.JSONField(

comps.py::PackageCategorySerializer       group_ids = serializers.JSONField(help_text=_("Category group list."), allow_null=True)
comps.py::PackageCategorySerializer       desc_by_lang = serializers.JSONField(
comps.py::PackageCategorySerializer       name_by_lang = serializers.JSONField(help_text=_("Category name by language."), allow_null=True)

comps.py::PackageEnvironmentSerializer    group_ids = serializers.JSONField(help_text=_("Environment group list."), allow_null=True)
comps.py::PackageEnvironmentSerializer    option_ids = serializers.JSONField(help_text=_("Environment option ids"), allow_null=True)
comps.py::PackageEnvironmentSerializer    desc_by_lang = serializers.JSONField(
comps.py::PackageEnvironmentSerializer    name_by_lang = serializers.JSONField(
comps.py::PackageLangpacksSerializer      matches = serializers.JSONField(help_text=_("Langpacks matches."), allow_null=True)

modulemd.py::ModulemdSerializer           artifacts = serializers.JSONField(help_text=_("Modulemd artifacts."), allow_null=True)
modulemd.py::ModulemdSerializer           dependencies = serializers.JSONField(help_text=_("Modulemd dependencies."), allow_null=True)
modulemd.py::ModulemdSerializer           profiles = serializers.JSONField(help_text=_("Modulemd profiles."), allow_null=True)
modulemd.py::ModulemdDefaultsSerializer   profiles = serializers.JSONField(help_text=_("Default profiles for modulemd streams."))

package.py::PackageSerializer             changelogs = serializers.JSONField(
package.py::PackageSerializer             files = serializers.JSONField(
package.py::PackageSerializer             requires = serializers.JSONField(
package.py::PackageSerializer             provides = serializers.JSONField(
package.py::PackageSerializer             conflicts = serializers.JSONField(
package.py::PackageSerializer             obsoletes = serializers.JSONField(
package.py::PackageSerializer             suggests = serializers.JSONField(
package.py::PackageSerializer             enhances = serializers.JSONField(
package.py::PackageSerializer             recommends = serializers.JSONField(
package.py::PackageSerializer             supplements = serializers.JSONField(

repository.py::RpmRepositorySerializer    repo_config = serializers.JSONField(
repository.py::RpmPublicationSerializer   repo_config = serializers.JSONField(
repository.py::CopySerializer             config = serializers.JSONField(
@pedro-psb
Copy link
Member Author

We should revert the fix in #3679 when these fields are correctly represented in openapi schema.

Some approaches here are:

  • creating more specific sub-serializers, which should affect runtime as we'll be adding more validation
  • using drf-spectacular extends, which I believe doesnt affect runtime

Something to keep in mind is that fixing this should modify the bindings somehow, as it will be altering the schema types. That can potentially require that bindings users adapt their code.

@ggainey
Copy link
Contributor

ggainey commented Jul 18, 2024

Changing the API == breaking the API, even if it's in a good cause. Our "breaking changes" commitment is to the plugin API - generally speaking, the user-API isn't available to be broken for anything other than "it doesn't actually work" bugs.

We can add to the API - but we need to maintain the existing behavior.

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

Successfully merging a pull request may close this issue.

3 participants