dataplat / dbatools

🚀 SQL Server automation and instance migrations have never been safer, faster or freer
https://dbatools.io
MIT License
2.39k stars 787 forks source link

New-DbaDbTable - Fix some issues #9314

Closed andreasjordan closed 2 months ago

andreasjordan commented 3 months ago

Type of Change

See issue for details.

potatoqualitee commented 2 months ago

this is a pretty big change, no? it no longer guesses. can it just guess better?

andreasjordan commented 2 months ago

Let me give a bit context about the changes. Inside of the process block, there are three changes.

The first change is just a syntax change and does not change any functionality.

The second change is the main fix and does change the functionality. It is inside an if block, so it is only executed, if $sqlDbType (which is [Microsoft.SqlServer.Management.Smo.SqlDataType]$($column.Type)) is one of the following: VarBinary, VarChar, NVarChar, Char, NChar. And additionally, there must not be a configured length - so we need the "Max"-Datatypes. The old code uses the function Get-SqlType for a reason I do not know (more on that later). and because inside of the function there was no switch part for the char-Datatypes, the default of VarChar was used. So the function Get-SqlType is targeted on non-char-types like numbers and dates, and (in my oppinion) must not be used here. So I removed the use here.

The third change is a fix as well. In this code block, $sqlDbType -eq 'Decimal', so we definitly have a decimal. So there is no "DecimalMax", and so there is no need to build this kind of datatype. The deleted line is most likely a copy-paste-bug.

After making these changes, the function Get-SqlType is not used anymore - so I deleted it. Even before my changes, the function was never called with datatypes like Int32 or Boolean. It was most likely just a leftover functionallity from an earlier version of the command.

wsmelton commented 2 months ago

The test for this command should be updated to show that the bug is fixed by this and still works for the other data types expected.

andreasjordan commented 2 months ago

I just added a new test to my local unpatched repository:

image

potatoqualitee commented 2 months ago

Thank you for the explanation 😊

potatoqualitee commented 2 months ago

and the test and the test suggestion haha