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

differs correctly between meta properties and attribute propertie… #1049

Closed
wants to merge 1 commit into from
Closed

differs correctly between meta properties and attribute propertie… #1049

wants to merge 1 commit into from

Conversation

jokiefer
Copy link
Contributor

@jokiefer jokiefer commented Feb 7, 2022

…s in component schemas.

Fixes #

Description of the Change

Checklist

  • PR only contains one change (considered splitting up PR)
    - [ ] unit-test added
    - [ ] documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

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)

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,
            }

@sliverc
Copy link
Member

sliverc commented Apr 26, 2022

@jokiefer Wanted to follow up whether you are still interested in working on this PR?

@jokiefer jokiefer closed this by deleting the head repository Jun 29, 2023
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.

None yet

3 participants