Consolidate content versioning #22413
Conversation
🦋 Changeset detectedLatest commit: c0ad03f The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
- I think we should disallow passing
deltaincreateOne/createMany, similar toupdateMany(see inline comment) - Reference docs need to be updated: https://docs.directus.io/reference/system/versions.html
|
|
||
| const finalVersionDelta = assign({}, existingDelta, revisionDelta ? JSON.parse(revisionDelta) : null); | ||
|
|
||
| await this.updateOne(key, { delta: finalVersionDelta }); |
There was a problem hiding this comment.
Saving a new version now requires update permission on directus_versions due to this call - we may want to disregard permissions here (need to check whether that is safe), make it a breaking change requiring update permissions, or find another solution
| @@ -167,10 +171,11 @@ export class VersionsService extends ItemsService { | |||
| } | |||
|
|
|||
| override async updateMany(keys: PrimaryKey[], data: Partial<Item>, opts?: MutationOptions): Promise<PrimaryKey[]> { | |||
| // Only allow updates on "key" and "name" fields | |||
| // Only allow updates on "key", "name" and "delta" fields | |||
There was a problem hiding this comment.
I think we should handle delta updates inside the save method and don't allow direct updates, instead keeping updateMany for updates to key, name only.
Currently, would also do unnecessary duplicate work when called via save, fetching and merging getVersionSavesById two times.
If we should keep it here, reference docs need to be updated 1 where we would need to make clear what the differences are.
Footnotes
| return [versions[0]['delta']]; | ||
| } | ||
|
|
||
| const saves = await this.getVersionSavesById(versions[0]['id']); |
There was a problem hiding this comment.
It feels strange, that this method now (in most/all cases?) returns only one item (the delta), especially while it is called getVersionSaves.
Do we even need that method and getVersionSavesById anymore? Can't we just always use the delta? I think the associated revision items now only serve the purpose of history, but not as source of truth anymore.
There was a problem hiding this comment.
This getVersionSavesById is just for the edge case where a new version is created but on the older system when the migration is being run. All usages of it are part of the else clause where the delta is missing. 🤔
There was a problem hiding this comment.
Not sure if we should/can account for this edge case. I think this would mean we could never really get rid of it. In any case, I think this method needs some clean-up, for the first reason stated above.
| const deltas = sortBy( | ||
| await knex.select('id', 'delta').from('directus_revisions').where('version', '=', version.id), | ||
| 'id', | ||
| ).map((revision) => (typeof revision.delta === 'string' ? JSON.parse(revision.delta) : revision.delta)); |
There was a problem hiding this comment.
I think we can/should sort directly via DB query
| @@ -238,9 +244,9 @@ router.post( | |||
|
|
|||
| await service.save(req.params['pk']!, req.body); | |||
|
|
|||
| const saves = await service.getVersionSavesById(req.params['pk']!); | |||
| const updatedVersion = await service.readOne(req.params['pk']!); | |||
There was a problem hiding this comment.
I think we could update save to return the delta and use that instead of an additional readOne call.
Scope
What's changed:
deltafield to thedirectus_versionstablePotential Risks / Drawbacks
Review Notes / Questions
versionsfield in thedirectus_revisionstable will be removed in a subsequent releasedeltafield indirectus_versionsis empty, the existing behaviour of fetching fromdirectus_revisionsremainsRelated to #17894
PR carried over from #22227 which was reverted in #22412.