Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[V5] Init Content API RFC #53

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[V5] Init Content API RFC #53

wants to merge 2 commits into from

Conversation

alexandrebodin
Copy link
Member

@alexandrebodin alexandrebodin commented Sep 5, 2023

Introduce the Content API changes for v5

You can read it here

@strapi-cla
Copy link

strapi-cla commented Sep 6, 2023

CLA assistant check
All committers have signed the CLA.


### Endpoints

Based on the [Database changes planned](https://github.com/strapi/rfcs/pull/52),we will now mainly work with the `documentId` within the API.
Copy link

@abdallahmz abdallahmz Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious about how users can make modifications to a particular entry using the API, given that it accepts documentId as input. For example, let's say I want to delete my entry with entryId : how would it be done with the new API ? (Similar questions can be raised for the other endpoints.)

One potential approach to address this issue (if it's considered as one) could be to introduce a new endpoint. Either /api/:contentType/document/:documentId to keep the old API working as is, or add /api/:contentType/entry/:entryId to at least have an endpoint similar to the old behavior. (edit: another idea is /api/:contentType/:documentId/:entryId though it might be harder to use for no real value)

From my current understanding, multiple entries can refer to the same documentId, and a "document" instance doesn't truly exist by itself (e.g., there isn't a structure like { documentId: '123', entries: [...] }).

If I misunderstood any aspect (I don't have deep expertise in the specifics of your codebase), feel free to ignore my message, though your clarifications will be appreciated. :)

"documentId": "clkgylw7d000108lc4rw1bb6s",
"entryId": 1,
"name": "Category A",
"meta": {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clashed with a component or field having the name "meta", is the later then shadowed?

"name": "Category A",
"meta": {
"locale": "en",
"createdAt": "2023-01-01T00:00:00.000Z",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think publication_state should be added to meta as well. (or all pivot elements)

# Unresolved questions

- Should we use `id` or `documentId` in the new response format.
- Should we use `entryId` or `entityId` to represent the underlying Ids to use for low level relations
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API should not rename fields, so if you want to use either of it, the database should be renamed from id to e?id as well.


Pros:

- No conflicts between user & system attributes
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can there be conflicts anyway? All fields are added flat to the database table, so noone can name a field created_at.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reference is to conflicts from existing projects with a field like "publishedBy" or "release" that do not exist as internal fields today but if upgraded to v5 would be a breaking change to those users and result in a conflict. Likewise if we decide to add any other system/internal fields during the v5 lifecycle.

| -------- | ----------------------------------------------- | ----------------------------------------------------------------------- |
| `GET` | `/api/:contentType` | Find a list of documents |
| `POST` | `/api/:contentType` | Create a document |
| `GET` | `/api/:contentType/:documentId` | Find a document |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both should be possible:

/api/:contentType/:documentId
and
/api/:contentType/:entityId.


| Method | Url | Desc |
| -------- | ----------------------------------------------- | ----------------------------------------------------------------------- |
| `GET` | `/api/:contentType` | Find a list of documents |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like filtering for pivot states will be baked into filters api. I think it would benefit, if there would be clear GET endpoints for like "all french documents" or "all live documents".

@roelbeerens
Copy link

I'm not sure where to comment on the RFC, so I'll add my comments here:

The simplified API response is a welcome feature 🥰. I do however have my concerns about the usage of 'documentId'. It isn't clear to me this response will also be used for the GraphQL response, but last I checked, using something like Apollo with caching, requires an 'id' field. Maybe this is configurable, but next to that, I don't really see the benefits of using something other than 'id'.

Rest looks super solid!

@ComfortablyCoding
Copy link

As mentioned above I would also advocate for keeping id as id instead of renaming it to documentId. id is a term everyone is already familiar with and understand what it is for (uid of a record).

I would say we keep ID for the uid of every record and the some other field (entryId sounds good to me) for indicating the id for the base record.

@alexandrebodin
Copy link
Member Author

Thank you all for your comments 🚀 We have an updated version coming next week that thanks to your notes 🔥

@alexandrebodin alexandrebodin force-pushed the v5/content-api branch 2 times, most recently from 8f5f31e to 8badeb6 Compare September 19, 2023 07:29
@alexandrebodin
Copy link
Member Author

Hello there 👋 There RFC was just updated to clarify certain differences between internal & meta attributes & show the different options that we have. If you can share what you think of each of them for your own DX that would be great 💯

@ComfortablyCoding
Copy link

ComfortablyCoding commented Sep 20, 2023

The updated version looks a lot better now!

Given the option return formats I prefer option 2, but with it being meta not internal

Reasoning

  • recognizable property from v4
  • "internal" can be seen as something that should not be touched or access normally which is not the intention.
  • common indicator of meta data of a record which most of the data being stored there will be.
  • i dont see it being nested one level deep as an issue. i actually prefer it this way
  • do not see a user confusing what is defined on the model and what is being shown in the meta. A lot of education has already been done via v4 on what meta is and what it contains.
  • can also put in the metadata information mentioned later on in the spec, having all this "meta" info in one place makes the most sense to me.

Looking forward!

@Boegie19
Copy link

Boegie19 commented Sep 20, 2023

This gives me an other question. no matter what option you do. how would you for come name conflicts on the DB layer.
Only way I see this working is by prefixing the user generated content.

Also lets say we go for option 3. would that mean none can start a string with a _ or $ since if you where to allow for that that would mean you can still have conflicts.

@alexandrebodin
Copy link
Member Author

This gives me an other question. no matter what option you do. how would you for come name conflicts on the DB layer. Only way I see this working is by prefixing the user generated content.

Also lets say we go for option 3. would that mean none can start a string with a _ or $ since if you where to allow for that that would mean you can still have conflicts.

Depending on the option we go with for the DX here we will of course adapt the DB layer to use prefixes when necessary. It would of course mean we forbidden those prefixes to be used (which is already the case)

@Boegie19
Copy link

Boegie19 commented Sep 21, 2023

Other question I see no where in the RFC how you would reference a specific entry in the document.

@alexandrebodin
Copy link
Member Author

alexandrebodin commented Sep 22, 2023

Other question I see no where in the RFC how you would reference a specific entry in the document.

the document is a virtual thing fetching a document will always return the content of a single entry. (Based on filters provided or default filters)

if you do (just an example) ?locale=en&state=draft you will get the draft entry for the english locale and by default you would get your default locale and the published version of your document.

@Boegie19
Copy link

Other question I see no where in the RFC how you would reference a specific entry in the document.

the document is a virtual thing fetching a document will always return the content of a single entry. (Based on filters provided or default filters)

if you do (just an example) ?locale=en&state=draft you will get the draft entry for the english locale and by default you would get your default locale and the published version of your document.

I think you should also be able to request by id since how would you get a record where a new one has been posted over so it no longer is first in any combination of keys also there needs to be an endpoint that lists all enteries in a document.

@alexandrebodin
Copy link
Member Author

Hello everyone.

Thank you all for your comments 💯

We will go ahead and update this RFC to reflect the option we are going to move forward with and merge it in the upcoming days.

After carefully considering the different Options, Option 1 is the most viable one for v5 and is the one we will be moving forward with.

Here are the main driving deciders

  • It provides the better DX for end users
    • Removes as much nesting as possible
    • Simpler query syntax & selecting
    • Less magic = less error prone
  • By managing reserverd words & pre-reserving words we can prevent conflicts for upcoming features and until another major comes along where we can introduce new sets of reserved words.
  • Offers the possiblility to incrementally add new features by reserving a specialized prefix for all new feature (e.g strapi_)
  • Allows introducing Option 2 or 3 without breaking changes by reserving a prefix & a meta/internal key if it becomes a requirement

@martinbeentjes
Copy link

I have been using Strapi for a short while now, and the nesting of attributes has been somewhat a hassle. Luckily, with ChatGPT nowadays the generation of all data classes (I consume in Kotlin) made it easier.

For a smooth migration, I think the best option is to supply a request parameter that allows for choosing the format. That way, all consumers of content can prepare for a new version and then a v5.3.x for example can migrate the version to be the new one by default.


What I am missing, how would the localizations be added under $.localizations here, just as flattened as the root level? Or is that to be defined?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants