-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
fix(postgres,mysql): better support for enum and enum array #3414
Conversation
src/driver/mysql/MysqlDriver.ts
Outdated
@@ -471,6 +473,9 @@ export class MysqlDriver implements Driver { | |||
|
|||
} else if (columnMetadata.type === "simple-json") { | |||
value = DateUtils.stringToSimpleJson(value); | |||
|
|||
} else if (columnMetadata.type === "enum" && !isNaN(value)) { |
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.
what if value was defined as "1"
and user expects numbers?
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.
I don't think we'll be able to make "heterogeneous" way possible to work. The only option I see is to support string-only or number-only. If enum was defined in a "heterogeneous" way - we either throw error either enforce strings.
(to understand if its string or numeric based we do Object.keys(enum).every(key => typeof enum[key] === "number")
)
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.
Yeah I decided to force all string numbers to classic javascript numbers, so values of this enum will be transformed to number after select from databases:
enum TestEnum {
NO = "0",
YES = "1"
}
But in line you are highlighted we can check if exists numeric enum value (Object.keys(enum).some(key => enum[key] === parseInt(value))
) and convert it only if exists. After that change the only one edge case will be enum like this:
enum TestEnum {
YES1 = 1
YES = "1"
}
And this looks like really edge case.
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.
Right, last case is really edge case and I think its okay if we don't cover it. But first case should work properly (convert "0" to string, not number).
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.
Updated src and added test enum with numeric values as string.
This PR is amazing 🎆 |
columnMetadata.type === "enum" | ||
&& columnMetadata.enum | ||
&& !isNaN(value) | ||
&& columnMetadata.enum.indexOf(parseInt(value)) >= 0 |
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.
much more cleaver than what I have suggested!
Awesome! |
number
are returned asstring
#3072