Skip to content
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

Stop stripping new lines from migration script literals #32788

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

ajcvickers
Copy link
Contributor

Fixes #32730

Will port to 8.0 for patch after merge.

@ajcvickers ajcvickers requested a review from a team January 11, 2024 14:10
{
continue;
}

var trimmed = line.TrimStart();
if (trimmed.StartsWith("GO", StringComparison.OrdinalIgnoreCase)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this also happen if a multiline string includes a "GO" line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndriySvyryd This is true, but I went back to the original regex and it also failed with this case. I'm not sure we can catch this case without really parsing the SQL--we need to know, for sure, are we in a SQL literal or not? I'm thinking we should fix the regression here, and I'll file another issue to discuss if we want to parse this SQL for-real.

Fixes #32730

Will port to 8.0 for patch after merge.
@ajcvickers ajcvickers merged commit f42b7cd into main Jan 17, 2024
@ajcvickers ajcvickers deleted the 240111_NotSoClose branch January 17, 2024 15:10
ajcvickers added a commit that referenced this pull request Jan 17, 2024
* Stop stripping new lines from migration script literals

Fixes #32730

Will port to 8.0 for patch after merge.

* More tests for cases with GO
maumar pushed a commit that referenced this pull request Feb 7, 2024
…#32788) (#32851)

* Stop stripping new lines from migration script literals (#32788)

* Stop stripping new lines from migration script literals

Fixes #32730

Will port to 8.0 for patch after merge.

* More tests for cases with GO

* Quirk

protected override void Up(MigrationBuilder migrationBuilder)
{
migrationBuilder.Sql($"INSERT INTO Table1 (Id, Bar, Description) VALUES (-2, ' ', '{TestValue}')");
Copy link
Member

@roji roji Feb 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajcvickers this fails on PostgreSQL because of unquoted identifier. Note that GO (below) is in general a SQL Server thing, so maybe move this specific testing to MigrationsInfrastructureSqlServerTest?

Opened #33056.


protected override void Up(MigrationBuilder migrationBuilder)
{
migrationBuilder.Sql($"INSERT INTO Table1 (Id, Bar, Description) VALUES (-2, ' ', '{TestValue}')");
Copy link
Contributor

@lauxjpn lauxjpn Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails using Pomelo.

Bar (renamed from Foo in 00000000000002_Migration2) is of type int. It is not a string compatible column, but is set to a string with a single whitespace ' ' here in 00000000000005_Migration5.

Leads to the following exception for us:

MySqlConnector.MySqlException (0x80004005): Incorrect integer value: ' ' for column 'Bar' at row 1
   at MySqlConnector.Core.ServerSession.ReceiveReplyAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in D:\Sources\MySqlConnector-8.0\src\MySqlConnector\Core\ServerSession.cs:line 936
   at MySqlConnector.Core.ResultSet.ReadResultSetHeaderAsync(IOBehavior ioBehavior) in D:\Sources\MySqlConnector-8.0\src\MySqlConnector\Core\ResultSet.cs:line 37

Opened #33331.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL Server Migrations - SqlOperation.Sql is stripped of all empty lines
4 participants