microsoft / SqlScriptDOM

ScriptDOM/SqlDOM is a .NET library for parsing T-SQL statements and interacting with its abstract syntax tree
MIT License
139 stars 19 forks source link

ScriptDom.TSql160Parser fails to parse simple T-SQL expression #28

Open clement911 opened 1 year ago

clement911 commented 1 year ago

The Microsoft.SqlServer.TransactSql.ScriptDom.TSql160Parser fails to parse the following expression.

We noticed the issue on a much more complicated and realistic expression but we simplified the repro to the following minimal expression.

Repro:

using Microsoft.SqlServer.TransactSql.ScriptDom;

var parser = new TSql160Parser(true); //sql server 2022 parser
IList<ParseError> errors;

var expr = @"
(
    SELECT 1
    WHERE (IIF(1 > 0 AND 2 > 1, 1, 0)) = 1
)";

var res = parser.ParseExpression(new StringReader(expr), out errors);

if (errors?.Count > 0)
    Console.WriteLine(errors.First().Message); //ERROR!!!!    Incorrect syntax near ).

To prove that the given expression is definitely a valid one, you can simply execute the following statement in SSMS and see that SQL SERVER executes the query succesfully.

SELECT
(
    SELECT 1
    WHERE (IIF(1 > 0 AND 2 > 1, 1, 0)) = 1
)
clement911 commented 1 year ago

@llali Could this be addressed with the release of the sql server 2022 parser?

SeenaAugusty commented 1 year ago

Hi @clement911, this expression worked var expr = @" ( SELECT 1 WHERE IIF(1 > 0 AND 2 > 1, 1, 0) = 1 )";

clement911 commented 1 year ago

That's right. The parser gets confused by extra wrapping parentheses so hopefully the code fix is not too hard?

Of course we wouldn't write such a simple expression manually but our actual expression is much more complicated. Also, sql is sometimes autogenerated or provided by a user, in which case we have no control over it.

clement911 commented 1 year ago

Could we please get an update on this bug?

SeenaAugusty commented 1 year ago

@clement911 Apologies for the delay, would update you whether it would be included in the next release. Thank you

clement911 commented 1 year ago

@SeenaAugusty we really look forward to it.

By the way, if you would open source the parser, we'd be happy to spend some time working on such issues

SeenaAugusty commented 1 year ago

@clement911 we are planning to make it open source in near future :)

clement911 commented 1 year ago

@SeenaAugusty this is great news that you will open source this project 😄

However, I was also told the same thing.... 2.5 years ago.

Could you share the (rough) time line of when you will be open sourcing this project?

clement911 commented 1 year ago

Hi, any updates on this issue?

clement911 commented 1 year ago

@SeenaAugusty any news on the open sourcing? Bugs remain open for months or years so the only hope I see of them getting fixed is if you open source the code.

dzsquared commented 1 year ago

While the time it takes to bring ScriptDOM to open source may be frustrating, we'll keep those conversations primarily over at https://github.com/microsoft/DacFx/issues/101

clement911 commented 1 year ago

@dzsquared could you give a quick update there? Are you still working towards open sourcing the ScriptDom?

clement911 commented 1 year ago

Thank you to the team for open sourcing this wonderful library 😍

clement911 commented 1 year ago

Any chance this bug could get looked at ?

chlafreniere commented 1 year ago

@SeenaAugusty are you looking into this currently? If not, I can take a crack at this 😄

SeenaAugusty commented 1 year ago

@chlafreniere sure, not yet though,

clement911 commented 9 months ago

Just checking back on the status for this one. It strikes me as a very core issue with the parser failing to parse a simple expression. It would be great if this could be looked into.

MatthewBentz commented 5 months ago

Adding to this as I am having the exact same issue.

Microsoft.SqlServer.DacFx 162.2.111 Microsoft.SqlServer.TransactSql.ScriptDom 161.9109.0 Microsoft.SqlServer.TransactSql.ScriptDom.TSql160Parser Parse() is erroring on multiple parse attempts for valid SQL.

I am seeing this on a dacpac built with SQL Server 2022 target platform.

The following exceptions I'm seeing are: antlr.MismatchedTokenException antlr.NoViableAltForCharException

Interestingly, SQLPackage does not throw these errors.

My intention is to generate deployment script, where SQLPackage.exe /Action:Script will work and DacServices.GenerateDeployScript() does not.

clement911 commented 4 months ago

Could this be fixed?

clement911 commented 1 month ago

Is there something fundamentally hard about fixing this bug? The parser fails to parse a very basic expression. It's been over two years. I see that the parser is being enhanced in various ways but this doesn't seem to get picked up. I'm just wondering if there is hard to fix or whether it just slipped through the cracks.

volodymyr-svystun commented 1 month ago

I have exactly the same issue with different parsers: TSql120Parser and TSql160Parser the same: works in management studio, but fails in the current package. if I remove extra parentheses - it works, but I try to validate user input and this case should work.

Are there any updates on this issue?

volodymyr-svystun commented 1 month ago

@clement911 I have used another microsoft sql parser package and it works properly for my cases https://www.nuget.org/packages/Microsoft.SqlServer.Management.SqlParser/172.0.1#show-readme-container

clement911 commented 1 month ago

@volodymyr-svystun I've tried that package in the past but it doesn't work for us. It only provides basic parsing and not a full fidelity tree. If I recall correctly, it is optimized for intellisense.

@SeenaAugusty @chlafreniere any update on this bug would be much appreciated.

t0bbz0n commented 23 hours ago

Seeing the same issue when running Deploy using DacServices (not sure that I'm using Sql160Parser though).

Impressive commitment from @clement911 (2year anniversary in three days! ) :)

clement911 commented 15 hours ago

@t0bbz0n indeed, this has been an issues for years. It's strange to me because it's such a core issue. The cannot even parse such a simple expression, yet the parser is getting enhanced with new features regularly.

@SeenaAugusty @chlafreniere @dzsquared could anyone look into that one? Is there a deep architectural reason this cannot be fixed?