denisenkom / go-mssqldb

Microsoft SQL server driver written in go language
BSD 3-Clause "New" or "Revised" License
1.82k stars 495 forks source link

Support a few other cases in isProc #677

Closed tc-hib closed 3 years ago

tc-hib commented 3 years ago

This PR fixes isProc which detects if a string contains nothing but an object name.

It may still be a long way from the real lexer.

I had to fix 2 tests:

codecov[bot] commented 3 years ago

Codecov Report

Merging #677 (b197f83) into master (a78da2a) will increase coverage by 0.20%. The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #677      +/-   ##
==========================================
+ Coverage   70.19%   70.39%   +0.20%     
==========================================
  Files          23       23              
  Lines        5163     5179      +16     
==========================================
+ Hits         3624     3646      +22     
+ Misses       1310     1305       -5     
+ Partials      229      228       -1     
Impacted Files Coverage Δ
mssql.go 87.61% <87.50%> (+0.34%) :arrow_up:
tvp_go19.go 93.02% <0.00%> (-1.40%) :arrow_down:
token.go 60.60% <0.00%> (+0.12%) :arrow_up:
tds.go 66.70% <0.00%> (+0.94%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a78da2a...b197f83. Read the comment docs.

kardianos commented 3 years ago

Is this change useful? I mean, I've never seen a proc with a newline in it. That seems really odd. Is there a spec? It also seems odd to allow ";" in an escaped sequence. It seems like we are missing something still, but maybe being technically correct is better.

tc-hib commented 3 years ago

I don't find it useful in real life indeed, but all the test cases I've added are valid. These are valid queries:

I've tried [proc\n1], not because I didn't know if it was valid, but because I was wondering if we had to unescape the name before sending it. It worked, so I made this PR, which should be technically better. For reference: https://docs.microsoft.com/en-us/sql/relational-databases/databases/database-identifiers?view=sql-server-ver15

But I agree this probably isn't useful :)

tc-hib commented 3 years ago

I've tried [proc\n1], not because I didn't know if it was valid, but because I was wondering if we had to unescape the name before sending it.

A little more about this: when you have a proc named my\nproc and another one named [my\nproc] (create proc [[my\nproc]]])...

I'm just being curious, this is all pretty useless. Unless someone carelessly uses code generation to make procs, maybe. I wouldn't mind closing this PR. I'm just a little surprised we have explicitly written 2 wrong test cases, especially the table type name 123.[Test].