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

New-DbaDbTable - Fix some issues #9314

Merged
merged 2 commits into from
Apr 12, 2024
Merged

Conversation

andreasjordan
Copy link
Contributor

Type of Change

  • Bug fix (non-breaking change, fixes New-DbaDbTable creates wrong datatype if no MaxLength specified #9198 )
  • New feature (non-breaking change, adds functionality, fixes # )
  • Breaking change (affects multiple commands or functionality, fixes # )
  • Ran manual Pester test and has passed (.\tests\manual.pester.ps1)
  • Adding code coverage to existing functionality
  • Pester test is included
  • If new file reference added for test, has is been added to github.com/dataplat/appveyor-lab ?
  • Unit test is included
  • Documentation
  • Build system

See issue for details.

@potatoqualitee
Copy link
Member

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

@andreasjordan
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

image

@potatoqualitee
Copy link
Member

Thank you for the explanation 😊

@potatoqualitee
Copy link
Member

and the test and the test suggestion haha

@potatoqualitee potatoqualitee merged commit 1f06a1c into development Apr 12, 2024
12 checks passed
@potatoqualitee potatoqualitee deleted the NewDbaDbTable_fix branch April 12, 2024 10:52
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.

New-DbaDbTable creates wrong datatype if no MaxLength specified
3 participants