-
Notifications
You must be signed in to change notification settings - Fork 3
Tag update #326
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
base: main
Are you sure you want to change the base?
Tag update #326
Changes from 3 commits
b3a76ca
a28c2ca
ac262e5
e22e4e4
f83ea3c
791ae67
8e22c82
d972adf
60efd6f
c28a60d
33e9adb
1c544b7
14b5c5e
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 | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,44 @@ | ||||||
| import * as dotenv from 'dotenv'; | ||||||
|
|
||||||
| dotenv.config(); | ||||||
|
|
||||||
| import { Collection, Db, MongoClient } from 'mongodb'; | ||||||
|
|
||||||
| const { MONGO_URL = 'mongodb://localhost:27017/erxes?directConnection=true' } = | ||||||
| process.env; | ||||||
|
|
||||||
| if (!MONGO_URL) { | ||||||
| throw new Error(`Environment variable MONGO_URL not set.`); | ||||||
| } | ||||||
|
|
||||||
| const client = new MongoClient(MONGO_URL); | ||||||
|
|
||||||
| let db: Db; | ||||||
| let Tags: Collection; | ||||||
|
|
||||||
| const command = async () => { | ||||||
| await client.connect(); | ||||||
| db = client.db() as Db; | ||||||
|
|
||||||
| Tags = db.collection('tags'); | ||||||
|
|
||||||
| try { | ||||||
| await Tags.updateMany( | ||||||
| {}, | ||||||
| { | ||||||
| $unset: { type: '', objectCount: '', order: '', scopeBrandIds: '' }, | ||||||
| $set: { isGroup: false, parentId: '' }, // flatten parentId: no nested parent tags allowed | ||||||
|
||||||
| $set: { isGroup: false, parentId: '' }, // flatten parentId: no nested parent tags allowed | |
| $set: { isGroup: false, parentId: null }, // flatten parentId: no nested parent tags allowed |
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.
Flattening parentId and enforcing unique name can collide; preflight duplicate names before migration.
If name is now unique (per tags schema), flattening to parentId: '' may create duplicate names that will break index creation or future writes.
- Preflight check duplicates and decide merge/rename rules before running at scale.
Example mongosh precheck:
db.tags.aggregate([
{ $group: { _id: "$name", c: { $sum: 1 }, ids: { $push: "$_id" } } },
{ $match: { c: { $gt: 1 } } },
{ $sort: { c: -1 } }
])Optionally migrate with a pipeline to compute fields atomically:
- await Tags.updateMany(
- {},
- {
- $unset: { type: '', objectCount: '', order: '', scopeBrandIds: '' },
- $set: { isGroup: false, parentId: '' }, // flatten parentId: no nested parent tags allowed
- },
- );
+ // Compute isGroup (if derivable) and flatten, then unset legacy fields
+ await Tags.updateMany(
+ {},
+ [
+ { $set: { parentId: '' } },
+ { $set: { isGroup: false } }, // or derive if you still have a marker field
+ { $unset: ['type', 'objectCount', 'order', 'scopeBrandIds'] },
+ ],
+ );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.
Ensure client connection is closed on success too (e.g., in a finally block) to avoid leaving open connections.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ export const documentSchema = new Schema( | |
| name: { type: String }, | ||
| content: { type: String }, | ||
| replacer: { type: String }, | ||
| code: { type: String }, | ||
| code: { type: String, optional: true }, | ||
|
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. 💡 Verification agent 🧩 Analysis chainUnique index on an optional field will cause duplicate-key errors; use a partial index and drop "optional".
Apply: - code: { type: String, optional: true },
+ code: { type: String, required: false, trim: true },
-documentSchema.index({ code: 1 }, { unique: true });
+documentSchema.index(
+ { code: 1 },
+ { unique: true, partialFilterExpression: { code: { $exists: true } } }
+);Verification (run in mongosh against the target DB): Also applies to: 18-18 🏁 Script executed: db.getCollection('documents').getIndexes()
# Expect the 'code_1' index to have { unique: true, partialFilterExpression: { code: { $exists: true } } }Length of output: 235 🏁 Script executed: #!/bin/bash
# Print the 'code' field definition and surrounding lines
rg -n "code:\s*\{" -C2 backend/core-api/src/modules/documents/db/definitions/documents.ts
# Print all index definitions on documentSchema
rg -n "documentSchema\.index" -C3 backend/core-api/src/modules/documents/db/definitions/documents.tsLength of output: 398 Use required:false (not optional) and add a partial filter to the unique ‘code’ index Replace the field and index definitions as follows: - code: { type: String, optional: true },
+ code: { type: String, required: false, trim: true },
- documentSchema.index({ code: 1 }, { unique: true });
+ documentSchema.index(
+ { code: 1 },
+ { unique: true, partialFilterExpression: { code: { $exists: true } } }
+ );After deployment, in the Mongo shell run: db.getCollection('documents').getIndexes();and confirm that the 🤖 Prompt for AI Agents |
||
| }, | ||
| { | ||
| timestamps: true, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,32 +5,16 @@ export const tagSchema = schemaWrapper( | |
| new Schema( | ||
| { | ||
| _id: mongooseStringRandomId, | ||
| name: { type: String, label: 'Name' }, | ||
| type: { | ||
| type: String, | ||
| label: 'Type', | ||
| index: true, | ||
| }, | ||
| name: { type: String, label: 'Name', unique: true }, | ||
|
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. 💡 Verification agent 🧩 Analysis chainGlobal unique on name is risky; scope uniqueness. unique: true enforces cross-tenant/global uniqueness and blocks same-name tags under different groups. Prefer a compound unique index (name, parentId) or include tenant scope (e.g., organizationId). - name: { type: String, label: 'Name', unique: true },
+ name: { type: String, label: 'Name' },
@@
-tagSchema.index({ _id: 1, name: 1, parentId: 1 });
+tagSchema.index({ _id: 1, name: 1, parentId: 1 });
+// Enforce uniqueness only within the same parent (adjust to include tenant scope if applicable)
+tagSchema.index({ name: 1, parentId: 1 }, { unique: true });If a unique index on name already exists in prod, we’ll need a migration to drop it before adding the compound index. I can prepare it. 🏁 Script executed: #!/bin/bash
# Locate the tags.ts schema definition
fd -t f tags.ts
# Preview the top of the file to see field definitions
sed -n '1,200p' backend/core-api/src/modules/tags/db/definitions/tags.ts
# Inspect the schema object and index declarations
rg -n "tagSchema" -C3 backend/core-api/src/modules/tags/db/definitions/tags.ts
rg -n "index" -C3 backend/core-api/src/modules/tags/db/definitions/tags.ts
# Check for tenant or hierarchical scope fields
rg -n "parentId" -C3 backend/core-api/src/modules/tags/db/definitions/tags.ts
rg -n "organizationId" -C3 backend/core-api/src/modules/tags/db/definitions/tags.tsLength of output: 2612 Scope tag name uniqueness to its parent (and tenant)
|
||
| colorCode: { type: String, label: 'Color code' }, | ||
| objectCount: { type: Number, label: 'Object count' }, | ||
| order: { type: String, label: 'Order', index: true }, | ||
| parentId: { | ||
| type: String, | ||
| optional: true, | ||
| index: true, | ||
| label: 'Parent', | ||
| }, | ||
| relatedIds: { | ||
| type: [String], | ||
| optional: true, | ||
| label: 'Children tag ids', | ||
| }, | ||
| parentId: { type: String, label: 'Parent' }, | ||
| relatedIds: { type: [String], label: 'Children tag ids' }, | ||
| isGroup: { type: Boolean, label: 'Is group', default: false }, | ||
| }, | ||
| { | ||
| timestamps: true, | ||
| }, | ||
| ), | ||
| ); | ||
|
|
||
| // for tags query. increases search speed, avoids in-memory sorting | ||
| tagSchema.index({ _id: 1, type: 1, order: 1, name: 1, createdAt: 1 }); | ||
| tagSchema.index({ _id: 1, name: 1, parentId: 1 }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,8 @@ | ||
| import { escapeRegExp } from 'erxes-api-shared/utils'; | ||
| import { tagSchema } from '@/tags/db/definitions/tags'; | ||
| import { removeRelatedTagIds, setRelatedTagIds } from '@/tags/utils'; | ||
| import { ITag, ITagDocument } from 'erxes-api-shared/core-types'; | ||
| import { Model } from 'mongoose'; | ||
| import { IModels } from '~/connectionResolvers'; | ||
| import { removeRelatedTagIds, setRelatedTagIds } from '@/tags/utils'; | ||
| import { tagSchema } from '@/tags/db/definitions/tags'; | ||
| export interface ITagModel extends Model<ITagDocument> { | ||
| getTag(_id: string): Promise<ITagDocument>; | ||
| createTag(doc: ITag): Promise<ITagDocument>; | ||
|
|
@@ -13,9 +12,40 @@ export interface ITagModel extends Model<ITagDocument> { | |
|
|
||
| export const loadTagClass = (models: IModels) => { | ||
| class Tag { | ||
| /* | ||
| * Get a tag | ||
| */ | ||
| public static async validate(_id: string | null, doc: ITag) { | ||
| const { name, parentId, isGroup } = doc; | ||
|
|
||
| const tag = await models.Tags.findOne({ name }); | ||
|
|
||
| if (tag && tag._id !== _id) { | ||
| throw new Error('There is already a tag with this name'); | ||
| } | ||
|
|
||
|
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. 💡 Verification agent ❓ Verification inconclusiveFix duplicate-name check: exclude self by _id and (optionally) scope by parentId Current comparison uses ObjectId !== string, which will always be true and will falsely block updates. Also consider scoping uniqueness by parentId if that’s the intended rule. Apply: - const tag = await models.Tags.findOne({ name });
-
- if (tag && tag._id !== _id) {
- throw new Error('There is already a tag with this name');
- }
+ const nameQuery: any = { name };
+ // Scope uniqueness to parent context; adjust if global uniqueness is desired
+ if (parentId) {
+ nameQuery.parentId = parentId;
+ } else {
+ nameQuery.$or = [
+ { parentId: { $exists: false } },
+ { parentId: '' },
+ { parentId: null },
+ ];
+ }
+ if (_id) {
+ nameQuery._id = { $ne: _id };
+ }
+ const dup = await models.Tags.findOne(nameQuery);
+ if (dup) {
+ throw new Error('There is already a tag with this name');
+ }Fix duplicate-name check: exclude current tag by const query: any = { name };
if (parentId) {
query.parentId = parentId;
} else {
query.$or = [
{ parentId: { $exists: false } },
{ parentId: null },
{ parentId: '' },
];
}
if (_id) {
query._id = { $ne: new mongoose.Types.ObjectId(_id) };
}
if (await models.Tags.findOne(query)) {
throw new Error('There is already a tag with this name');
}This ensures updates to an existing tag don’t conflict with itself, and enforces uniqueness within the intended scope. |
||
| if (parentId) { | ||
| const parentTag = await models.Tags.findOne({ _id: parentId }); | ||
|
|
||
| if (!parentTag?.isGroup) { | ||
| throw new Error('Parent tag must be a group'); | ||
| } | ||
| } | ||
|
|
||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (isGroup && parentId) { | ||
| throw new Error('Group tag cannot have parent tag'); | ||
| } | ||
|
|
||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if (_id) { | ||
| const existingTag = await models.Tags.findOne({ _id }); | ||
|
|
||
| if (isGroup && parentId) { | ||
| throw new Error('Group tag cannot have parent tag'); | ||
| } | ||
|
|
||
| if (!existingTag?.isGroup && isGroup && existingTag?.parentId) { | ||
| throw new Error('Cannot convert a nested tag into a group'); | ||
| } | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| public static async getTag(_id: string) { | ||
| const tag = await models.Tags.findOne({ _id }); | ||
|
|
||
|
|
@@ -26,178 +56,48 @@ export const loadTagClass = (models: IModels) => { | |
| return tag; | ||
| } | ||
|
|
||
| /** | ||
| * Create a tag | ||
| */ | ||
| public static async createTag(doc: ITag) { | ||
| const isUnique = await this.validateUniqueness(null, doc.name, doc.type); | ||
|
|
||
| if (!isUnique) { | ||
| throw new Error('Tag duplicated'); | ||
| } | ||
| await this.validate(null, doc); | ||
|
|
||
| const parentTag = await this.getParentTag(doc); | ||
|
|
||
| // Generatingg order | ||
| const order = await this.generateOrder(parentTag, doc); | ||
|
|
||
| const tag = await models.Tags.create({ | ||
| ...doc, | ||
| order, | ||
| createdAt: new Date(), | ||
| }); | ||
| const tag = await models.Tags.create(doc); | ||
|
|
||
|
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. suggestion (bug_risk): Removing parentId from child tags on tag removal may leave orphaned tags. Currently, child tags become orphaned when their parent is removed. Please review if this is intentional, or if child tags should be deleted or reassigned to maintain data integrity. |
||
| await setRelatedTagIds(models, tag); | ||
|
|
||
| return tag; | ||
| } | ||
|
|
||
| /** | ||
| * Update Tag | ||
| */ | ||
| public static async updateTag(_id: string, doc: ITag) { | ||
| const isUnique = await this.validateUniqueness( | ||
| { _id }, | ||
| doc.name, | ||
| doc.type, | ||
| ); | ||
|
|
||
| if (!isUnique) { | ||
| throw new Error('Tag duplicated'); | ||
| } | ||
|
|
||
| const parentTag = await this.getParentTag(doc); | ||
|
|
||
| if (parentTag && parentTag.parentId === _id) { | ||
| throw new Error('Cannot change tag'); | ||
| } | ||
| await this.validate(_id, doc); | ||
|
|
||
| const tag = await models.Tags.getTag(_id); | ||
|
|
||
| // Generatingg order | ||
| const order = await this.generateOrder(parentTag, doc); | ||
|
|
||
| const childTags = await models.Tags.find({ | ||
| $and: [ | ||
| { order: { $regex: new RegExp(escapeRegExp(tag.order || ''), 'i') } }, | ||
| { _id: { $ne: _id } }, | ||
| ], | ||
| const updated = await models.Tags.findOneAndUpdate({ _id }, doc, { | ||
| new: true, | ||
| }); | ||
|
|
||
| if (childTags.length > 0) { | ||
| const bulkDoc: Array<{ | ||
| updateOne: { | ||
| filter: { _id: string }; | ||
| update: { $set: { order: string } }; | ||
| }; | ||
| }> = []; | ||
|
|
||
| // updating child tag order | ||
| childTags.forEach((childTag) => { | ||
| let childOrder = childTag.order || ''; | ||
|
|
||
| childOrder = childOrder.replace(tag.order || '', order); | ||
|
|
||
| bulkDoc.push({ | ||
| updateOne: { | ||
| filter: { _id: childTag._id }, | ||
| update: { $set: { order: childOrder } }, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| await models.Tags.bulkWrite(bulkDoc); | ||
|
|
||
| await removeRelatedTagIds(models, tag); | ||
| } | ||
|
|
||
| await models.Tags.updateOne({ _id }, { $set: { ...doc, order } }); | ||
|
|
||
| const updated = await models.Tags.findOne({ _id }); | ||
|
|
||
| if (updated) { | ||
| await setRelatedTagIds(models, updated); | ||
| } | ||
|
|
||
| return updated; | ||
| } | ||
|
|
||
| /** | ||
| * Remove Tag | ||
| */ | ||
| public static async removeTag(_id: string) { | ||
| const tag = await models.Tags.getTag(_id); | ||
|
|
||
| const childCount = await models.Tags.countDocuments({ | ||
| parentId: _id, | ||
| }); | ||
| const childTagIds = await models.Tags.find({ parentId: _id }).distinct( | ||
| '_id', | ||
| ); | ||
|
|
||
| if (childCount > 0) { | ||
| throw new Error('Please remove child tags first'); | ||
| } | ||
| await models.Tags.updateMany( | ||
| { _id: { $in: childTagIds } }, | ||
| { $unset: { parentId: 1 } }, | ||
| ); | ||
|
|
||
| await removeRelatedTagIds(models, tag); | ||
|
|
||
| return models.Tags.deleteOne({ _id }); | ||
| } | ||
|
|
||
| /* | ||
| * Validates tag uniquness | ||
| */ | ||
| public static async validateUniqueness( | ||
| selector: any, | ||
| name: string, | ||
| type: string, | ||
| ): Promise<boolean> { | ||
| // required name and type | ||
| if (!name || !type) { | ||
| return true; | ||
| } | ||
|
|
||
| // can't update name & type same time more than one tags. | ||
| const count = await models.Tags.countDocuments(selector); | ||
|
|
||
| if (selector && count > 1) { | ||
| return false; | ||
| } | ||
|
|
||
| const obj = selector && (await models.Tags.findOne(selector)); | ||
|
|
||
| const filter: any = { name, type }; | ||
|
|
||
| if (obj) { | ||
| filter._id = { $ne: obj._id }; | ||
| } | ||
|
|
||
| const existing = await models.Tags.findOne(filter); | ||
|
|
||
| if (existing) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /* | ||
| * Get a parent tag | ||
| */ | ||
| static async getParentTag(doc: ITag) { | ||
| return models.Tags.findOne({ | ||
| _id: doc.parentId, | ||
| }).lean(); | ||
| } | ||
|
|
||
| /** | ||
| * Generating order | ||
| */ | ||
| public static async generateOrder( | ||
| parentTag: ITagDocument | null, | ||
| { name }: { name: string }, | ||
| ) { | ||
| const order = parentTag ? `${parentTag.order}${name}/` : `${name}/`; | ||
|
|
||
| return order; | ||
| } | ||
| } | ||
|
|
||
| tagSchema.loadClass(Tag); | ||
|
|
||
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.
Call client.close() before process.exit() for proper cleanup of the MongoDB connection.