Validation failure because of invalid TypeScript enum configuration
khashayar opened this issue · 10 comments
TypeScript Enums are configured as type: Object, which is wrong, hence mongoose doesn't validate the enum fields correctly which results to persist incorrect values outside of enum definition into database.
I ran into this as well today. The issue only appears when using the --transpile-only flag of ts-node, in which case as @khashayar says, schema validation fails with the error Type of type property is not a string. It seems the reflected type is not identical when the flag is switched on.
I also found that the issue only appears if the enum is being referenced from another file — if it’s in the same file, there’s no problem.
You can try it out by cloning this gist:
https://gist.github.com/LeoBakerHytch/a9d2ebe7c0993ac947473715a3747bf3
import { prop, Typegoose } from 'typegoose';
import { MyEnum } from './enum';
class MySchema extends Typegoose {
@prop({ required: true, enum: MyEnum })
value!: MyEnum;
}export enum MyEnum {
foo = 'foo',
bar = 'bar',
} "scripts": {
"ok": "ts-node index.ts",
"error": "ts-node --transpile-only index.ts"
},
I have no clue if this is an actual bug in either reflect-metadata or the TypeScript compiler. For now turning off --transpile-only fixes it.
sorry, there is nothing we as typegoose maintainers could do about it (at least i cant...), because your code seems correct, and without the flag it dosnt give any compile errors
which typegoose version did you use @LeoBakerHytch ? (could you try 6.0.0-22?)
Hey, thanks for the quick response @hasezoey, and for the awesome work on this library!
I updated the linked gist to use 6.0.0-22. Same story unfortunately, it’s still broken with ts-node --transpile-only.
I also added a test case with ts-node --transpile-only --type-check, which does not fail. According to the ts-node docs, the effect of --transpile-only is to run the TypeScript compiler with isolatedModules: true. Strangely enough, tsc emits the correct output with this flag enabled, so the two modes must not be completely equivalent.
Reading around in the issues on the ts-node repo would suggest that it’s simply not possible to emit the correct type information without the type-checking step, which is what I’m really hoping to avoid. We’re using typegoose and ts-node on an API project that is incredibly slow to restart unless --transpile-only alone is used.
Would you consider a pull request to separate enum schema validation from string validation? At the same time I think it could be worth making it impossible to do the following:
import { prop } from '@hasezoey/typegoose';
export enum Enum {
foo = 'foo',
}
class MySchema {
@prop({ enum: Enum, minlength: 4, maxlength: 1, match: 'not-foo' }) // 🚨OK
value?: Enum;
}Correct me if I’m wrong, but I don’t think it ever makes sense to have generic string validations on an enum field — in this contrived example, each of the string validations in fact conflicts with the only allowed value from the enum, but this currently passes validation.
Roughly speaking, I propose to introduce an enumProp decorator, which would not accept the string validation options, and in baseProp the validation could allow a reflected type of either String or Object (which crucially would work even with --transpile-only). I suppose this is somewhat unsafe, in that you could specify a different object type than your enum, and the validation would pass:
import { prop } from '@hasezoey/typegoose';
import { ExportedEnum } from './ExportedEnum';
type NotAnEnum = {};
class MySchema {
@enumProp({ enum: ExportedEnum })
value?: NotAnEnum; // 🚨 Mistake, but would pass validation
}I don’t think it’s any less safe than arrayProp already is though, where this passes validation but is a mistake:
import { arrayProp } from '@hasezoey/typegoose';
class MySchema {
@arrayProp({ items: String })
value?: number[]; // 🚨 Also a mistake, but passes validation
}To avoid complicating the arrayProp, it might make sense for enumProp to accept an option of array: true, in which case it would accept a reflected type of Array, and would of course need the appropriate logic to add the field to the generated schema as an array.
I think this could be done as a non-breaking change, although arguably enum should be removed as an option from prop and arrayProp if there’s going to be a dedicated enumProp. If you’re already preparing a v6 it could be a good opportunity to make such a change.
Incidentally, I’ve had no response yet to my issue on ts-node (TypeStrong/ts-node#873), but I’m expecting that it won’t be possible to fix the issue there. Either way, I think enumProp could still make sense, but by all means say if you think this is a terrible idea altogether!
I’ve updated the gist, and you can run npm test to see all the cases discussed here:
https://gist.github.com/LeoBakerHytch/a9d2ebe7c0993ac947473715a3747bf3
By the way, I spotted a mistake in the docs for itemsRefPath:
class Car extends Typegoose {}
class Shop extends Typegoose {}
// in another class
class Another extends Typegoose {
@prop({ required: true, enum: 'Car' | 'Shop' })
which!: string;
@arrayProp({ itemsRefPath: 'which' })
items?: Ref<Car | Shop>[];
}I guess enum: 'Car' | 'Shop' should be enum: ['Car', 'Shop'], right? Because 'Car' | 'Shop' is a bitwise OR operation that evaluates to 0. It doesn’t type-check, but I was a little confused by that when I was looking at arrayProp for comparison.
Happy to roll fixing that into a PR if you like.
...TypeScript compiler with isolatedModules: true
sorry i dont know what this options does, and typegoose is not designed to be isolated...
Would you consider a pull request to separate enum schema validation from string validation?
i generally appreciate PR's, but to accept it, it must meet the guidelines and then on the implementation (but on this specific case, i will first look into it)
"I don’t think it’s any less safe than arrayProp already is though, where this passes validation but is a mistake:"' code
Yes i know the validation is wrong, but because typescript is just a pre-processor, and not an actual language, and reflect-metadata dosnt output nested things, it is not possible to infer the type of an array...
expecting that it won’t be possible to fix the issue there.
why shouldnt it be fixable there?
using typegoose and ts-node on an API project that is incredibly slow to restart unless
couldn't you just first compile it with tsc, and then execute from there? (to remove the ts-node step)
By the way, I spotted a mistake in the docs for itemsRefPath:
true, will get fixed
but I don’t think it ever makes sense to have generic string validations on an enum field
i know, but mongoose allows it because if enum's prop is a string, string validation can be applied
did i miss something?
Additional Note:
this is the currently only instance enum is used in the prop file, and otherwise the handling of enum dosnt differ from the normal prop, so i think an "enumProp" would be too much, unlike an arrayProp, which is needed because it has many different options, incompatible options, or an "mapProp" because map is needed for more type infomation and option handling
a note got added in 806e870 to show that this module should not be used with that option alone
I think you’re right that enumProp is overkill...
Digging into issues on ts-node and TypeScript some more, I found this comment on an issue which I think explains my problem:
The transpile (and transpileModule) functions unconditionally set isolatedModules, noLib, and noResolve to true. As a result, we cannot resolve Map to its constructor and instead return Object.
— microsoft/TypeScript#16872 (comment)
This is in reference to an issue about incorrect reflected types — basically the same problem as I’m having with the imported enum not being reflected as String (ts-node --transpile-only uses transpileModule internally).
So, I realised that the workaround was right in front of me the whole time: it’s as simple as just not importing the enum in the first place, but defining it in the same file as the schema 🤦♂
(For context, the reason to use ts-node without type-checking is that it’s incredibly slow, at least for the project I’m working on. I’m using it in conjunction with nodemon for local development, which restarts the app server whenever it detects source changes. This grinds to a halt if type-checking is enabled.)
Thanks for updating the docs with the warning by the way! Hopefully this issue doesn’t have to trip anyone else up.