microsoft / sqlmanagementobjects

Sql Management Objects, an API for scripting and managing SQL Server and Azure SQL Database
Other
130 stars 21 forks source link

SMO issue with default column values. #175

Open sdonovanuk opened 1 day ago

sdonovanuk commented 1 day ago

Greetings. To reproduce. . .

Create the following table:

CREATE TABLE DefaultTest (
    [FlagValue] INT DEFAULT 10
)

Then, generate the SMO script. You will get:

CREATE TABLE [dbo].[DefaultTest] (
    [FlagValue] INT DEFAULT ((10)) NULL
);

This is odd, why has the 10 become ((10))? Syntax wise, 10 is OK. The same happens if you create the table without the default, then add it with:

ALTER TABLE DefaultTest ADD DEFAULT 10 FOR [FlagValue]

What's interesting, is that if you use Azure Data Studio to edit the ((10)) default value in the UI back to 10 (maybe the UI isn't using SMO?), the SQL it generates displays as:

CREATE TABLE [dbo].[DefaultTest] (
    [FlagValue] INT DEFAULT 10 NULL
);

. . . which instantly becomes ((10)) again when you apply the change to the database.

Now, the engine always stores the constraints definition wrapped with ( and ). I contend there is a bug in SMO -- if the value within parenthesis is a primitive (string, integer, etc), then it doesn't need parenthesis.

Why is this a problem? Currently writing code to compare schemas against two databases, and having to parse weird output from DacFx's ScriptComparison, which is complaining that default values of (0) sometimes become ((0)). Now, I can write an AST parser using ScriptDom, but isn't this an SMO error?

Basically, it means that SMO isn't generating a technically identical representation of the table.

Thanks!

shueybubbles commented 11 hours ago

I 'd be reluctant to try to make SMO smart. Today it just gets the text of the definition and emits it. Doing anything else introduces a chance of emitting bad code.

sdonovanuk commented 11 hours ago

"I'd be reluctant to try to make SMO smart". I can understand. However, it means that DacFx's SchemaComparison generates false-positives that you need to parse out, by writing your own (increasingly complex) derived object from TSqlFragmentVisitor. This negates the whole purpose of that DacFx offering schema comparator. This is clearly a bug, either SMO needs to tackle it, or SchemaComparison does. I'm concerned that SMO isn't generating technically identical scripts.

Update: it's not just these primitive values in default-constraints. Check-constraints have problems, they introduce (or remove) spaces on conditions, and, the casing on binary operators (AND, OR, etc,) changes. Again, this flags as a difference, that you have to code out. I'm having to take the generated SQLs from the SchemaComparison, RE-parse them, and using those to compare. So far, I've found 4 such problems, and I haven't even delved that deep yet.

  1. Not attaching default schema to procedures, views, UDFs.
  2. Default constraints: primitives getting extra parenthesis.
  3. Check constraints: OR/AND changing case.
  4. Check constraints: space being inserted/removed (can't remember which) between comparisons.

It's not just me -- anyone using schema-compare in Azure Data Studio will be having the same problem.

I don't understand how this isn't a fundamental bug in what these libraries are doing.

Now, back to default-constraints, I can accept that the definition field on sys.default_constraints is explicitly wrapped with ( and ), and that is probably considered . . . legacy handling, but that doesn't give SchemaComparison an excuse to treat it as a difference. At the very least, it should be an option.

Note: I've also logged as an issue against DacFx, but they appear to have a 3x week backlog looking into things :-)

llali commented 9 hours ago

@sdonovanuk I agree with you on schema compare needs lots of improvements, so it doesn't show differences on things that are basically the same thing. we are aware of some of those bugs but not all. if you please add any schema compare bug you can think of in dacfx repo (https://github.com/microsoft/DacFx/issues?q=is%3Aissue%20state%3Aopen) and we prioritize them in our future planning. However, schema compare doesn't use SMO for parsing/generating scripts so I'm not sure how this SMO bug can have any impact of schema compare.