upgrade mongo and mongoose packages #3950
Conversation
app/api/odm/ModelWithPermissions.ts
Outdated
| const user = permissionsContext.getUserInContext(); | ||
| const query = { _id: data._id } as PermissionsUwaziFilterQuery<T>; |
There was a problem hiding this comment.
Not sure why I am required to do this "casts". In Typescript 4.x, they are not needed, in 3.x it does not accept this as an argument for the below function. I am open to ideas as to what may be wrong. Same issue in line 158.
There was a problem hiding this comment.
I am not sure how to solve but what i think is wrong is:
this method save uses data: WithPermissionsDataType<T> while appendPermissionsQuery uses as the first param query: PermissionsUwaziFilterQuery<T>
the first is based on PartialDataType, which means, _id can be not defined, the second is based on DataType which means _id can NOT be undefined. i think despite the difficult to read error this is the issue.
RafaPolit
changed the title
...pi/migrations/migrations/19-rename-uploads-to-files/specs/19-rename-uploads-to-files.spec.js
Show resolved
Hide resolved
| @@ -115,14 +120,15 @@ const controlPermissionsData = <T>(data: T & { published?: boolean }, user?: Use | |||
| }; | |||
|
|
|||
| export class ModelWithPermissions<T> extends OdmModel<T> { | |||
| async save(data: DataType<T & { permissions?: PermissionSchema[] }>) { | |||
There was a problem hiding this comment.
I believe this was wrong to start with, the & permissions should have been OUTSIDE of the bracket.
There was a problem hiding this comment.
im not sure this is the case with the current implementation, lets take a look together.
| import { tenants } from '../tenants/tenantContext'; | ||
| import { DB } from './DB'; | ||
|
|
||
| class MultiTenantMongooseModel<T> { | ||
| dbs: { [k: string]: mongoose.Model<WithId<T> & mongoose.Document> }; | ||
| dbs: { [k: string]: mongoose.Model<DataType<T>> }; |
There was a problem hiding this comment.
In the first iteration, DataType included an & Document clause. This is no longer true. This means that dbs no longer explicitly has the mongoose.Document type extended, but I think that T should already have that passed along on instance.
| } | ||
|
|
||
| return this.dbs[currentTenant.name]; | ||
| } | ||
|
|
||
| findById(id: any | string | number, select?: any) { | ||
| findById(id: any, select?: any) { |
There was a problem hiding this comment.
I was confused that this had any | string | number... any is already any.
| ) { | ||
| return this.dbForCurrentTenant().findOneAndUpdate(query, update, options); | ||
| } | ||
|
|
||
| async create(data: Readonly<Partial<T>> & { _id?: any }) { | ||
| async create(data: PartialDataType<T>) { |
There was a problem hiding this comment.
Nothing in the new implementation has Readonly. Any hint as to why this could have been needed? I just removed it from everywhere and nothing is failing. I'm open to being enlightened about this as well.
There was a problem hiding this comment.
I don't think it's needed for the permissions model 🤔
| // @ts-ignore | ||
| await expect(logHelper.upsertLogMany({ _id: null })).resolves.toBeEmpty; |
There was a problem hiding this comment.
This is testing a value that is not valid as _id. This type of errors should, in theory, be caught by TS from now on, so this test scenario should ideally not arise. Unsure if this merits a deletion of both expects (this and the next one, which is more or less the same situation), but I'm keeping them as I am unsure of the scenario this was written in.
fixes #3395
PR checklist:
QA checklist: