-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Consolidate content versioning #22413
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 1c529f8 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think we should disallow passing
delta
increateOne
/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards the breaking change by requiring admins to update permissions. 🤔
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can/should sort directly via DB query
Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
Scope
What's changed:
delta
field to thedirectus_versions
tablePotential Risks / Drawbacks
Review Notes / Questions
versions
field in thedirectus_revisions
table will be removed in a subsequent releasedelta
field indirectus_versions
is empty, the existing behaviour of fetching fromdirectus_revisions
remainsRelated to #17894
PR carried over from #22227 which was reverted in #22412.