dibley1973 / StoredProcedureFramework

A .Net framework for calling stored procedures
MIT License
4 stars 2 forks source link

Return parameter value not set #8

Closed davegra closed 8 years ago

davegra commented 8 years ago

A parameter defined as ParameterDirection.ReturnValue does not get set to the return value of the stored procedure. This was working in 0.1 but no longer works in 0.9.

The IdentifyOutputParameters method in OutputParameterValueProcessor should have an additional OR term for ParameterDirection.ReturnValue. Adding this to the where clause fixed this but I have not done any other checking or testing.

Thanks for the very useful framework!

Dave

dibley1973 commented 8 years ago

Hi @davegra ,

Thank you for your feedback. My bad for letting that slip through.

I am out of town until Thursday so I cannot promise to have a look until then. However I do have my laptop with me, so if I get chance to spin the framework up on it then I will see what I can do.

dibley1973 commented 8 years ago

I do have an integration test for this, which I will detail below and that is passing. Therefore I may need a little more information, like what datatype is the return value, what ordinal in the params is it? I can then try and create a test which shows this failing.

My current integration test is:

    [TestMethod]
    public void CountCharsInOutputParameterStoredProcedure_WithOutputParamatersAndNoReturnType_ReturnsOutputParamtersCorrectly()
    {
        // ARRANGE
        const string expectedValue1 = "MonkeyTube";
        const int initialValue2 = 0;
        int expectedvalue2 = expectedValue1.Length;
        var parameters = new CountCharsInOutputParameterStoredProcedure.Parameter
        {
            Value1 = expectedValue1,
            Value2 = initialValue2
        };
        var procedure = new CountCharsInOutputParameterStoredProcedure(parameters);

        // ACT
        Connection.ExecuteStoredProcedure(procedure);

        // ASSERT
        Assert.AreEqual(expectedvalue2, parameters.Value2);
    }

The code for the stored procedure is...

    public CountCharsInOutputParameterStoredProcedure(Parameter parameters)
        : base(parameters)
    {
    }

    public class Parameter
    {
        [Size(100)]
        public string Value1 { get; set; }

        [Direction(ParameterDirection.Output)]
        public int Value2 { get; set; }
    }

and the stored procedure itself is...

CREATE PROCEDURE [dbo].[CountCharsInOutputParameterStoredProcedure]
    @Value1 varchar(100)
,   @Value2 int output
AS
BEGIN
    -- SET NOCOUNT ON added to prevent extra result sets from
    -- interfering with SELECT statements.
    SET NOCOUNT ON;

    -- Does nothing!
    SET @Value2 = LEN(@Value1);
END
dibley1973 commented 8 years ago

@davegra ,

If you can please let me know what key differences your procedure has then I can take a look as soon as i get time.

Thanks and regards, Duane.

dibley1973 commented 8 years ago

@davegra ,

Sorry: I have re-read what you have written and can see the ParameterDirection.Output.ReturnValue is missing as you say.

davegra commented 8 years ago

Hi Duane,

I think you have it but here is the code snippet from my parameters declaration:

internal class IntercompanyTransferLogAddParameters

{

    [Direction(ParameterDirection.ReturnValue), DbType(SqlDbType.Int)]

    public int Id { get; set; }

    [DbType(SqlDbType.VarChar), Size(60)]

    public string TransferType { get; set; }

Thanks for the quick replies!

Regards

Dave

From: Duane Dibley [mailto:notifications@github.com] Sent: January 25, 2016 1:02 PM To: dibley1973/StoredProcedureFramework StoredProcedureFramework@noreply.github.com Cc: dgml dgml@cottonwood.ca Subject: Re: [StoredProcedureFramework] Return parameter value not set (#8)

@davegra https://github.com/davegra ,

Sorry: I have re-read what you have written and can see the ParameterDirection.Output.ReturnValue is missing as you say.

— Reply to this email directly or view it on GitHub https://github.com/dibley1973/StoredProcedureFramework/issues/8#issuecomment-174622242 .

dibley1973 commented 8 years ago

Added following test for this issue:

    [TestMethod]
    public void Issue8_CountCharsInReturnParameterStoredProcedure_WithReturnParamatersAndNoReturnType_ReturnsOutputParamtersCorrectly()
    {
        // ARRANGE
        const string expectedValue1 = "MonkeyTube";
        const int initialValue2 = 0;
        int expectedvalue2 = expectedValue1.Length;
        var parameters = new CountCharsInReturnParameterStoredProcedure.Parameter
        {
            Value1 = expectedValue1,
            Value2 = initialValue2
        };
        var procedure = new CountCharsInReturnParameterStoredProcedure(parameters);

        // ACT
        Connection.ExecuteStoredProcedure(procedure);

        // ASSERT
        Assert.AreEqual(expectedvalue2, parameters.Value2);
    }
dibley1973 commented 8 years ago

Added new stored proc in integration test DB:

-- =============================================
-- Author:      Duane Wingett
-- Create date: 2015-08-25
-- Description: Returns the count of characters of first 
--              parameter in return paraemeter.
-- =============================================
ALTER PROCEDURE [dbo].[CountCharsInReturnParameterStoredProcedure]
    @Value1 varchar(100)
--,   @Value2 int output
AS
BEGIN
    -- SET NOCOUNT ON added to prevent extra result sets from
    -- interfering with SELECT statements.
    SET NOCOUNT ON;

    -- Does nothing!
    --SET @Value2 = LEN(@Value1);
    RETURN LEN(@Value1);
END
dibley1973 commented 8 years ago

New test failed...

Made changes to OutputParameterValueProcessor.IdentifyOutputParameters() method as per @davegra 's suggestion...

    private void IdentifyOutputParameters()
    {
        _outputSqlParameters = _sqlParameters
            .Where(sqlParameter => sqlParameter.Direction == ParameterDirection.InputOutput
                || sqlParameter.Direction == ParameterDirection.Output
                || sqlParameter.Direction == ParameterDirection.ReturnValue)
            .Select(sqlParameter => sqlParameter);
    }

Test now pass. Re-run all other tests and hand to change one more `` test

dibley1973 commented 8 years ago

@davegra - please can you check if this test correctly identifies and fixes the issue you have spotted? The change is checked into the main branch.

BTW, latest version does not need DbType attribute set for some of the basic type mappings now.

davegra commented 8 years ago

Hi Duane,

Test looks good and fix as well. Thanks for resolving this one.

Dave From: Duane Dibley [mailto:notifications@github.com] Sent: January 26, 2016 3:02 PM To: dibley1973/StoredProcedureFramework StoredProcedureFramework@noreply.github.com Cc: dgml dgml@cottonwood.ca Subject: Re: [StoredProcedureFramework] Return parameter value not set (#8)

@davegrahttps://github.com/davegra - please can you check if this test correctly identifies and fixes the issue you have spotted? The change is checked into the main branch.

BTW, latest version does not need DbType attribute set for some of the basic type mappings now.

— Reply to this email directly or view it on GitHubhttps://github.com/dibley1973/StoredProcedureFramework/issues/8#issuecomment-175228365.

dibley1973 commented 8 years ago

@davegra - No problem. I want to keep the product from going stale so will apply fixes as soon as I can given my available time. You had basically already found the line of code causing the issue for me, so you had done the hard work for me. I just needed to write a test that proved the existence of the bug, apply your fix, and then re-run the tests.

Thanks for raising the issue and supporting the product. BTW, did you find the code laid out well enough that understanding the code and finding the line in question was not a problem? Is there anything I can do to make the code clearer in that area?

dibley1973 commented 8 years ago

Closing issue as resolved.

davegra commented 8 years ago

Hi Duane,

The layout of the code was pretty easy to follow so no worries there. Names were very explanatory which always helps. Nothing came to mind that would improve the code as I was going through it.

It’s a great framework. I never liked coding with SQL Command/Connection objects and setting up parameters etc. each time I had a SP to call. Our data access policy states that we use SPs for all database access so anything that would simplify this is a great help. I also like the ability to strongly type everything. The recent support ( we started with 0.1 back in September 2015) for structured data parameters and multiple record sets is also very handy.

We ended up just using EF for the database context and not keeping any datasets in EF. I never liked the overhead of having to add EF to every project that has data access so have been replacing the code using DbContext with your framework.

Keep up the great work!

Regards

Dave

From: Duane Dibley [mailto:notifications@github.com] Sent: January 27, 2016 2:25 AM To: dibley1973/StoredProcedureFramework StoredProcedureFramework@noreply.github.com Cc: dgml dgml@cottonwood.ca Subject: Re: [StoredProcedureFramework] Return parameter value not set (#8)

@davegra https://github.com/davegra - No problem. I want to keep the product from going stale so will apply fixes as soon as I can given my available time. You had basically already found the line of code causing the issue for me, so you had done the hard work for me. I just needed to write a test that proved the existence of the bug, apply your fix, and then re-run the tests.

Thanks for raising the issue and supporting the product. BTW, did you find the code laid out well enough that understanding the code and finding the line in question was not a problem? Is there anything I can do to make the code clearer in that area?

— Reply to this email directly or view it on GitHub https://github.com/dibley1973/StoredProcedureFramework/issues/8#issuecomment-175477330 .

dibley1973 commented 8 years ago

@davegra - Thank you for the feedback. I carried out a lot of refactoring to try and make the code as readable and intention revealing as I can, so its great to hear you were able to follow it through relatively easily.

Like you I am not a fan of using E.F. for the "reading" of data as I am trying to split data reading from data writing in my projects, and like you prefer everything to be as strongly typed as possible. These were the key drivers for the whole project. I'm glad the framework is working well for you. enjoy, and please don not hesitate to raise any further issues if you find them! :)