Retrieve a Note by its id #57

jhodapp · 2024-07-27T17:22:54Z

Description

This PR adds the ability to retrieve a Note by its id.

GitHub Issue: None

Changes

Retrieve a Note by its id
Make sure to return an error when an invalid id is specified

Testing Strategy

Using RAPIdoc, try and retrieve an existing Note by its Id (look in the DB for an existing Note's id to test)
Observe that a 200 is returned with a Note JSON object
Try entering a valid Id but of a Note that doesn't yet exist
Observe that a 204 is returned and No Content

Concerns

None

… no content

…he console

qafui

Nice work!

calebbourg

This looks great! Some of the changes bring up some great points about our conventions that might be good to discuss at this point.

I took a look at the JSON API specification for fetching resources and it outlines as convention:

Always returning an HTTP 200 and an empty list from endpoints that return collections when the collection is empty
Always returning an HTTP 200 and NULL from endpoints that return a single record when the record doesn't exist.

I think that adopting these standards makes sense considering their source as well as allowing us to simplify the BE code here and remove some of the nesting of match and other conditionals.

This makes me think we might want to update the code in our web crate to convert the RecordNotFound error to a 200 {"data": NULL}

web/src/controller/note_controller.rs

calebbourg · 2024-08-02T10:56:47Z

web/src/controller/note_controller.rs

@@ -37,7 +37,7 @@ pub async fn create(
    // TODO: create a new Extractor to authorize the user to access
    // the data requested
    State(app_state): State<AppState>,
-    Json(note_model): Json<Model>,
+    Json(note_model): Json<notes::Model>,


One of my plans in the back of my head is to go back through and clean up all of the imports and define some standard for the project.

That's a good idea, whichever it is we just desire consistency and I think highest (and quickest) readability, meaning with once glance you know which Model this is.

calebbourg · 2024-08-02T11:06:56Z

entity_api/src/note.rs

@@ -79,7 +104,27 @@ pub async fn find_by(
        }
    }

-    Ok(query.all(db).await?)
+    match query.all(db).await {


Disclaimer: This is from own standards in my brain which I never put on paper so take it with a grain of salt.

It was an intentional decision to return the empty collection when a query that results in a WHERE SQL clause returns 0 records. This is conventional in other ORMs that I've used.

RecordNotFound should only be returned for queries that result in a SELECT SQL statement.

The reason being is exactly so that we didn't have to do the extra check like !notes.is_empty()

I'm curious what the reasoning here is and if we could provide the same benefits without the extra nesting and conditional logic.

@calebbourg You don't get an error for the empty case, so you'd always get a 200 OK which is not correct, the record isn't found. So that second match branch, i.e. Err(err) does not occur when a Note isn't found, for example, if you provide an id of a Note that doesn't exist.

I'm proposing that we do actually want to return a 200 when no records exist for index fetches to align with the JSON API convention.

server MUST respond to a successful request to fetch a resource collection with an array of resource objects or an empty array ([]) as the response document’s primary data.

I think the philosophy is that the request itself is still "successful" in that there were no exceptions or auth issues. We just happened to return 0 records.

Reading more into the spec I think that the 204 No Content we're mapping to RecordNotFound is not semantically inline with the HTTP standard as it states.

This might be used, for example, when implementing "save and continue editing" functionality for a wiki site. In this case a PUT request would be used to save the page, and the 204 No Content response would be sent to indicate that the editor should not be replaced by some other page.

Which I read as "something was successful here but this endpoint itself doesn't return anything meaningful". The fact that adhering to the standard yields less matching and conditional logic in the code leads me to believe that seaORM may have designed their API to support these patterns.

This is all interpretation, of course, and I'm learning these standards as we go.

Yeah I hear you on this, I think this warrants more live discussion. Just as a preview of where I'm confused: returning a 200 OK for an entity that doesn't exist seems to me to be at odds with a RESTful API.

But maybe this is just a layering issue?

HTTP is part of the OSI layer 7, and technically so is JSON. But JSON sits at a higher level than HTTP so we'd be choosing to ignore what many APIs do, which is return a 404 when you put in a URI to a resource that doesn't exist. So I think we need to discuss what layer dictates the errors around entities.

Now having read the JSON API spec in this area, here's what I see it saying:

https://jsonapi.org/format/#fetching-resources-responses-404

There are 3 cases:

Return [] for a resource that does exist, e.g. http://localhost/articles/ but there are no articles yet but also the URI exists.

Return a null for an nested entity that doesn’t exist (e.g. an article that has an author that hasn’t been defined yet) under the requested resource

A request for an instance of an entity at a specific URI that doesn’t exist should return a 404

calebbourg · 2024-08-02T11:10:08Z

entity_api/src/note.rs

+                })
+            }
+        }
+        Err(err) => {


Similarly to above. when an Err() occurs from query.all(db).await, it should be propagated directly to the caller to be converted to a top level error.

calebbourg · 2024-08-02T11:20:34Z

entity_api/src/note.rs

@@ -57,6 +57,31 @@ pub async fn update(db: &DatabaseConnection, id: Id, model: Model) -> Result<Mod
    }
 }

+pub async fn find_by_id(db: &DatabaseConnection, id: Id) -> Result<Option<Model>, Error> {
+    match Entity::find_by_id(id).one(db).await {


This makes sense here I think

…a 404 Not Found error. Also make sure an empty list of Notes is returned as an empty list and not a 404 Not Found

calebbourg

Looks great!!

Retrieve a Note by its id

7e5ccba

jhodapp added the enhancement Improves existing functionality or feature label Jul 27, 2024

jhodapp added this to the 0.8 - Fully hydrate the current coaching session page milestone Jul 27, 2024

jhodapp requested a review from calebbourg July 27, 2024 17:22

jhodapp self-assigned this Jul 27, 2024

Run cargo fmt

1fa63d1

jhodapp force-pushed the fetch_note_by_id branch from 494e99c to 1fa63d1 Compare July 27, 2024 17:49

jhodapp added 3 commits July 28, 2024 15:46

Make sure to document the 204 no content response

3ae1f41

Cargo fmt

7de786f

Make sure that no notes retrieved returns an HTTP status code of 204,…

c3d16a6

… no content

jhodapp mentioned this pull request Jul 29, 2024

Introduce MVP-level coaching session Note creation/updating Jim-Hodapp-Coaching/refactor-platform-fe#25

Merged

Use error!() macro instead of debug!() to print error statements to t…

65ec8dd

…he console

qafui approved these changes Aug 1, 2024

View reviewed changes

calebbourg requested changes Aug 2, 2024

View reviewed changes

Make sure that when fetching a Note by id that doesn't exist returns …

e03376f

…a 404 Not Found error. Also make sure an empty list of Notes is returned as an empty list and not a 404 Not Found

calebbourg approved these changes Aug 2, 2024

View reviewed changes

jhodapp merged commit b295cff into main Aug 2, 2024
4 checks passed

jhodapp deleted the fetch_note_by_id branch August 2, 2024 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrieve a Note by its id #57

Retrieve a Note by its id #57

jhodapp commented Jul 27, 2024 •

edited

Loading

qafui left a comment

calebbourg left a comment

calebbourg Aug 2, 2024

jhodapp Aug 2, 2024 •

edited

Loading

calebbourg Aug 2, 2024

jhodapp Aug 2, 2024 •

edited

Loading

calebbourg Aug 2, 2024

jhodapp Aug 2, 2024

jhodapp Aug 2, 2024

calebbourg Aug 2, 2024

calebbourg Aug 2, 2024

calebbourg left a comment

Retrieve a Note by its id #57

Retrieve a Note by its id #57

Conversation

jhodapp commented Jul 27, 2024 • edited Loading

Description

GitHub Issue: None

Changes

Testing Strategy

Concerns

qafui left a comment

Choose a reason for hiding this comment

calebbourg left a comment

Choose a reason for hiding this comment

calebbourg Aug 2, 2024

Choose a reason for hiding this comment

jhodapp Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

calebbourg Aug 2, 2024

Choose a reason for hiding this comment

jhodapp Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

calebbourg Aug 2, 2024

Choose a reason for hiding this comment

jhodapp Aug 2, 2024

Choose a reason for hiding this comment

jhodapp Aug 2, 2024

Choose a reason for hiding this comment

calebbourg Aug 2, 2024

Choose a reason for hiding this comment

calebbourg Aug 2, 2024

Choose a reason for hiding this comment

calebbourg left a comment

Choose a reason for hiding this comment

jhodapp commented Jul 27, 2024 •

edited

Loading

jhodapp Aug 2, 2024 •

edited

Loading

jhodapp Aug 2, 2024 •

edited

Loading