-
-
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
feat: mariadb uuid inet4 inet6 column data type support #9845
Merged
pleerock
merged 12 commits into
typeorm:master
from
smith-xyz:8832/mariadb-uuid-inet4-inet6-data-types
Apr 25, 2023
Merged
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
89c21a2
feat: mariadb inet4, inet6, uuid data type support
smith-xyz 4a8c454
refactor: cleanup unnecessary methods
smith-xyz f938bb5
style: mysqldriver formatting
smith-xyz 9fa5053
fix: handle length column metadata mariadb uuid
smith-xyz 1ade6b1
fix: 8832 test suite to verify errors correctly
smith-xyz cd37c35
style: fix 8832 test formatting
smith-xyz 9e78052
fix: 8832 error testing cleanup
smith-xyz c39dbec
fix: remove defaulting column type feature
smith-xyz 8253ce7
style: fix formatting
smith-xyz 165fa23
fix: remove unnecessary dbms error test
smith-xyz 0cffbe6
fix: remove unused import in test
smith-xyz a7213fc
fix: ensure defaulting uuid generation column type
smith-xyz File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import { Column, Entity, PrimaryGeneratedColumn } from "../../../../src" | ||
|
||
@Entity() | ||
export class BadInet4 { | ||
@PrimaryGeneratedColumn("uuid") | ||
id?: string | ||
|
||
@Column({ type: "inet4", length: "36" }) | ||
inet4: string | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import { Column, Entity, PrimaryGeneratedColumn } from "../../../../src" | ||
|
||
@Entity() | ||
export class BadInet6 { | ||
@PrimaryGeneratedColumn("uuid") | ||
id?: string | ||
|
||
@Column({ type: "inet6", length: "36" }) | ||
inet6: string | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import { Column, Entity, PrimaryGeneratedColumn } from "../../../../src" | ||
|
||
@Entity() | ||
export class BadUuid { | ||
@PrimaryGeneratedColumn("uuid") | ||
id?: string | ||
|
||
@Column({ type: "uuid", length: "36" }) | ||
uuid: string | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import { | ||
Entity, | ||
PrimaryGeneratedColumn, | ||
Column, | ||
ManyToOne, | ||
} from "../../../../src" | ||
import { User } from "./User" | ||
|
||
@Entity() | ||
export class Address { | ||
@PrimaryGeneratedColumn("increment") | ||
id?: number | ||
|
||
@Column() | ||
city: string | ||
|
||
@Column() | ||
state: string | ||
|
||
@ManyToOne(() => User, (user) => user.addresses) | ||
user: User | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import { | ||
Column, | ||
Entity, | ||
OneToMany, | ||
PrimaryGeneratedColumn, | ||
} from "../../../../src" | ||
import { Address } from "./Address" | ||
|
||
@Entity() | ||
export class User { | ||
@PrimaryGeneratedColumn("uuid") | ||
id?: string | ||
|
||
/** can use a default but testing against mysql since they're shared drivers */ | ||
@Column({ type: "uuid" }) | ||
uuid: string | ||
|
||
@Column({ type: "inet4" }) | ||
inet4: string | ||
|
||
@Column({ type: "inet6" }) | ||
inet6: string | ||
|
||
/** testing generation */ | ||
@Column({ type: "uuid", generated: "uuid" }) | ||
another_uuid_field?: string | ||
|
||
@OneToMany(() => Address, (address) => address.user) | ||
addresses?: Address[] | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
correct me if I'm wrong, but from what I see for
mysql
generationStrategy isuuid
and column type isuuid
as well. But we don't return36
in such case now, which is different from what we had before? If yes, this can cause issues for those who is using mysql right now.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.
that's correct I believe. It looks like before if the generationStrategy was "uuid" it would default the column type from "uuid" to "varchar". Then here if no length was attributed to the column, it would plug in "36" since mysql needs a length for the varchar.
Yes, in returning I see your point this does cause issues for existing users that need the column type to be a varchar here when they want the generation strategy to be uuid. I'll have to rethink this one. Let me know if you have any suggestions.
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.
@pleerock my thoughts are that maybe PrimaryGeneratedColumnUUIDOptions should probably add one more property that allows for configuring the actual column type to be used so that this logic isn't tied up in the driver itself.
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.
Better to save the scope of this PR only to mariadb. So we just need to add proper checks I avoid any breaking changes for mysql users.
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.
yes good point.
For this, maybe it makes more sense to do this conditional to make it clear of the exact case its needed:
if (column.generationStrategy === "uuid" && column.type === "varchar") return 36
the reason its needed is because the generation strategy being uuid would default to varchar column type when normalized. Because of that when we get the column length it would wrongly do a length of 255 by default, but should be 36 in this case.
Other than that at line 727 the else if will check if the column type is "uuid" and if its a mysql driver it will default to varchar.
Let me know if that sounds correct and I can make the updates.
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.
yes, sounds good. But please check again everything before it will be merged, just to make sure we didn't break user use cases.
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.
added a private property for seeing if the uuid data type can be used, if false then when generation strategy is uuid or if the column type is uuid and its not supported it will default to varchar as it use to. Also modified the conditional as mentioned. Added a regression test for mysql to the test suite. Let me know if anything else is needed.
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.
Could we get a merge? @pleerock