Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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.

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
jokiefer wants to merge 1 commit into django-json-api:main from jokiefer:bug/fix-schema-meta-attributes
Closed

differs correctly between meta properties and attribute propertie... #1049

jokiefer wants to merge 1 commit into django-json-api:main from jokiefer:bug/fix-schema-meta-attributes

Conversation

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

@sliverc sliverc Feb 9, 2022

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.

auvipy reacted with thumbs up emoji
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.

n2ygk reacted with eyes emoji
Copy link
Member

@sliverc sliverc Mar 9, 2022

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

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
Reviewers

@sliverc sliverc sliverc requested changes

@n2ygk n2ygk n2ygk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /