Links specification compliance

isBatak commented 3 years ago

This is still WIP, as I need some time to write tests. I'll make it as a draft so I can refer to it in issue #243.

TODO:

[x] include links in resources (self) and relationships (self and related)
[x] move baseUrl to LinkBuilder inside serializer
[x] pagination links
[x] disable links per resource
[ ] add omitResourceIdentifiers option to relationship (renamed to alwaysIncludeLinkageData) WIP
[ ] ability to disable links globally
[ ] implement links routes
[ ] support meta in links
[ ] fix all TODOs
[ ] fix and write more tests
[ ] write docs

isBatak commented 3 years ago

@joelalejandro Current progress:

Top-level document links
Relationships links
Computed resource meta in the processor. Useful for exposing some read-only attributes. In my opinion createdAt, updatedAt and similar should be exposed in meta so no one can update them manually... but maybe I'm wrong so I used stats as an example. This is something to discuss.

isBatak commented 3 years ago

@joelalejandro @spersico Why is id optional in the Resource definition? https://github.com/kurierjs/kurier/blob/master/src/resource.ts#L14

I'm having a problem with this definition inside application.serializeResources method optional_id I'm expecting to have id on the resource, is there any edge-case where id is undefined?

joelalejandro commented 3 years ago

@isBatak id can be undefined when you're POSTing a new resource to the API and you're using database-generated IDs. In the context of links, it's safe to assume that you'll always have an id, so you can cast it to string.

joelalejandro commented 3 years ago

Hey @isBatak, just a heads-up: #279 is being merged soon, and it'll upgrade all dependencies in the project. It might bring some conflicts because Knex changed the way its exports work, so you can expect to see a lot of stuff like this:

- import * as Knex from "knex";
+ import { Knex, knex } from "knex";

where uppercase-Knex is the namespace and lowercase-knex is the knex instance.

isBatak commented 3 years ago

@joelalejandro now when we have links "almost" finished should we consider removing linkages on relationships if URL doesn't include specific relationship. For example, article has many comments

GET /article/1

{
  "data": {
    "id": "1",
    "type": "article",
    "attributes": {
      "body": ""
    },
    "relationships": {
      "comments": {
        "links": {
          "self": "http://localhost:3000/api/article/1/relationships/comments",
          "related": "http://localhost:3000/api/article/1/comments"
        },
        "data": [
          {
            "id": 1,
            "type": "comment"
          },
          {
            "id": 2,
            "type": "comment"
          },
          {
            "id": 3,
            "type": "comment"
          },
          {
            "id": 4,
            "type": "comment"
          },
          {
            "id": 5,
            "type": "comment"
          }
          // ..
        ]
      }
    },
    "links": {
      "self": "http://localhost:3000/api/article/1"
    }
  }
}

Should we somehow omit relationships data resource identifiers for both toOne or toMany relationships if compound document isn't requested (request URL doesn't include). This will potentially speed up the response because operation doesn't have to get the comments from DB to build linkage.

To get resource identifiers later on, we can fetch relationship self link and make a connection (this should be handled by client lib) From docs

When fetched successfully, this link returns the linkage for the related resources as its primary data. (See Fetching Relationships.)

To get actual resource data we can use related link. The point is that we can cache those links and make response even faster.

I'm thinking that we can opt-in to this behaviour in Resource relationships settings similar to this plugin https://github.com/kurierjs/kurier-addon-auto-include Something like:

import { Resource } from "kurier";
import Comment from "./comment";

export default class Article extends Resource {
  static schema = {
    attributes: {
      body: String,
    },
    relationships: {
      comments: {
        type: () => Comment,
        foreignKeyName: "comment_id",
        hasMany: true,
        omitResourceIdentifiers: true, // this could be false by default
      },
    },
  };
}

And in this case response will be:

GET /article/1

{
  "data": {
    "id": "1",
    "type": "article",
    "attributes": {
      "body": ""
    },
    "relationships": {
      "comments": {
        "links": {
          "self": "http://localhost:3000/api/article/1/relationships/comments",
          "related": "http://localhost:3000/api/article/1/comments"
        },
      }
    },
    "links": {
      "self": "http://localhost:3000/api/article/1"
    }
  }
}

and if we request Compound Document

GET /article/1?include=comments

{
  "data": {
    "id": "1",
    "type": "article",
    "attributes": {
      "body": ""
    },
    "relationships": {
      "comments": {
        "links": {
          "self": "http://localhost:3000/api/article/1/relationships/comments",
          "related": "http://localhost:3000/api/article/1/comments"
        },
        "data": [
          {
            "id": 1,
            "type": "comment"
          }
        ]
      }
    },
    "included": [{
      "type": "comment",
      "id": "1",
      "attributes": { ... }.
      "links": {
          "self": "http://localhost:3000/api/comment/1"
       },
    } ],
    "links": {
      "self": "http://localhost:3000/api/article/1"
    }
  }
}

joelalejandro commented 3 years ago

Sounds great @isBatak, go ahead!

joelalejandro commented 3 years ago

Hey @isBatak, just wanted to let you know you're doing a great job here <3 Sorry for the little-to-no-support these past few weeks, I've been busy getting stuff done at @somoscodear for our conference, WebConf LATAM 2021. Hopefully things will get calmer on September.

Keep up the great, great work!

isBatak commented 3 years ago

@joelalejandro @spersico I'm having a hard time merging main to this branch because of these changes https://github.com/kurierjs/kurier/commit/acf7a7491b28fcdd1a84d6233b2af5b418d162ca and https://github.com/kurierjs/kurier/commit/1bd6672f971561b15c82ca8fe365264fe738b826 ... there is a lot of formatting changes which are in conflict with my changed code. Can I assume that nothing logical is changed and just accept my current changes and run formatting again on my codebase npm run lint:fix?

spersico commented 3 years ago

@joelalejandro @spersico I'm having a hard time merging main to this branch because of these changes acf7a74 and 1bd6672 ... there is a lot of formatting changes which are in conflict with my changed code. Can I assume that nothing logical is changed and just accept my current changes and run formatting again on my codebase npm run lint:fix?

@isBatak hey there!

Sorry for giving you a bad time on the merge 😅. Those two commits indeed only lint and update some dependencies, don't worry. Bear in mind that the other commits do indeed have logical changes (for example the nested included resources fix that I've pushed, and better errors that @joelalejandro added)

isBatak commented 3 years ago

Hi @spersico 👋 It's not such a big deal, just want to be sure I can assume some things. I looked at other commits, they are smaller and isolated. But, for those two commits I'm not sure if there is any hidden change, that I could miss, if I jus do npm run lint:fix on my own codebase.

spersico commented 3 years ago

Hi @spersico 👋 It's not such a big deal, just want to be sure I can assume some things. I looked at other commits, they are smaller and isolated. But, for those two commits I'm not sure if there is any hidden change, that I could miss, if I jus do npm run lint:fix on my own codebase.

Yeah, I think you can safely assume that, except for the package.json, it's all linting.

joelalejandro commented 2 years ago

Putting this PR on hold, since it hasn't had any activity in the past 6 months.

kurierjs / kurier

Links specification compliance #275