PRO-6472: at startup, automatically supply a value for any new schema property that has a def or fallback def #4721

boutell · 2024-09-10T13:32:40Z

Summary

Summarize the changes briefly, including which issue/ticket this resolves. If it closes an existing Github issue, include "Closes #[issue number]"

What are the specific steps to test this change?

See tests.

What kind of change does this PR introduce?

(Check at least one)

Make sure the PR fulfills these requirements:

It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
The changelog is updated
Related documentation has been updated
Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

linear · 2024-09-10T13:32:42Z

PRO-6472 Undefined schema properties should be added to the database automatically

boutell · 2024-09-10T13:34:12Z

index.js

    self.apos.schema.registerAllSchemas();
    await self.apos.lock.withLock('@apostrophecms/migration:migrate', async () => {
-      await self.apos.migration.migrate(); // emits before and after events, inside the lock
+      await self.apos.migration.migrate(self.apos.argv);


For consistency as this was passed in the other, legacy invocation. It is not currently used.

boutell · 2024-09-10T13:34:58Z

index.js

    self.apos.schema.registerAllSchemas();
    await self.apos.lock.withLock('@apostrophecms/migration:migrate', async () => {
-      await self.apos.migration.migrate(); // emits before and after events, inside the lock
+      await self.apos.migration.migrate(self.argv);


For consistency, it was passed in the other, redundant invocation.

boutell · 2024-09-10T13:35:07Z

lib/moog.js

      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.`;


Updated language.

boutell · 2024-09-10T13:35:25Z

modules/@apostrophecms/migration/index.js

            });
          });
-        },
-        async executeMigrations() {


D'oh. This was completely redundant in the current setup.

boutell · 2024-09-10T13:36:05Z

modules/@apostrophecms/migration/index.js

-          }
+        }
+      },
+      before: {


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.

boutell · 2024-09-10T13:36:41Z

modules/@apostrophecms/migration/index.js

+          // Other migration-related facts that are not migration
+          // names are stored with a leading *, leave them alone
+          await self.db.removeMany({
+            _id: /^[^*]/


Somewhere to keep memos like the previous list of existing schema fields. Otherwise we have a proliferation of tiny mongo collections.

We could have just one new, separate collection like aposModuleSchema or aposRegisteredSchema so we don't store it into the aposMigration collection, which is a bit tied to migrations, but not really...

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.
Just wanted to have your thoughts on that, to have the schemas stored in a dedicated collection for clarity

I would rather not proliferate collections for very small amounts of data unless necessary as they can add open file descriptors in MongoDB.

boutell · 2024-09-10T13:37:33Z

modules/@apostrophecms/migration/index.js

+        // Migrations actually run on every invocation
+        // and automatically detect whether any work
+        // must be done
+        task: () => {}


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

should we remove the entire tasks block?

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.

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.

@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.

boutell · 2024-09-10T13:39:33Z

modules/@apostrophecms/migration/lib/addMissingSchemaFields.js

+  return {
+    async addMissingSchemaFields() {
+      let scans = 0; let updates = 0;
+      const lastPropLists = await self.getLastPropLists();


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.

boutell · 2024-09-10T13:43:21Z

modules/@apostrophecms/migration/lib/addMissingSchemaFields.js

+    },
+    expandPropList(schema, propList, dotPath) {
+      for (const field of schema) {
+        const newDotPath = dotPath ? `${dotPath}.${field.name}` : field.name;


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.

boutell · 2024-09-10T17:37:26Z

Please note I broke document versions out separately because it can be done at any point, ideally also this cycle, but nothing gets worse if we don't get to it this cycle:

https://linear.app/apostrophecms/issue/PRO-6577/improve-handling-of-new-defaults-for-properties-of-old-document

ETLaurent

overall logic lgtm

haroun

Also, we need to come up with a solution for the document-versions module.
Versions are stored in another collections.
If the field is required, we might want to add the def values or something (like for required fields without default value) during the restore method call.

haroun · 2024-09-13T15:03:29Z

modules/@apostrophecms/migration/index.js

        }
-      }
+      },
+      ...require('./lib/addMissingSchemaFields.js')(self)


Can we put the require at the top to make the dependency clear

const addMissingSchemaFields = require('./lib/addMissingSchemaFields.js');

and then

Suggested change

...require('./lib/addMissingSchemaFields.js')(self)

...addMissingSchemaFields(self)

haroun · 2024-09-13T15:08:30Z

modules/@apostrophecms/migration/index.js

+        // Migrations actually run on every invocation
+        // and automatically detect whether any work
+        // must be done
+        task: () => {}


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.

haroun · 2024-09-16T08:04:42Z

modules/@apostrophecms/migration/lib/addMissingSchemaFields.js

+        if (!_.isEqual(lastPropLists?.[name], propLists[name])) {
+          changesToPropLists = true;
+          scans++;
+          updates += await self.addMissingSchemaFieldsForDocType(name, propLists[name]);


This method accepts only 1 parameter below

The migration phase will run on any startup. Including an empty task. So omitting the call from the task is just eliminating redundancy.

At a future time we might reintroduce an optional behavior where the migration does not run in production except in the task, but that behavior has effectively not been possible before, so I don't have to introduce that feature to fix the redundancy bug.

* main: upgrate stylelint-config-apostrophe to 4.2.0 (#4735) PRO-6472: at startup, automatically supply a value for any new schema property that has a def or fallback def (#4721) fix sass warning (#4730) fix piecesFilters with dynamic choices (#4731)

* upstream/main: fix oversights in add missing schema fields (apostrophecms#4736) upgrate stylelint-config-apostrophe to 4.2.0 (apostrophecms#4735) PRO-6472: at startup, automatically supply a value for any new schema property that has a def or fallback def (apostrophecms#4721) fix sass warning (apostrophecms#4730) fix piecesFilters with dynamic choices (apostrophecms#4731) hotfix 4.7.1 (apostrophecms#4733) PRO-6591: parked fields overrides (apostrophecms#4732) do not let a page become a child of itself (apostrophecms#4726)

boutell added 4 commits September 9, 2024 17:19

works great but I have to write tests for more than top level properties

627f3b6

runs clean, good test coverage

e3a8280

lint free

28e38c7

changelog

7133dcb

boutell requested review from ETLaurent and haroun September 10, 2024 13:32

boutell commented Sep 10, 2024

View reviewed changes

argv pass consistently

10b0813

boutell commented Sep 10, 2024

View reviewed changes

ETLaurent mentioned this pull request Sep 11, 2024

PRO-6576 fix default values for object fields in widgets and generally unify newInstance implementations #4722

Merged

Merge branch 'main' into pro-6472

03243f1

ETLaurent previously approved these changes Sep 13, 2024

View reviewed changes

mispositioned a changelog entry

66f0a2a

boutell dismissed ETLaurent’s stale review via 66f0a2a September 13, 2024 12:20

boutell requested a review from ETLaurent September 13, 2024 12:22

haroun requested changes Sep 16, 2024

View reviewed changes

boutell added 2 commits September 16, 2024 10:58

issues pointed out by Harouna

f23f175

Merge branch 'main' into pro-6472

caf4423

haroun previously approved these changes Sep 20, 2024

View reviewed changes

Merge branch 'main' into pro-6472

d439f91

boutell dismissed haroun’s stale review via d439f91 September 23, 2024 12:53

boutell requested a review from haroun September 23, 2024 12:53

haroun approved these changes Sep 23, 2024

View reviewed changes

boutell merged commit ed608c3 into main Sep 23, 2024
8 checks passed

boutell deleted the pro-6472 branch September 23, 2024 13:11

	...require('./lib/addMissingSchemaFields.js')(self)
	...addMissingSchemaFields(self)

PRO-6472: at startup, automatically supply a value for any new schema property that has a def or fallback def #4721

PRO-6472: at startup, automatically supply a value for any new schema property that has a def or fallback def #4721

Uh oh!

Conversation

boutell commented Sep 10, 2024

Summary

What are the specific steps to test this change?

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

Uh oh!

linear bot commented Sep 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boutell Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boutell Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boutell Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boutell Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boutell commented Sep 10, 2024

Uh oh!

ETLaurent left a comment

Choose a reason for hiding this comment

Uh oh!

haroun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

boutell Sep 10, 2024 •

edited

Loading

boutell Sep 12, 2024 •

edited

Loading

boutell Sep 10, 2024 •

edited

Loading

boutell Sep 10, 2024 •

edited

Loading