-
-
Notifications
You must be signed in to change notification settings - Fork 194
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
Resolving IndexCreateStatement
not generating a valid query when no name is provided
#638
base: master
Are you sure you want to change the base?
Conversation
The to use the typed builder is good, but in the given case it is better to ensure the query cannot fail by providing a name, where it 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.
Hey @mohs8421, nice stuffs!! Yes, then we always have a proper index name regardless.
@ikrivosheev do you still remember the "bug" where MySQL constraint / index name exceed the maximum length of MySQL's limit? Looks like @mohs8421 just truncate the long name.
well I didn't want to introduce an other source of error, so I just looked up the limit, although I know that this approach is still flawed because it could abreviate more properly, but then again, it is rather unlikely that somebody introduces an index, which exceeds this limit and have two of these indexes on the same table. In that case that user would still be able to manually specify a name, so nothing lost on that end. |
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.
@mohs8421 thank you for the PR! Some comment.
return name; | ||
} | ||
let prefix = if self.is_primary_key() { | ||
"pri" |
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'm not sure it's a good solution to hard-code prefixes in the library... @billy1624 what do you think?
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 think it is ok in these cases because:
it was like that before at other locations you can already find these prefixes.
This is merely a fallback if nothing has been specified by the user.
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.
CC @billy1624 @tyt2y3)
} else { | ||
"idx" | ||
}; | ||
format!("{}-{}", prefix, self.index.get_column_names().join("-")) |
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.
This is similar to the preview comment...
@billy1624 hello! Yes, I do. Well, It's makes sense to truncate the long name... |
On mysql systems, the
IndexCreateStatement
will generate a name, if no name was provided, resulting in always having a valid statement regarding the name.PR Info
Bug Fixes