Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

Scope #1

Closed
maennchen opened this issue Dec 27, 2018 · 10 comments
Closed

Scope #1

maennchen opened this issue Dec 27, 2018 · 10 comments
Labels
help wanted Extra attention is needed
Milestone

Comments

@maennchen
Copy link
Contributor

This ticket is supposed to be the base for the discussion of the scope of V1. Please write here if you've questions/ideas/improvements.

@maennchen maennchen self-assigned this Dec 27, 2018
@maennchen maennchen changed the title V1 - Discussion Scope Dec 27, 2018
@maennchen maennchen added this to the V1 milestone Dec 27, 2018
@maennchen maennchen added the help wanted Extra attention is needed label Dec 29, 2018
@maennchen maennchen removed their assignment Dec 29, 2018
@greysteil
Copy link

Nice work! A few quick thoughts from playing around with the REST API:

  • revisions is a great idea
  • it should be possible to omit all params for list views and still get back some data (probably the first 10 entries)
  • using edges and returning an array of node objects doesn't feel like a normal JSON API. If you're keener on using GraphQL maybe just remove the REST option? Otherwise I'd suggest using JSON API format (looks like there's a decent package for it here).

(I generally prefer REST APIs for situations like this, where there aren't that many related objects, but no strong feelings.)

One thing I know folks are going to want - webhooks.

@maennchen
Copy link
Contributor Author

@greysteil Thanks for your feedback.

  • I could add a default for first if neither first or last is present.
  • The edges / node construct is there to enanle cursor based pagination. I don‘t know a way to lake this nicer without removing this capability. Any suggestions?
  • I used graphql because it allows to request any amount of detail specifically and because it support subscriptions.

I thought about webhooks but I wanted to start without since it can also be achieved with subscriptions and requires no db / auth etc. I would add webhooks to the roadmap for later.

@greysteil
Copy link

greysteil commented Jan 3, 2019

I could add a default for first if neither first or last is present.

Default makes sense to me!

The edges / node construct is there to enanle cursor based pagination. I don‘t know a way to lake this nicer without removing this capability. Any suggestions?

Interesting. JSON API might be a bit heavyweight here (there's details of its approach here). I don't have super strong feelings here, just struck me as very GraphQL-y for a REST API. What I would initially expect would be for the resource IDs to be the cursors - so in the case of https://elixir-security-advisory.gigalixirapp.com/v1/packages/UGFja2FnZTplY3Rv/vulnerabilities?first=5 I'd expect VnVsbmVyYWJpbGl0eTpwYWNrYWdlcy9lY3RvLzIwMTctMDgtMjcueW1s to be the cursor, and for the structure to be something like:

{
  "data": [
    {
      "cve": null,
      "id": "VnVsbmVyYWJpbGl0eTpwYWNrYWdlcy9lY3RvLzIwMTctMDgtMjcueW1s",
      etc.
    },
    { ... }
  ]
  "meta": {
    "endCursor": "VnVsbmVyYWJpbGl0eTpwYWNrYWdlcy9lY3RvLzIwMTctMDgtMjcueW1s",
    "hasNextPage": false,
    "hasPreviousPage": false,
    "startCursor": "VnVsbmVyYWJpbGl0eTpwYWNrYWdlcy9lY3RvLzIwMTctMDgtMjcueW1s"
  }
}

I thought about webhooks but I wanted to start without since it can also be achieved with subscriptions and requires no db / auth etc.

Makes sense! Hadn't thought about the need for a DB.

@maennchen
Copy link
Contributor Author

@greysteil A specific cursor field can transport much more than an id. If we would, for example, introduce sorting, depending on the value, the cursor could store other data. Also if we would use a database cursor like the one from postgres in the future, the cursor identification could also be stored in there. Therefore I'd like to keep the cursor field.

I think it makes sense to do some more normalization for the rest API like the data / meta fields you wrote in your example.

I'd like to implement the following based on your inputs:

  • Use JSON API Specification for the REST API.
  • Default Pagination Parameters (only for REST, keep GraphQL as is)

For v1.1:

  • Plan for a WebHook strategy. We would need the following:

Things I'm not happy with:

  • The ID of the vulnerabilities is based on its path in the Git repo.

If the path changes, the ID changes. We have two options on how to proceed:

  • Change the ID to something else (CVE ID / newly introduced UUID / suggestions?)
  • Make ID changes part of the design. We could save the old id and in case it gets requested, return an object directing you to the new ID. For REST, this would be a 301.

@greysteil
Copy link

Sounds great! On IDs, let's go for UUID with advice on how to generate them - I'll update the DB.

@maennchen
Copy link
Contributor Author

@greysteil Great!

Could you also add a check to the CI to prevent duplicate ID's? Otherwise, we could have problems if people copy them from another vulnerability.

@greysteil
Copy link

Done! And have added a check for uniqueness and UUID format.

@maennchen
Copy link
Contributor Author

Great, thanks :)

May I steal the logo of the data repo for the organisation of this project?

@greysteil
Copy link

Of course!

@maennchen
Copy link
Contributor Author

Closing ticket in favor of newly created tickets #14 #13 #12 #11

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants