-
Notifications
You must be signed in to change notification settings - Fork 615
fix oversights in add missing schema fields #4736
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 all commits
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 |
|---|---|---|
|
|
@@ -35,12 +35,48 @@ module.exports = (self) => { | |
| }, async doc => { | ||
| const changes = {}; | ||
| await self.addMissingSchemaFieldsFor(doc, schema, '', changes); | ||
| if (doc.type === '@apostrophecms/user') { | ||
| // In an abundance of caution we should leave permissions-related | ||
| // properties alone. The role will only be missing if transitioning | ||
| // from advanced permissions back to regular and there is a right | ||
| // way to do that (via a task) so we should not interfere. There | ||
| // should be no default for the password, but don't chance it; | ||
| // also leave advanced permission group data alone just for good measure | ||
| delete changes.password; | ||
| delete changes.role; | ||
| delete changes.groupsIds; | ||
| delete changes.groupsFields; | ||
| } | ||
| if (Object.keys(changes).length > 0) { | ||
| updates++; | ||
| const seen = new Set(); | ||
| const keep = {}; | ||
| for (const key of Object.keys(changes)) { | ||
| // MongoDB won't tolerate $set for both | ||
| // a parent key and a subkey, which makes sense | ||
| // because we set the entire value for the | ||
| // parent key anyway. Filter out subkeys | ||
| // when the parent key is also being set | ||
| const components = key.split('.'); | ||
| let path = ''; | ||
| let preempted = false; | ||
| for (const component of components) { | ||
| path = path ? `${path}.${component}` : component; | ||
| if (seen.has(path)) { | ||
| preempted = true; | ||
| break; | ||
| } | ||
| } | ||
| if (preempted) { | ||
| continue; | ||
| } | ||
| keep[key] = changes[key]; | ||
| seen.add(key); | ||
| } | ||
| return self.apos.doc.db.updateOne({ | ||
| _id: doc._id | ||
| }, { | ||
| $set: changes | ||
| $set: keep | ||
| }); | ||
| } | ||
| }); | ||
|
|
@@ -54,8 +90,10 @@ module.exports = (self) => { | |
| 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) { | ||
| // Supply the default if a field is undefined, and also in the | ||
|
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. Not really our fault per se if the object field had a weird non-object value rather than being undefined, but it happened "in the field" with testbed, so fix it! |
||
| // edge case where due to a past bug an object field was falsy | ||
| if ((doc[field.name] === undefined) || | ||
| ((field.type === 'object') && !doc[field.name])) { | ||
| // Only undefined should fall back here | ||
| const def = klona((field.def === undefined) ? self.apos.schema.fieldTypes[field.type]?.def : field.def); | ||
| if (def !== undefined) { | ||
|
|
@@ -91,7 +129,10 @@ module.exports = (self) => { | |
| } | ||
| } else if (field.type === 'relationship') { | ||
| for (const [ key, item ] of Object.entries(doc[field.fieldsStorage] || {})) { | ||
| const itemPath = `${newDotPath}.${field.fieldsStorage}.${key}`; | ||
|
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. This was the wrong path. |
||
| // Careful, newDotPath contains the relationship name, we are storing | ||
| // in the fieldsStorage property for this relationship | ||
| const storageDotPath = dotPath ? `${dotPath}.${field.fieldsStorage}` : field.fieldsStorage; | ||
| const itemPath = `${storageDotPath}.${key}`; | ||
| self.addMissingSchemaFieldsFor(item, field.schema, itemPath, changes); | ||
| } | ||
| } | ||
|
|
@@ -101,7 +142,7 @@ module.exports = (self) => { | |
| getPropLists() { | ||
| const schema = {}; | ||
| for (const [ name, module ] of Object.entries(self.apos.doc.managers)) { | ||
| if (!module.__meta.name) { | ||
| if (!module.__meta?.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. Fake managers have no |
||
| // Just a placeholder for a type present in the | ||
| // database but not found in the project code | ||
| continue; | ||
|
|
||
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 seems like the existing unit tests should have caught this, so I don't know of a good way to test it better, but in testbed this did come up and required the fix I have implemented here.