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

Documentation update & Broken for relationships #777

Closed
denizderman opened this issue Oct 20, 2022 · 3 comments
Closed

Documentation update & Broken for relationships #777

denizderman opened this issue Oct 20, 2022 · 3 comments

Comments

@denizderman
Copy link

denizderman commented Oct 20, 2022

Hello,
thank you for this library. One of the most intuitive one for the JSON api. I was wondering if there is any chance to update the documentation? I think the documentation is a little bit out of date. For example:

Example 1:
(doc)

api.create('posts', {
  content: 'Hello World',
  targetUser: {
    id: '42603',
    type: 'users'
  },
  user: {
    id: '42603',
    type: 'users'
  }
})

POST https://example.test/api/posts

{
  "data": {
    "type": "posts",
    "attributes": {
      "content": "Hello World",
      "targetUser": {
        "id": "42603",
        "type": "users"
      },
      "user": {
        "id": "42603",
        "type": "users"
      }
    }
  }
}

(In here as well: https://github.com/wopian/kitsu/blob/master/packages/kitsu/example/basic.js)

(correct)

api.create('posts', {
  content: 'Hello World',
  targetUser: {
    data: {
      id: '42603',
      type: 'users'
    }
  },
  user: {
    data: {
      id: '42603',
      type: 'users'
    }
  }
});

POST https://example.test/api/posts

{
  "data": {
    "type": "posts",
    "attributes": {
      "content": "Hello World"
    },
    "relationships": {
      "targetUser": {
        "data": {
          "id": "42603",
          "type": "users"
        }
      },
      "user": {
        "data": {
          "id": "42603",
          "type": "users"
        }
      }
    }
  }
}

Example 2:
(broken for relationships)
api.delete('posts/123/relationships/comments', 321);

DELETE https://example.test/api/posts/123/relationships/comments/321

{
  "data": {
    "type": "comments",
    "id": "321"
  }
}

(working)
api.delete('posts/123/relationships/comments', [321]);

DELETE https://example.test/api/posts/123/relationships/comments

{
  "data": [{
    "type": "comments",
    "id": "321"
  }]
}

In this case, could you please clarify if the [1,2] version needs to be used to delete from relationships rather than only using the “bulk extension”?

Additionally, adding a to-many relationship example can be useful:

api.create('posts', {
  content: 'Hello World',
  user: {
    data: {
      id: '42603',
      type: 'users'
    }
  },
  tags: {
    data: [
      {
        id: '13985',
        type: 'tags'
      },
      {
        id: '92383',
        type: 'tags'
      }
    ]
  }
});

I hope my examples are explanatory. Thanks again for this library.

@denizderman denizderman changed the title Documentation Update & Broken for relationships Documentation update & Broken for relationships Oct 20, 2022
@pedep
Copy link
Contributor

pedep commented Oct 27, 2022

Your first example looks to be a simple mistake in the docs, which #782 will fix

As for example 2, it seems you encounter an issue when trying to delete a single link in a to-many relationship?
If i'm to interpret the specification, it seems like to-many relationships must supply an array when issuing a DELETE request:

https://jsonapi.org/format/1.2/#crud-updating-to-many-relationships

If the client makes a DELETE request to a URL from a relationship link the server MUST delete the specified members from the relationship

Going by the wording, i would say its reasonable to expect the users of Kitsu to supply the argument for api.delete as an array, rather than implementing logic for checking the supplied URL and performing modifications to the body.

I'm happy to PR the changes to the documentation to make this clearer, or add the logic to the delete implementation, but i think @wopian should have final say in whether its within kitsu's scope to include this logic.

@wopian
Copy link
Owner

wopian commented Oct 27, 2022

As for example 2, it seems you encounter an issue when trying to delete a single link in a to-many relationship?
If i'm to interpret the specification, it seems like to-many relationships must supply an array when issuing a DELETE request:

https://jsonapi.org/format/1.2/#crud-updating-to-many-relationships

the client makes a DELETE request to a URL from a relationship link the server MUST delete the specified members from the relationship

Going by the wording, i would say its reasonable to expect the users of Kitsu to supply the argument for api.delete as an array, rather than implementing logic for checking the supplied URL and performing modifications to the body.

I'm happy to PR the changes to the documentation to make this clearer, or add the logic to the delete implementation, but i think @wopian should have final say in whether its within kitsu's scope to include this logic.

You're correct here, which was likely an oversight from me when initialy writing the documentation - the PATCH documentation has its respective equivalents already.

The documentation for api.delete(resource, [123]) should really have had two documentation entries instead of just:

Remove multiple resources (API must support the Bulk Extension)

If you would like to add a PR that adds the following example, that would be great 👍

Remove relationship(s) from a resource
api.delete('posts/relationships/authors', [ 1, 2 ])

I would perhaps put it above the Bulk Extension delete example as that appears to still be an extension-only behaviour in JSON:API 1.1 too.

@wopian wopian closed this as completed in f6e53cb Oct 30, 2022
@wopian
Copy link
Owner

wopian commented Oct 30, 2022

Documentation updated in 10.0.3 (2022-10-30)

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

No branches or pull requests

3 participants