Skip to content

Conversation

@arttuperala
Copy link
Contributor

Description of the Change

Add additionalProperties: true to the resource schema's attributes and relationships objects to document the fact that these objects will contain keys.

  components:
    schemas:
      resource:
        type: object
        required:
        - type
        - id
        additionalProperties: false
        properties:
          type:
            $ref: '#/components/schemas/type'
          id:
            $ref: '#/components/schemas/id'
          attributes:
            type: object
+           additionalProperties: true
          relationships:
            type: object
+           additionalProperties: true
          links:
            $ref: '#/components/schemas/links'
          meta:
            $ref: '#/components/schemas/meta'

Discovered during an OpenAPI schema validation run as the included array was found to contain too many keys in those objects.

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

@sliverc
Copy link
Member

sliverc commented Jul 3, 2023

I am not an expert on this, but is the additionalProperties really needed if the schema defines all possible properties of a object, like attributes and relationships?

@arttuperala
Copy link
Contributor Author

The schema doesn't define all of the possible properties inside the attributes and relationships.

The included is defined to be an array of resource objects.

included:
  type: array
  uniqueItems: true
  items:
    $ref: '#/components/schemas/resource'

And the resource object simply says that there exists an attributes object and a relationships object inside it. However, no actual properties are defined for those two objects and since there's no additionalProperties, there's no indication that these two objects would contain additional keys.

resource:
  type: object
  required:
  - type
  - id
  additionalProperties: false
  properties:
    type:
      $ref: '#/components/schemas/type'
    id:
      $ref: '#/components/schemas/id'
    attributes:
      type: object
    relationships:
      type: object
    links:
      $ref: '#/components/schemas/links'
    meta:
      $ref: '#/components/schemas/meta'

So once we fed the following response to the OpenAPI schema validator, we got an error message for the key name inside the /included/0/attributes since there was no indication that there would be a key name inside that object.

{
    "data": {...},
    "included": [
        {
            "type": "User",
            "id": "1",
            "attributes": {"name": "John Doe"}
        },
        {
            "type": "User",
            "id": "2",
            "attributes": {"name": "Jane Doe"}
        }
    ]
}

@sliverc
Copy link
Member

sliverc commented Jul 4, 2023

I assume this error only exists when the resource is used in the included list? When rendering the resource in data and also when creating a resource, no additional properties should be allowed.

I think it does not make sense to use the same schema type for main resource and included because it is not the same resource. In included it is a variety of resources which we can not determine what types it will be so additional properties makes sense. In the main resource additional properties does not make sense though. I guess best would be to define what is allowed in included directly without referencing resource, what do you think?

@arttuperala
Copy link
Contributor Author

Refactored to have a completely separate scheme for included items.

This allows documenting the "attributes" and "relationships" objects to
possibly have additional properties.
Copy link
Member

@sliverc sliverc left a comment

Choose a reason for hiding this comment

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

Thanks for adapting the PR. This looks great. Merging.

@sliverc sliverc merged commit 126cd9f into django-json-api:main Jul 5, 2023
@arttuperala arttuperala deleted the resource-additional-properties branch July 6, 2023 10:26
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.

2 participants