Skip to content

Commit ed608c3

Browse files
authored
PRO-6472: at startup, automatically supply a value for any new schema property that has a def or fallback def (#4721)
* works great but I have to write tests for more than top level properties * runs clean, good test coverage * lint free * changelog * argv pass consistently * mispositioned a changelog entry * issues pointed out by Harouna
1 parent 01c1bf0 commit ed608c3

File tree

6 files changed

+488
-15
lines changed

6 files changed

+488
-15
lines changed

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
### Adds
66

7+
* Apostrophe now automatically adds the appropriate default values for new properties in the schema, even for existing documents in the database. This is done automatically during the migration phase of startup.
78
* Adds focus states for media library's Uploader tile
89
* Adds focus states file attachment's input UI
910
* Simplified importing rich text widgets via the REST API. If you you have HTML that contains `img` tags pointing to existing images, you can now import them all quickly. When supplying the rich text widget object, include an `import` property with an `html` subproperty, rather than the usual `content` property. You can optionally provide a `baseUrl` subproperty as well. Any images present in `html` will be imported automatically and the correct `figure` tags will be added to the new rich text widget, along with any other markup acceptable to the widget's configuration.
@@ -14,6 +15,7 @@
1415

1516
### Fixes
1617

18+
* Apostrophe's migration logic is no longer executed twice on every startup and three times in the migration task. It is executed exactly once, always at the same point in the startup process. This bug did not cause significant performance issues because migrations were only executed once, but there is a small performance improvement.
1719
* The `@apostrophecms/page` module APIs no longer allow a page to become a child of itself. Thanks to [Maarten Marx](https://github.com/Pixelguymm) for reporting the issue.
1820
* Uploaded SVGs now permit `<use>` tags granted their `xlink:href` property is a local reference and begins with the `#` character. This improves SVG support while mitgating XSS vulnerabilities.
1921
* Default properties of object fields present in a widget now populate correctly even if never focused in the editor.

index.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,7 @@ async function apostrophe(options, telemetry, rootSpan) {
300300
self.apos.schema.validateAllSchemas();
301301
self.apos.schema.registerAllSchemas();
302302
await self.apos.lock.withLock('@apostrophecms/migration:migrate', async () => {
303-
await self.apos.migration.migrate(); // emits before and after events, inside the lock
303+
await self.apos.migration.migrate(self.argv);
304304
// Inserts the global doc in the default locale if it does not exist; same for other
305305
// singleton piece types registered by other modules
306306
for (const module of Object.values(self.modules)) {

lib/moog.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ module.exports = function(options) {
170170
}
171171
for (const key of Object.keys(step)) {
172172
if (!(validKeys.includes(key) || cascades.includes(key))) {
173-
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.`;
173+
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.`;
174174
throw `${clarifyModuleName(step.__meta.name)}: ${message}`;
175175
}
176176
}

modules/@apostrophecms/migration/index.js

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
const broadband = require('broadband');
22
const _ = require('lodash');
3+
const addMissingSchemaFields = require('./lib/addMissingSchemaFields.js');
4+
35
// Provide services for database migration. The `@apostrophecms/migration:migrate` task
46
// carries out all migrations that have been registered with this module. Migrations
57
// are used to make changes to the database at the time of a new code deployment,
@@ -38,15 +40,11 @@ module.exports = {
3840
manager.addSortifyMigration(field.name);
3941
});
4042
});
41-
},
42-
async executeMigrations() {
43-
if ((process.env.NODE_ENV !== 'production') || self.apos.isNew) {
44-
// Run migrations at dev startup (low friction).
45-
// Also always run migrations at first startup, so even
46-
// in prod with a brand new database the after event always fires
47-
// and we get a chance to mark the migrations as skipped
48-
await self.migrate(self.apos.argv);
49-
}
43+
}
44+
},
45+
before: {
46+
async addMissingSchemaFields() {
47+
await self.addMissingSchemaFields();
5048
}
5149
}
5250
};
@@ -245,8 +243,13 @@ module.exports = {
245243
// Just in case the db has no documents but did
246244
// start to run migrations on a previous attempt,
247245
// which causes an occasional unique key error if not
248-
// corrected for here
249-
await self.db.removeMany({});
246+
// corrected for here.
247+
//
248+
// Other migration-related facts that are not migration
249+
// names are stored with a leading *, leave them alone
250+
await self.db.removeMany({
251+
_id: /^[^*]/
252+
});
250253
await self.db.insertMany(self.migrations.map(migration => ({
251254
_id: migration.name,
252255
at,
@@ -286,14 +289,18 @@ module.exports = {
286289
throw err;
287290
}
288291
}
289-
}
292+
},
293+
...addMissingSchemaFields(self)
290294
};
291295
},
292296
tasks(self) {
293297
return {
294298
migrate: {
295299
usage: 'Apply any necessary migrations to the database.',
296-
task: self.migrate
300+
// Migrations actually run on every invocation
301+
// and automatically detect whether any work
302+
// must be done
303+
task: () => {}
297304
}
298305
};
299306
}
Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
const _ = require('lodash');
2+
const { klona } = require('klona');
3+
4+
module.exports = (self) => {
5+
return {
6+
async addMissingSchemaFields() {
7+
let scans = 0; let updates = 0;
8+
const lastPropLists = await self.getLastPropLists();
9+
const propLists = self.getPropLists();
10+
let changesToPropLists = false;
11+
for (const name of Object.keys(propLists)) {
12+
if (!_.isEqual(lastPropLists?.[name], propLists[name])) {
13+
changesToPropLists = true;
14+
scans++;
15+
updates += await self.addMissingSchemaFieldsForDocType(name);
16+
}
17+
}
18+
if (changesToPropLists) {
19+
await self.updateLastPropLists(propLists);
20+
}
21+
// Returned for tests
22+
return {
23+
scans,
24+
updates
25+
};
26+
},
27+
async addMissingSchemaFieldsForDocType(name) {
28+
let updates = 0;
29+
const schema = (self.apos.doc.managers[name] || {}).schema;
30+
if (!schema) {
31+
return;
32+
}
33+
await self.eachDoc({
34+
type: name
35+
}, async doc => {
36+
const changes = {};
37+
await self.addMissingSchemaFieldsFor(doc, schema, '', changes);
38+
if (Object.keys(changes).length > 0) {
39+
updates++;
40+
return self.apos.doc.db.updateOne({
41+
_id: doc._id
42+
}, {
43+
$set: changes
44+
});
45+
}
46+
});
47+
return updates;
48+
},
49+
// Adds changes to the object "changes" so that a single
50+
// $set call can be made at the end. Use of a single
51+
// object passed by reference also avoids creating many
52+
// unnecessary objects in memory during a time-sensitive
53+
// operation
54+
addMissingSchemaFieldsFor(doc, schema, dotPath, changes) {
55+
for (const field of schema) {
56+
const newDotPath = dotPath ? `${dotPath}.${field.name}` : field.name;
57+
// Supply the default
58+
if (doc[field.name] === undefined) {
59+
// Only undefined should fall back here
60+
const def = klona((field.def === undefined) ? self.apos.schema.fieldTypes[field.type]?.def : field.def);
61+
if (def !== undefined) {
62+
if (!Object.hasOwn(changes, dotPath)) {
63+
changes[newDotPath] = def;
64+
}
65+
// Also change it in memory so that if this is a subproperty of a
66+
// new object, the change for that new object will have this
67+
// subproperty too, plus we don't get crashes above when testing the
68+
// subproperties' current values
69+
doc[field.name] = def;
70+
}
71+
}
72+
// Address defaults of subproperties
73+
if (field.type === 'area') {
74+
const basePath = `${newDotPath}.items`;
75+
for (let i = 0; (i < (doc[field.name]?.items || []).length); i++) {
76+
const widgetPath = `${basePath}.${i}`;
77+
const widget = doc[field.name].items[i];
78+
const widgetSchema = self.apos.area.getWidgetManager(widget.type)?.schema;
79+
if (!widgetSchema) {
80+
continue;
81+
}
82+
self.addMissingSchemaFieldsFor(widget, widgetSchema, widgetPath, changes);
83+
}
84+
} else if (field.type === 'object') {
85+
self.addMissingSchemaFieldsFor(doc[field.name], field.schema, newDotPath, changes);
86+
} else if (field.type === 'array') {
87+
for (let i = 0; (i < (doc[field.name] || []).length); i++) {
88+
const itemPath = `${newDotPath}.${i}`;
89+
const item = doc[field.name][i];
90+
self.addMissingSchemaFieldsFor(item, field.schema, itemPath, changes);
91+
}
92+
} else if (field.type === 'relationship') {
93+
for (const [ key, item ] of Object.entries(doc[field.fieldsStorage] || {})) {
94+
const itemPath = `${newDotPath}.${field.fieldsStorage}.${key}`;
95+
self.addMissingSchemaFieldsFor(item, field.schema, itemPath, changes);
96+
}
97+
}
98+
}
99+
return changes;
100+
},
101+
getPropLists() {
102+
const schema = {};
103+
for (const [ name, module ] of Object.entries(self.apos.doc.managers)) {
104+
if (!module.__meta.name) {
105+
// Just a placeholder for a type present in the
106+
// database but not found in the project code
107+
continue;
108+
}
109+
schema[name] = [];
110+
self.expandPropList(module.schema, schema[name], '');
111+
}
112+
return schema;
113+
},
114+
expandPropList(schema, propList, dotPath) {
115+
for (const field of schema) {
116+
const newDotPath = dotPath ? `${dotPath}.${field.name}` : field.name;
117+
propList.push(newDotPath);
118+
if (field.schema) {
119+
self.expandPropList(field.schema, propList, newDotPath);
120+
}
121+
}
122+
},
123+
async getLastPropLists() {
124+
const result = await self.db.findOne({
125+
_id: '*lastPropLists'
126+
});
127+
return result?.propLists;
128+
},
129+
async updateLastPropLists(propLists) {
130+
return self.db.updateOne({
131+
_id: '*lastPropLists'
132+
}, {
133+
$set: {
134+
propLists
135+
}
136+
}, {
137+
upsert: true
138+
});
139+
}
140+
};
141+
};

0 commit comments

Comments
 (0)