Add parentId parameter in core/front apis #9261

tdraier · 2024-12-10T16:14:50Z

Description

Adds parent_id in core and front API , in request when upserting a doc/table/folder and in document/table/folder response.
parent_id is not stored in database but we only ensure the value is equals to parents[1].

Risk

Errors when upserting documents/tables/folders. If parent_id is not passed (which is the case for now), nothing should change. Can be rolledback

Deploy Plan

deploy core
then deploy front

github-actions · 2024-12-10T16:16:35Z

	Warnings
⚠️	Files in `**/api/v1/` have been modified and the PR has the `documentation-ack` label. Don't forget to run `npm run docs` and use the `Deploy OpenAPI Docs` Github action to update https://docs.dust.tt/reference.

Generated by 🚫 dangerJS against e213f12

spolu

If the array is called parents I wonder if we should just call it parent vs parent_id ?

front/pages/api/v1/w/[wId]/spaces/[spaceId]/data_sources/[dsId]/documents/[documentId]/index.ts

spolu

Thinking more about it, I really think we want to name it parent given that the other field is named parents

.../pages/api/v1/w/[wId]/spaces/[spaceId]/data_sources/[dsId]/documents/[documentId]/parents.ts

front/pages/api/v1/w/[wId]/spaces/[spaceId]/data_sources/[dsId]/tables/index.ts

tdraier · 2024-12-11T09:33:29Z

Thinking more about it, I really think we want to name it parent given that the other field is named parents

I find this more consistent to have parent_id (or parentId) as we do for document_id / folder_id / table_id and we're more clear on the fact we're not passing (or receiving) an object. And as the other fields "parents" is going to be removed at some point I don't think we need to align on that one ?

spolu · 2024-12-11T09:53:43Z

I find this more consistent to have parent_id (or parentId) as we do for document_id / folder_id / table_id and we're more clear on the fact we're not passing (or receiving) an object. And as the other fields "parents" is going to be removed at some point I don't think we need to align on that one ?

Well Stripe does not use _id when refering to other objects (eg creating a refund for a charge here: https://docs.stripe.com/api/refunds/create)

Really think the _id is cumbersome. It's required when upserting of course but otherwise I think it's just more elegant and more usable to not have _id prefix as long as things are clearly documented as string

spolu · 2024-12-11T09:54:34Z

Other than upsert flow do we already have any _id parameters?

spolu · 2024-12-11T09:55:46Z

We do have fileId on content fragment creation :/

spolu · 2024-12-11T09:56:51Z

Even for table upsert this should be a post to /tabe/{tableId} not a post to /tables :/ this API is a mess

spolu · 2024-12-11T09:57:57Z

Let's make sure to have a proper upsert path for folders

That way we can standardize on create something and you're given an id: POST .../thing
Upsert POST .../thing/:id
Refering to thing in another request: body { thing: string }

philipperolet · 2024-12-11T10:09:33Z

IMO parent_id is more suited here:

consistent with node_id
parents will disappear from api long term, so consistency should go with other fields
in general, having thing: refer to a sub-object (with other fields) and thing_id to refer to a string is IMO good; otherwise when you see an arg thing: you'll wonder whether it's a string id, or another object.

I think when I say "thing" I refer to the whole thing, and when I say "thing_id" I refer to its id is the most sensible and the convergence point we should aim at for consitency in the API (and in naming in general)

Are you strong on removing _id @spolu ?

tdraier · 2024-12-11T10:21:53Z

Even for table upsert this should be a post to /tabe/{tableId} not a post to /tables :/ this API is a mess

💯
In front, only tables uses body for passing id - actually in 2 different endpoint, either /tables but also /tables/csv . We should clearly use /tables/[tId] and handle here both csv/table - i think it's perfectly fine to add a post on /tables/[tId] and deprecate the 2 other endpoints

spolu · 2024-12-11T10:22:13Z

as per IRL stong feeling from @philipperolet to keep parent_id. Fine to go with it under the argument that we already have fileId

philipperolet · 2024-12-11T10:27:23Z

as per IRL stong feeling from @philipperolet to keep parent_id. Fine to go with it under the argument that we already have fileId

Thanks; I'd be ok to take renaming parents to parent_ids if that would make this choice more reasonable to you. The API v1 users that actually use this field apart from ourselves are close to 0 IMO.

philipperolet

Left a batch of coms 👍

core/bin/core_api.rs

front/pages/api/v1/w/[wId]/spaces/[spaceId]/data_sources/[dsId]/tables/index.ts

core/src/stores/migrations/20241210_nodes_parent.sql

connectors/src/lib/data_sources.ts

spolu · 2024-12-11T11:00:48Z

core/src/data_sources/data_source.rs

@@ -686,7 +691,8 @@ impl DataSource {
            title.as_deref().unwrap_or(document_id),
            mime_type.as_deref().unwrap_or("application/octet-stream"),
            &tags,
-            &parents,
+            parents.get(1).cloned(),


This one seems off?

spolu · 2024-12-11T11:00:58Z

core/src/data_sources/data_source.rs

@@ -142,6 +142,7 @@ pub struct Document {
    pub title: String,
    pub mime_type: String,
    pub tags: Vec<String>,
+    pub parent_id: Option<String>,


why diff this file for now?

don't we want to return the parent_id here ?

seen irl with @philipperolet , to be consistent we want to add parent_id in return too

spolu · 2024-12-11T11:01:16Z

core/src/data_sources/folder.rs

@@ -6,6 +6,7 @@ pub struct Folder {
    folder_id: String,
    timestamp: u64,
    title: String,
+    parent_id: Option<String>,


why diff here for now?

core/src/stores/migrations/20241210_nodes_parent.sql

philipperolet

Good for me 👍
Left a few coms, not requiring changes (but check them out :) )

core/bin/core_api.rs

front/lib/api/data_sources.ts

tdraier added 2 commits December 10, 2024 17:07

Add parent_id in core api

b73ddc0

Add parentId in public api

c475fbd

tdraier added 2 commits December 10, 2024 17:59

fix compilation for connectors

e00137f

update doc

daef1bf

tdraier added the documentation-ack Used to acknowledge that documentation is up-to-date with this PR label Dec 10, 2024

tdraier marked this pull request as ready for review December 10, 2024 17:31

spolu reviewed Dec 10, 2024

View reviewed changes

front/pages/api/v1/w/[wId]/spaces/[spaceId]/data_sources/[dsId]/documents/[documentId]/index.ts Outdated Show resolved Hide resolved

tdraier added 3 commits December 11, 2024 09:30

api consistency

57d0114

prefer snake_case for new apis

6c39ed0

fixes for parent_id

58936b2

spolu reviewed Dec 11, 2024

View reviewed changes

.../pages/api/v1/w/[wId]/spaces/[spaceId]/data_sources/[dsId]/documents/[documentId]/parents.ts Outdated Show resolved Hide resolved

front/pages/api/v1/w/[wId]/spaces/[spaceId]/data_sources/[dsId]/tables/index.ts Show resolved Hide resolved

Merge branch 'main' into thomas/parent-id

1c1b1e1

tdraier added 2 commits December 11, 2024 10:35

clean comment

74a0a6e

Update doc

8c8595a

Enfore parent to be valid

b961286

philipperolet reviewed Dec 11, 2024

View reviewed changes

spolu reviewed Dec 11, 2024

View reviewed changes

tdraier added 2 commits December 11, 2024 12:08

clean

df84662

copy

29bdc91

tdraier added 4 commits December 11, 2024 13:23

use refs

654c4b8

Update comments

0ab9dbc

error handling

0a32210

docs

ee57b59

philipperolet approved these changes Dec 11, 2024

View reviewed changes

core/bin/core_api.rs Show resolved Hide resolved

core/bin/core_api.rs Show resolved Hide resolved

front/lib/api/data_sources.ts Show resolved Hide resolved

tdraier added 5 commits December 11, 2024 15:53

add parent_id in returns

463f7e7

pass parentId param from parents array

401bd03

Merge branch 'main' into thomas/parent-id

9582189

enforce parentid value in folder

4cdc2ec

lint

e213f12

tdraier merged commit 5e2bd9c into main Dec 11, 2024
4 checks passed

tdraier deleted the thomas/parent-id branch December 11, 2024 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parentId parameter in core/front apis #9261

Add parentId parameter in core/front apis #9261

tdraier commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading

spolu left a comment

spolu left a comment

tdraier commented Dec 11, 2024

spolu commented Dec 11, 2024

spolu commented Dec 11, 2024

spolu commented Dec 11, 2024

spolu commented Dec 11, 2024

spolu commented Dec 11, 2024

philipperolet commented Dec 11, 2024

tdraier commented Dec 11, 2024

spolu commented Dec 11, 2024

philipperolet commented Dec 11, 2024

philipperolet left a comment

spolu Dec 11, 2024

spolu Dec 11, 2024

tdraier Dec 11, 2024

tdraier Dec 11, 2024

spolu Dec 11, 2024

philipperolet left a comment

Add parentId parameter in core/front apis #9261

Add parentId parameter in core/front apis #9261

Conversation

tdraier commented Dec 10, 2024 • edited Loading

Description

Risk

Deploy Plan

github-actions bot commented Dec 10, 2024 • edited Loading

spolu left a comment

Choose a reason for hiding this comment

spolu left a comment

Choose a reason for hiding this comment

tdraier commented Dec 11, 2024

spolu commented Dec 11, 2024

spolu commented Dec 11, 2024

spolu commented Dec 11, 2024

spolu commented Dec 11, 2024

spolu commented Dec 11, 2024

philipperolet commented Dec 11, 2024

tdraier commented Dec 11, 2024

spolu commented Dec 11, 2024

philipperolet commented Dec 11, 2024

philipperolet left a comment

Choose a reason for hiding this comment

spolu Dec 11, 2024

Choose a reason for hiding this comment

spolu Dec 11, 2024

Choose a reason for hiding this comment

tdraier Dec 11, 2024

Choose a reason for hiding this comment

tdraier Dec 11, 2024

Choose a reason for hiding this comment

spolu Dec 11, 2024

Choose a reason for hiding this comment

philipperolet left a comment

Choose a reason for hiding this comment

tdraier commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading