lamanchy / bis

0 stars 1 forks source link

API: Objects in response don't include related data #2

Closed mrkvon closed 2 years ago

mrkvon commented 2 years ago

When i fetch an event, i receive in response

{
  ...
  "location": 1893,
  "category": 5,
  "program": 2,
  "administration_units": [64],
  "main_organizer": null,
  "other_organizers": [
    17696,
    17690,
    14988,
    12873,
    2807,
    2783
  ],
  "duration": 3,
  ...
}

These fields refer related data by their numerical id, instead of serving the data as part of a response. This forces frontend apps to fetch the related data separately, and perform joins manually, to display them properly. Backend database JOIN would be more suitable for this. Is this by purpose, or is completing the data a planned feature?

Note: This affects other endpoints as well

lamanchy commented 2 years ago

I expected, that for each endpoint there would be initial fetch of data, and for each "model" a map would be created with keys as ids and values as objects. Then everything can be accessed locally.

lamanchy commented 2 years ago

I can pass whole objects instead of ids, but then it's not quite clear how to "consistently" perform updates. For example: PATCH { ... 'registration': { 'is_event_full': true } ... }

changes field of nested model EventRegistration, which is fine, but PATCH { ... 'category': { 'id': 5 } ... }

looks like you are trying to change field id, not to... change the relation. And I would have to deal with something like

PATCH { ... 'category': { 'name': 'xyz' } ... }

which is forbidden, since you cannot (obviously) change category's name.

mrkvon commented 2 years ago

there would be initial fetch of data

Do you mean that the app would pre-fetch all its needed data? Or just categories?

If yes, i have hard time imagining keeping the whole list of users in browser memory (e.g. to fill event organizers). We fetch the needed data for each page/view separately, although caching of often-used endpoints would take place, too. But pre-fetching everything is not an option, really.

for each "model" a map would be created with keys as ids and values as objects

Do you mean that we would store these pre-fetched data in an "index", and then access them as needed, to fill the necessary ids?

Regardless of whether we pre-fetch all data or fetch as needed, this is a kind of gymnastics that frontend is capable of, but not well suited for. But it's precisely what SQL database joins are for... (I found this, but that's hardly an authoritative resource)

it's not quite clear how to "consistently" perform updates

What do you mean by "consistently"? That responses have similar shape to requests?

To me personally, it doesn't matter whether we perform updates with category: { 'id': 5 }, or category: 5. But i can imagine it may matter to django-rest-framework (is that what we use here?). Personally, i've barely used it. But it's such a well known and popular library, that this may be a well-resolved problem. Generally - i'd think - fetch the full objects, update ids. Semantics can be chosen.

looks like you are trying to change field id, not to... change the relation. And I would have to deal with something like

Absolutely, i agree, that shouldn't be an allowed move.

I do get that it's more work. If you want, we can continue here or in private chat (today i have to finish though).

lamanchy commented 2 years ago

Oh, alright, I thought that there would be some kind of ORM as part of frontend, my bad.

fetch the full objects, update ids This sounds alright, I dug into that last night, it's quite ugly, but it works. But I don't really like the idea of sending duplicate data, imagine you would get list of your events, then for each event there's gonna be your whole profile into each one of them, because you organized them all. I like more the idea of:

GET /events// and GET /users/?event= - which would return all the users used in such event.

But sure, I can pass users your way. But you'll have to prefetch AdministrationUnits still, cause: user can have donor donor can support administration_unit administration_unit has board_members board_member can have donor etc...

And if you'll be prefetcing all AdministrationUnits, you can prefetch all the categories as well.

But I don't want to be bitching as much, so just tell me what you wish for and I'll oblige.

this is a kind of gymnastics that frontend is capable of, but not well suited for https://redux-orm.github.io/redux-orm/basics/fields

mrkvon commented 2 years ago

This sounds alright, I dug into that last night, it's quite ugly, but it works. But I don't really like the idea of sending duplicate data, imagine you would get list of your events, then for each event there's gonna be your whole profile into each one of them, because you organized them all. I like more the idea of:

... sorry i missed this part, because it looked like quote

GET /events// and GET /users/?event= - which would return all the users used in such event.

This sounds very much fine to me. Difficult thing would be if we had to fetch each person separately. I think the second case in term of GET /events/:eventId/participants and GET /events/:eventId/organizers, but it doesn't matter much how it's written.

But you'll have to prefetch AdministrationUnits

I don't think it's necessary to include everything that could potentially be included. We could focus on the data that we actually need in our context, and not include more (and not include less).

In my experience, this is a difficult problem with REST APIs, leading to bikeshedding (i started it a bit, too 😅 ). GraphQL and jsonapi try to resolve it, each in their own way. I guess we'll find a way, too.

mrkvon commented 2 years ago

I started this discussion, because i needed the current user, and i was missing the roles in the result. It felt unintuitive to have to pre-fetch/fetch the role separately, and still feels that way. But i wrongly communicated that we need every relational data filled everywhere... that's not the case (my bad)

Thank you @talitadzikzlasu for pointing this out to me

talitadzikzlasu commented 2 years ago

Hmm... Maybe there is confusion in "fetch the full objects"?

I think, for a list of actions (GET /events/) we don't need a list of users for each event and actually just quite limited data for each of them:

And for the singular event (GET /events//) we will need probably all of the data of the event and some additional data from the USERS table, MEALS table, etc.

So no "full objects" for GET as @mrkvon said, but particular data from particular tables joined together (so no cycles). (and for that I see we should specify which data are needed for which endpoint)

lamanchy commented 2 years ago

... sorry i missed this part, because it looked like quote

yeah, sorry, I need to learn how to properly format text in markdown, I'll try improve that.

I started this discussion, because i needed the current user, and i was missing the roles in the result. It felt unintuitive to have to pre-fetch/fetch the role separately, and still feels that way. But i wrongly communicated that we need every relational data filled everywhere... that's not the case (my bad)

Yeah, I always had and ORM when developing any frontend app, so... my intuitive view is quite opposite, but I get the difference.

I'll include all the categories and simlpe objects as nested ones, so you don't have to join them in frontend. And I'll add some filters for users API, so you can fetch relevant users for event / events.

I'll also add filter to each endpoint like: /events/?id__in=1,2,3,4.... (comma separated list of ids), so in the case you need multiple objects with specific id, you can fetch them in one request.

I think, for a list of actions (GET /events/) we don't need a list of users for each event and actually just quite limited data for each of them:

I would like to keep APIs rather more universal then specific, this would mean that frontend and API would be closely connected, and there would be a lot of "I need to add this field into this API and that field into that one".

lamanchy commented 2 years ago

Vytvořil jsem endpointy:

/api/frontend/users/{user_id}/participated_in_events/ /api/frontend/users/{user_id}/registered_in_events/ /api/frontend/users/{user_id}/events_where_was_organizer/

/api/frontend/events/{event_id}/registered/ /api/frontend/events/{event_id}/record/participants/ /api/frontend/events/{event_id}/organizers/

Do každého endpointu lze také přidat id=1,2,3...

Kategorie atd vkládám rovnou do response místo id. Při zápisu se lze řídit tímto pravidlem: Pokud vnořený objekt obsahuje pole id, je potřeba místo objektu vložit jen ono id, toho daného objektu či jiného (pokud ho chceš změnit) Pokud vnotřený objekt neobsauje pole id, čeká se, že tam vložíš celý objekt. (případně část u PATCH)

Je to takto ok?

lamanchy commented 2 years ago

Ups, that was not english, I can rewrite it, if you want me to, I just forgot, sorry.

mrkvon commented 2 years ago

Ups, that was not english

I think that's fine... 🙂

Vytvořil jsem endpointy

Sounds good, thank you for including and describing that, will test. 🙂 And thank you also for such speedy responses, and tackling this...

lamanchy commented 2 years ago

And thank you also for such speedy responses, and tackling this...

No problem, if anything, just let me know. ;)

mrkvon commented 2 years ago

Do každého endpointu lze také přidat id=1,2,3...

@lamanchy

It seems like we've lost that ability with /api/frontend/users along with the recent adding of ?search param

mrkvon commented 2 years ago

We replaced that need with GET /api/frontend/users/:id, so no worries...

lamanchy commented 2 years ago

@mrkvon it's back, sorry, I just pushed it

I added page_size param as well, when doing that.