fix: fix filter by autogenerated _id for mongodb #188
Conversation
adminforth/dataConnectors/mongo.ts
Outdated
| return value?.toString(); | ||
| } else if (field.name === '_id' && !field.fillOnCreate) { | ||
| // if "_id" was created by mongo it will be ObjectId | ||
| return value?.toString(); |
There was a problem hiding this comment.
@Astaldos I am a little bit afraid of case if user returns bson object from fillOnCreate.
Maybe it is better to check it like if type of value is object then do .toString(), is it safe or you see any potential issues with it?
adminforth/dataConnectors/mongo.ts
Outdated
| return Decimal128.fromString(value?.toString()); | ||
| } else if (field.name === '_id' && !field.fillOnCreate) { | ||
| // if "_id" was created by mongo it supposed to be saved as ObjectId | ||
| return ObjectId.createFromHexString(value); |
There was a problem hiding this comment.
same here, we can check if value is type of string or number or null or undefined, then wrap it in bson Object id, otherwise pass "as is"
adminforth/dataConnectors/mongo.ts
Outdated
| return !!value; | ||
| } else if (field.type == AdminForthDataTypes.DECIMAL) { | ||
| return value?.toString(); | ||
| } else if (field.name === '_id' && !field.fillOnCreate) { |
There was a problem hiding this comment.
@Astaldos will it brake something if we will remove && !field.fillOnCreate? I mean for the case if user returns object id in fillOnCreate.
NoOne7135
force-pushed
the
fix-mongo-id-filter
branch
from
66f9bdc to
5ba0c0e
Compare
… key candidate handling in MongoConnector
…prove primary key extraction logic feat: use ObjectId for car resource creation in demo and change data type to DECIMAL in resource template
ivictbor
left a comment
ivictbor
left a comment
There was a problem hiding this comment.
@NoOne7135 great move ahead, I think only couple of very minors left here. But yep, this is very complex one, but with our target to support anything what can happen in real world, our strategy sounds pretty fine and best we can do for now...
Did you carefully test all 9 cells in this table?
If needed fell free to pass to QA
| const HEX24 = /^[0-9a-f]{24}$/i; // 24-hex (Mongo ObjectId) | ||
|
|
||
| function idToString(v: any) { | ||
| if (v == null) return null; |
There was a problem hiding this comment.
== is very implicit, better use !v maybe or
=== null || v === undefined return v
| private pkCandidates(pkValue: any): any[] { | ||
| if (pkValue == null || typeof pkValue !== "string") return [pkValue]; | ||
| const candidates: any[] = [pkValue]; | ||
| try { candidates.push(new UUID(pkValue)); } catch(err) { console.error(`Failed to create UUID from ${pkValue}: ${err.message}`); } |
There was a problem hiding this comment.
@NoOne7135 console.error? so if user uses own string ids like "asdfa1231" we will say him "Error"?
| if (pkValue == null || typeof pkValue !== "string") return [pkValue]; | ||
| const candidates: any[] = [pkValue]; | ||
| try { candidates.push(new UUID(pkValue)); } catch(err) { console.error(`Failed to create UUID from ${pkValue}: ${err.message}`); } | ||
| try { candidates.push(new ObjectId(pkValue)); } catch(err) { console.error(`Failed to create ObjectId from ${pkValue}: ${err.message}`); } |
There was a problem hiding this comment.
@NoOne7135 are you sure logging is needed in both catch blocks? I suppose if there is uuid it might be not object id right?
No description provided.