dwyl / mvp

πŸ“² simplest version of the @dwyl app
https://mvp.fly.dev
GNU General Public License v2.0
87 stars 2 forks source link

EPIC: Basic `REST API` #256

Open nelsonic opened 1 year ago

nelsonic commented 1 year ago

As discussed in: https://github.com/dwyl/app/issues/273 having a REST and WebSocket API is a Top Priority for us. πŸ” We want anyone to be able to securely query their data using the API and an AUTH_API_KEY or JWT; this is our API Roadmap. For now we just need something super basic so that we can get moving on the Flutter MVP: https://github.com/dwyl/product-roadmap/issues/40

Todo

We are breaking down this EPIC into 3 stages, an attempt chunk the work so that we can ship each stage as fast as possible. We expect there to be 3 separate Pull Requests; one for each stage.

Rather than extending the already very long BUILDIT.md file, which is 75 pages at the latest viewing:

buildit-log-is-75-pages

We will instead:

Stage 1: No Auth πŸ“–

You'll notice that we are not doing tags in the first stage; this is deliberate because there's quite a lot going on in the tags and we want to get the first

!important

The purpose of this first stage is to ensure we have a good foundation for testing the API. That means having automated:

Stage 2: Tags 🏷️

Creating tags and associating them to items is a bit more involved because it can require multiple API requests if we follow traditional REST conventions. We should definitely do that for completeness. But for UX we should also allow people to create an item and specify the tags in a single API request. πŸ’­

Next:

This is definitely a denormalisation and potentially "frowned upon" by some REST purists. But we don't care. We only care about UX. If the person using the API can make fewer requests and get their data faster, we are happy. 😊

Stage 3: With Authentication! πŸ”

Once the Basic REST API is working [deployed!] we will allow people to view a JWT within the App which they can use to make authenticated API Requests. There are a few extra steps to enable this so as soon as the previous 2 stages are complete (PRs merged & deployed) we will re-visit and expand on this stage. πŸ‘Œ

Note: What we will eventually do is have an api.dwyl.com similar to how GitHub does it: https://docs.github.com/en/rest/overview/resources-in-the-rest-api so the API will run on a completely different server/service

@LuchoTurtle please take a look at this and LMK if it's clear enough for you to start work on it today. πŸ™

LuchoTurtle commented 1 year ago

Just an update on this. Since :authOptional has to be used (according to what is said in the issue), it's giving me lots of problems because of sessions (that don't exist when calling an API anonymously). Making a simple POST request is yielding me the following error:

session not fetched, call fetch_session/2

This error occurs in auth_plug and I've located where. I'm just thinking of how I should change this code so it doesn't break the core functionality of the package and doesn't fetch_session two times needlessly. The error is in get_jwt, when calling get_session..

It's weird, this error shouldn't be happening according to the Conn.Plug's source code, since the :jwt atom is being passed.

This could easily be surpassed by. just creating the authOptional pipeline with a plug:fetch_session, like so:

  pipeline :authOptional do
    plug :fetch_session
    plug(AuthPlugOptional)
  end

However, I feel like auth_plug should gracefully handle these scenarios. I won't create any PRs because it is out of the scope of this, but I created an issue for discussion -> https://github.com/dwyl/auth_plug/issues/97#issue-1529339416.

nelsonic commented 1 year ago

@LuchoTurtle in this instance a PR with tests is very welcome. πŸ™

nelsonic commented 1 year ago

Just attempted:

curl -X POST http://localhost:4000/api/items/1/timers -H 'Content-Type: application/json'

Got:

{"code":400,"errors":{"start":["can't be blank"]},"message":"Malformed request"}

This is not the desired behaviour. We want a timer to be started on the server without having to specify the timer.start. Apologies for not making that clear above. But that is the desired action. We also want the ability to specify the timer.start in order to control the start time. But the default response should not be a 404 if the timer.start is not set by the API requestor.

nelsonic commented 1 year ago

While testing the API using cURL on my localhost I noted that the 404 page for /api routes is HTML ... Please see: https://github.com/dwyl/mvp/issues/269

nelsonic commented 1 year ago

At present the API route to stop a timer is: /api/items/:item_id/timers/:id It should be /api/timers/:id I shouldn't need to specify the :item_id

LuchoTurtle commented 1 year ago

Regarding the tag stage 2, when creating an item with tags, doesn't it make sense and less prone to errors if tags are passed as an array instead of a comma-separated string?

{"text": "my awesome update", "tags": ["awesome", "priority-1"]}

instead of

{"text": "my awesome update", "tags": "awesome, priority-1"}
nelsonic commented 1 year ago

Side Quest: #278

nelsonic commented 1 year ago

@LuchoTurtle this is what I had mentioned for API docs: https://github.com/Doctave/doctave πŸ’­

LuchoTurtle commented 1 year ago

Would it make sense to use it for our API docs? Or do you want to battle-test it first?

nelsonic commented 1 year ago

Fairly confident that doctave will work for our needs. πŸ’­ The docs/tutorial seems pretty straightforward: https://cli.doctave.com/tutorial

question is: will we get API docs by using doctave or will we still need to use a tool like: https://github.com/open-api-spex/open_api_spex/blob/v3.16.1/README.md

LuchoTurtle commented 1 year ago

Should this issue be closed? A basic API Rest is already implemented (albeit without auth).

nelsonic commented 1 year ago

@LuchoTurtle as much as I would love to close this, auth is what makes this useful.