-
Notifications
You must be signed in to change notification settings - Fork 615
PRO-6472: at startup, automatically supply a value for any new schema property that has a def or fallback def #4721
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
Changes from 5 commits
627f3b6
e3a8280
28e38c7
7133dcb
10b0813
03243f1
66f0a2a
f23f175
caf4423
d439f91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -170,7 +170,7 @@ module.exports = function(options) { | |
| } | ||
| for (const key of Object.keys(step)) { | ||
| if (!(validKeys.includes(key) || cascades.includes(key))) { | ||
| const message = upgradeHints[key] || `${key} is not a valid top level property for an Apostrophe 3.x module. Make sure you nest regular module options in the new "options" property.`; | ||
| const message = upgradeHints[key] || `${key} is not a valid top level property for an Apostrophe module. Make sure you nest regular module options in the "options" property.`; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated language. |
||
| throw `${clarifyModuleName(step.__meta.name)}: ${message}`; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||
| const broadband = require('broadband'); | ||||||
| const _ = require('lodash'); | ||||||
|
|
||||||
| // Provide services for database migration. The `@apostrophecms/migration:migrate` task | ||||||
| // carries out all migrations that have been registered with this module. Migrations | ||||||
| // are used to make changes to the database at the time of a new code deployment, | ||||||
|
|
@@ -38,15 +39,11 @@ module.exports = { | |||||
| manager.addSortifyMigration(field.name); | ||||||
| }); | ||||||
| }); | ||||||
| }, | ||||||
| async executeMigrations() { | ||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. D'oh. This was completely redundant in the current setup. |
||||||
| if ((process.env.NODE_ENV !== 'production') || self.apos.isNew) { | ||||||
| // Run migrations at dev startup (low friction). | ||||||
| // Also always run migrations at first startup, so even | ||||||
| // in prod with a brand new database the after event always fires | ||||||
| // and we get a chance to mark the migrations as skipped | ||||||
| await self.migrate(self.apos.argv); | ||||||
| } | ||||||
| } | ||||||
| }, | ||||||
| before: { | ||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Invoke the new capability, which is not a registered, named migration, but rather an independent part of the migration phase that does its own checks to determine if work is needed. |
||||||
| async addMissingSchemaFields() { | ||||||
| await self.addMissingSchemaFields(); | ||||||
| } | ||||||
| } | ||||||
| }; | ||||||
|
|
@@ -245,8 +242,13 @@ module.exports = { | |||||
| // Just in case the db has no documents but did | ||||||
| // start to run migrations on a previous attempt, | ||||||
| // which causes an occasional unique key error if not | ||||||
| // corrected for here | ||||||
| await self.db.removeMany({}); | ||||||
| // corrected for here. | ||||||
| // | ||||||
| // Other migration-related facts that are not migration | ||||||
| // names are stored with a leading *, leave them alone | ||||||
| await self.db.removeMany({ | ||||||
| _id: /^[^*]/ | ||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Somewhere to keep memos like the previous list of existing schema fields. Otherwise we have a proliferation of tiny mongo collections.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could have just one new, separate collection like Inside that dedicated collection, we could have the schema stored in documents: 1 document in that collection = 1 module schema. That would require changing a lot of code, and since you've already finished the feature, maybe it's not worth it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would rather not proliferate collections for very small amounts of data unless necessary as they can add open file descriptors in MongoDB. |
||||||
| }); | ||||||
| await self.db.insertMany(self.migrations.map(migration => ({ | ||||||
| _id: migration.name, | ||||||
| at, | ||||||
|
|
@@ -286,14 +288,18 @@ module.exports = { | |||||
| throw err; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| }, | ||||||
| ...require('./lib/addMissingSchemaFields.js')(self) | ||||||
|
||||||
| ...require('./lib/addMissingSchemaFields.js')(self) | |
| ...addMissingSchemaFields(self) |
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.
Done
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.
Previously this task invoked the migrate code three times in dev: once in the actual, intended migration phase of startup (see index.js), once in an "apostrophe:ready" handler, and once here. Being thorough I guess geez past Tom
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.
should we remove the entire tasks block?
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.
The task must exist otherwise users will get an error when launching the migration task according to our documentation. The fact that it is implicit is an implementation detail that should not concern users. Later we may eliminate the implicit invocation except when the task is being invoked, in production.
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 means that if I manually run node app.js @apostrophecms/migration:migrate nothing will happen now.
I might want to run the migration without starting apostrophe server, or start apostrophe without the migration.
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.
@haroun it has worked like this for about as long as A3 has existed. Plus extra, redundant invocations in two other places. Since we have this automatic invocation at every startup I am fixing the bug of redundant invocations. As I mentioned we might reconsider this behavior later for production, but in dev it never makes sense to start up without migrating, if your code does not behave after migrations then that is a bug in the migrations that needs attention and something that will break in production if bypassed, etc.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| const _ = require('lodash'); | ||
| const { klona } = require('klona'); | ||
|
|
||
| module.exports = (self) => { | ||
| return { | ||
| async addMissingSchemaFields() { | ||
| let scans = 0; let updates = 0; | ||
| const lastPropLists = await self.getLastPropLists(); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A "prop list" is a list of all schema properties that existed for a doc type last time, for comparison with the new list for this startup to determine if any property additions might potentially be needed. We make this determination quickly and move on if new work is not needed. |
||
| const propLists = self.getPropLists(); | ||
| let changesToPropLists = false; | ||
| for (const name of Object.keys(propLists)) { | ||
| if (!_.isEqual(lastPropLists?.[name], propLists[name])) { | ||
| changesToPropLists = true; | ||
| scans++; | ||
| updates += await self.addMissingSchemaFieldsForDocType(name, propLists[name]); | ||
|
||
| } | ||
| } | ||
| if (changesToPropLists) { | ||
| await self.updateLastPropLists(propLists); | ||
| } | ||
| // Returned for tests | ||
| return { | ||
| scans, | ||
| updates | ||
| }; | ||
| }, | ||
| async addMissingSchemaFieldsForDocType(name) { | ||
| let updates = 0; | ||
| const schema = (self.apos.doc.managers[name] || {}).schema; | ||
| if (!schema) { | ||
| return; | ||
| } | ||
| await self.eachDoc({ | ||
| type: name | ||
| }, async doc => { | ||
| const changes = {}; | ||
| await self.addMissingSchemaFieldsFor(doc, schema, '', changes); | ||
| if (Object.keys(changes).length > 0) { | ||
| updates++; | ||
| return self.apos.doc.db.updateOne({ | ||
| _id: doc._id | ||
| }, { | ||
| $set: changes | ||
| }); | ||
| } | ||
| }); | ||
| return updates; | ||
| }, | ||
| // Adds changes to the object "changes" so that a single | ||
| // $set call can be made at the end. Use of a single | ||
| // object passed by reference also avoids creating many | ||
| // unnecessary objects in memory during a time-sensitive | ||
| // operation | ||
| addMissingSchemaFieldsFor(doc, schema, dotPath, changes) { | ||
| for (const field of schema) { | ||
| const newDotPath = dotPath ? `${dotPath}.${field.name}` : field.name; | ||
| // Supply the default | ||
| if (doc[field.name] === undefined) { | ||
| // Only undefined should fall back here | ||
| const def = klona((field.def === undefined) ? self.apos.schema.fieldTypes[field.type]?.def : field.def); | ||
| if (def !== undefined) { | ||
| if (!Object.hasOwn(changes, dotPath)) { | ||
| changes[newDotPath] = def; | ||
| } | ||
| // Also change it in memory so that if this is a subproperty of a | ||
| // new object, the change for that new object will have this | ||
| // subproperty too, plus we don't get crashes above when testing the | ||
| // subproperties' current values | ||
| doc[field.name] = def; | ||
| } | ||
| } | ||
| // Address defaults of subproperties | ||
| if (field.type === 'area') { | ||
| const basePath = `${newDotPath}.items`; | ||
| for (let i = 0; (i < (doc[field.name]?.items || []).length); i++) { | ||
| const widgetPath = `${basePath}.${i}`; | ||
| const widget = doc[field.name].items[i]; | ||
| const widgetSchema = self.apos.area.getWidgetManager(widget.type)?.schema; | ||
| if (!widgetSchema) { | ||
| continue; | ||
| } | ||
| self.addMissingSchemaFieldsFor(widget, widgetSchema, widgetPath, changes); | ||
| } | ||
| } else if (field.type === 'object') { | ||
| self.addMissingSchemaFieldsFor(doc[field.name], field.schema, newDotPath, changes); | ||
| } else if (field.type === 'array') { | ||
| for (let i = 0; (i < (doc[field.name] || []).length); i++) { | ||
| const itemPath = `${newDotPath}.${i}`; | ||
| const item = doc[field.name][i]; | ||
| self.addMissingSchemaFieldsFor(item, field.schema, itemPath, changes); | ||
| } | ||
| } else if (field.type === 'relationship') { | ||
| for (const [ key, item ] of Object.entries(doc[field.fieldsStorage] || {})) { | ||
| const itemPath = `${newDotPath}.${field.fieldsStorage}.${key}`; | ||
| self.addMissingSchemaFieldsFor(item, field.schema, itemPath, changes); | ||
| } | ||
| } | ||
| } | ||
| return changes; | ||
| }, | ||
| getPropLists() { | ||
| const schema = {}; | ||
| for (const [ name, module ] of Object.entries(self.apos.doc.managers)) { | ||
| if (!module.__meta.name) { | ||
| // Just a placeholder for a type present in the | ||
| // database but not found in the project code | ||
| continue; | ||
| } | ||
| schema[name] = []; | ||
| self.expandPropList(module.schema, schema[name], ''); | ||
| } | ||
| return schema; | ||
| }, | ||
| expandPropList(schema, propList, dotPath) { | ||
| for (const field of schema) { | ||
| const newDotPath = dotPath ? `${dotPath}.${field.name}` : field.name; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're not coming up with a real dot path to patch anything at this stage, just a conceptual dot path that namespaces these properties enough to determine if there are differences from one generation of code to the next by comparing two lists of paths. Real dot paths get figured out in the guts of addMissingSchemaFieldsFor, which does the hard work if a difference does exist. |
||
| propList.push(newDotPath); | ||
| if (field.schema) { | ||
| self.expandPropList(field.schema, propList, newDotPath); | ||
| } | ||
| } | ||
| }, | ||
| async getLastPropLists() { | ||
| const result = await self.db.findOne({ | ||
| _id: '*lastPropLists' | ||
| }); | ||
| return result?.propLists; | ||
| }, | ||
| async updateLastPropLists(propLists) { | ||
| return self.db.updateOne({ | ||
| _id: '*lastPropLists' | ||
| }, { | ||
| $set: { | ||
| propLists | ||
| } | ||
| }, { | ||
| upsert: true | ||
| }); | ||
| } | ||
| }; | ||
| }; | ||
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.
For consistency, it was passed in the other, redundant invocation.