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

Add a setting to turn off Ole Db "smell check" #1974

Merged
merged 6 commits into from
Oct 11, 2023

Conversation

Giorgi
Copy link
Contributor

@Giorgi Giorgi commented Oct 10, 2023

Implements #1971

Copy link
Member

@mgravell mgravell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few comments inline; needs some kind of test; this is a little awkward because of concurrency concerns, but we should at least try

Dapper/SqlMapper.cs Outdated Show resolved Hide resolved
Dapper/SqlMapper.Settings.cs Outdated Show resolved Hide resolved
@Giorgi
Copy link
Contributor Author

Giorgi commented Oct 10, 2023

I think it now should work correctly in both cases but I'm not sure how to test it.

@Giorgi Giorgi requested a review from mgravell October 10, 2023 10:47
Dapper/SqlMapper.cs Outdated Show resolved Hide resolved
@mgravell
Copy link
Member

Re testing; IIRC in the original ticket there was an example where it did the wrong thing; can we not just use something based on that to show it continuing to fail when set to true and working when set to false? The only caveat is that we should use try/finally to set the value back to what it was originally after the test

@Giorgi
Copy link
Contributor Author

Giorgi commented Oct 10, 2023

I tried #1914 and it still throws the same exception.

@mgravell
Copy link
Member

mgravell commented Oct 10, 2023

I tried #1914 and it still throws the same exception.

well yes, but that is a different glitch; I wouldn't expect that to change, so it is good that it didn't - however, we should be able to validate with a scenario more like #1971 - for example:

        [Theory]
        [InlineData(true)]
        [InlineData(false)]
        public void OleDbParamFilterFails(bool yourNewSettingHere)
        {
            var oldValue = SqlMapper.Settings.YourNewSettingHere;
            try
            {
                SqlMapper.Settings.YourNewSettingHere = yourNewSettingHere;

                if (yourNewSettingHere) // OLE DB parameter support enabled; can false-positive
                {
                    Assert.Throws<NotImplementedException>(() => GetValue(connection));
                }
                else // OLE DB parameter support disabled; more reliable
                {
                    Assert.Equal("this ? could be awkward", GetValue(connection));
                }
            }
            finally
            {
                SqlMapper.Settings.YourNewSettingHere = oldValue;
            }

            static string GetValue(DbConnection connection)
                => connection.QuerySingle<string>("select 'this ? could be awkward'",
                    new TypeWithDodgyProperties());
        }
        class TypeWithDodgyProperties
        {
            public string Name => throw new NotSupportedException();
        }

I haven't executed that at all, note

@Giorgi Giorgi closed this Oct 10, 2023
@Giorgi Giorgi deleted the Skip-OleDb-Check branch October 10, 2023 14:40
@Giorgi Giorgi reopened this Oct 10, 2023
@Giorgi Giorgi requested a review from mgravell October 10, 2023 15:29
@mgravell
Copy link
Member

looks good; can you add a release-note entry? for an example see index.md here: a37b151#diff-b4d68dc855d0f9476d3f2ee343853bd21bf82ea9960d0cf06661baa244439dd6

@mgravell
Copy link
Member

also, can you confirm you're free and willing to contribute this code irrevocably etc in line with the project license, i.e. "don't sue me later"

@mgravell
Copy link
Member

Note: re the pseudo-positional parameter support (#1914) - that's one that Dapper.Analyzer will definitely be helping with ASAP; the problem is that we don't want to have to constantly parse SQL, so for @foo and ?foo?, at runtime we kinda have to do things the "probably" way; however, Dapper.Analyzer can take the time at build to do a full parse, find the actual parameters, and offer a warning and advice on workarounds. So if that one is hurting you: worry not!

@Giorgi
Copy link
Contributor Author

Giorgi commented Oct 10, 2023

looks good; can you add a release-note entry? for an example see index.md here: a37b151#diff-b4d68dc855d0f9476d3f2ee343853bd21bf82ea9960d0cf06661baa244439dd6

Under the ### unreleased section?

@Giorgi
Copy link
Contributor Author

Giorgi commented Oct 10, 2023

also, can you confirm you're free and willing to contribute this code irrevocably etc in line with the project license, i.e. "don't sue me later"

I confirm

@mgravell mgravell merged commit 19193b5 into DapperLib:main Oct 11, 2023
1 check passed
@mgravell
Copy link
Member

Merged with thanks

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.

2 participants