Skip to content

differs correctly between meta properties and attribute propertie… #1049

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

differs correctly between meta properties and attribute propertie… #1049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
Note that in line with [Django REST framework policy](https://www.django-rest-framework.org/topics/release-notes/),
any parts of the framework not mentioned in the documentation should generally be considered private API, and may be subject to change.

## [Unreleased] - 2022-x-xx

### Fixed

* differs correctly between `meta` properties and `attribute` properties in component schemas.

## [5.0.0] - 2022-01-03

This release is not backwards compatible. For easy migration best upgrade first to version
Expand Down
8 changes: 7 additions & 1 deletion rest_framework_json_api/schemas/openapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,7 @@ def map_serializer(self, serializer):
required = []
attributes = {}
relationships = {}
meta = {}

for field in serializer.fields.values():
if isinstance(field, serializers.HyperlinkedIdentityField):
Expand Down Expand Up @@ -683,7 +684,12 @@ def map_serializer(self, serializer):
schema["description"] = str(field.help_text)
self.map_field_validators(field, schema)

attributes[format_field_name(field.field_name)] = schema
if field.field_name in list(
getattr(serializer.Meta, "meta_fields", list())
):
meta[format_field_name(field.field_name)] = schema
Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Nice catch this can certainly be a problem.

meta dict is never used so I guess meta fields could simply be ignored? further up with a if meta_field then continue for instance. Otherwise would need to think about changing how we represent meta in the schema in general.

Also a test which test this use case will be needed for merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i wouldn't remove it from the schema.

Example usecase:

  "components": {
    "schemas": {
      "WebMapService": {
        "type": "object",
        "properties": {
          "type": {
            "type": "string",
            "description": "The [type](https://jsonapi.org/format/#document-resource-object-identification) member is used to describe resource objects that share common attributes and relationships.",
            "enum": [
              "WebMapService"
            ]
          },
          "attributes": {
            "type": "object",
            "properties": {
            }
          },
          "relationships": {
          },
          "meta": {
            "type": "object",
            "properties": {
              "stringRepresentation": {
                "type": "string",
                "readOnly": true,
                "title": "The string representation of the object it self"
                "description": "This representation could be used to display a short title of the object"
              }
            }
          }
      }
    }

The stringRepresentation could be the string from the __str__ function of the model object. This would be usefull for several use cases, such as autocomplete widgets.

Sure, the stringRepresentation could be part of the regular attributes of the object, but i dont know if it is the best place for it.

Copy link
Member

Choose a reason for hiding this comment

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

@n2ygk You have more in-depth knowledge about openapi. What is your take on the meta issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if sparse fields are used, the meta object is still part of the response with possible null data. For that reason i would say it is better to place it in the attributes. Then it is possible to get only the stringRepresentation attribute in autocomplete cases.

Copy link
Member

Choose a reason for hiding this comment

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

This is actually a bug see #332 and needs to be fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sliverc sorry I missed this mention from 5 days ago. It looks like you figured it out?

Copy link
Member

Choose a reason for hiding this comment

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

not really... what was the reason to represent meta on a resource level as stringRepresentation? Couldn't there be a more detailed representation as with attributes as all meta fields are actually serializer fields? Or did I overlook something here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Spending all of 30 seconds looking at this, it looks like confusing Django Models' Meta attribute with the optional meta jsonapi field: https://jsonapi.org/format/#document-meta

They are not related.

Copy link
Member

Choose a reason for hiding this comment

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

It is a bit confusing that within DJA we sometimes mix up the API with DRF but this is a discussion for another time... 😄 but actually in this case meta_fields even though defined in Meta class is a DJA feature (see docs)

django-rest-framework-json-api/rest_framework_json_api/renderers.py

Line 421 in c46d495

meta_fields = getattr(meta, "meta_fields", [])
)

It works the way that serializer fields are used to build the JSON:API spec resource meta (not the top level which is build manually by overwriting get_root_meta). So as i see the resource meta class could be defined in more detail where as the root meta cannot.

Copy link
Member

Choose a reason for hiding this comment

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

As a follow up as of my explanation above in my opinion can the meta fields be assigned to the result schema further below with something like this:

        if relationships:
            result["properties"]["meta"] = {
                "type": "object",
                "properties": meta,
            }

else:
attributes[format_field_name(field.field_name)] = schema

result = {
"type": "object",
Expand Down