Yesterday's blog post on the need for contracts for stored procedures caused a lot of comments and email. One of the most interesting comments came from Jamie Thomson regarding return values. Jamie's totally correct on this. Return values should be part of any contract.
I've been thinking further about how return values should be incorporated into a contract and initially thought it should be something like this:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 |
<em>CREATE OR ALTER PROCEDURE SomeSchema.SomeSproc (someparameters)</em> <em>WITH </em><em>CONTRACT SalesOrderHeaderAndDetails ENFORCED</em> <em> (ROWS OrderHeaders(SomeColumn INT, SomeOtherColumn NVARCHAR(50) NULL),</em> <em> OrderDetails(AnotherColumn INT, YetAnotherColumn INT, </em> <em> EvenYetAnotherColumn GEOGRAPHY),</em> <em> RETURNS INT,</em> <em> EXCEPTIONS NoSuchCustomer(50020,’No such Customer’),</em> <em> DuplicateOrder(50022,’That order already exists’)),</em> <em> EXECUTE AS (execution options here if needed)</em> |
I thought the values could be RETURNS INT or RETURNS NULL, but on reflection (no pun intended), I realized that in many cases it is necessary to resort to documentation to know what a stored procedure return value is. That would be eased if the return value also had a name as part of its metadata. So perhaps a more complete contract would look like:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 |
<em>CREATE OR ALTER PROCEDURE SomeSchema.SomeSproc (someparameters)</em> <em>WITH </em><em>CONTRACT SalesOrderHeaderAndDetails ENFORCED</em> <em> (ROWS OrderHeaders(SomeColumn INT, SomeOtherColumn NVARCHAR(50) NULL),</em> <em> OrderDetails(AnotherColumn INT, YetAnotherColumn INT, </em> <em> EvenYetAnotherColumn GEOGRAPHY),</em> <em> RETURNS OrderCount(INT),</em> <em> EXCEPTIONS NoSuchCustomer(50020,’No such Customer’),</em> <em> DuplicateOrder(50022,’That order already exists’)),</em> <em> EXECUTE AS (execution options here if needed)</em> |
The idea is that you could have a value like RETURNS SomeName(INT) or RETURNS NULL where there is no return value.
The feedback item is here: https://feedback.azure.com/d365community/idea/3b9b8fea-e74f-ec11-a819-0022484bf651
Greg, you and Jamie seem to be mixing up the usage of RETURN and OUTPUT. I don't see any mention here of OUTPUT parameters, which are in my mind a more important aspect of the contract, since they return data of varying data types, whereas a RETURN value is just supposed to return execution status, and be limited to INT.
Hi Aaron,
I'm thinking that OUTPUT parameters are already provided for adequately in the current definitions and metadata. But I think the lack of a name for a return value is an issue.
Regards,
Greg
Greg,
What you are describing is not new, it became mainstream in object oriented programming two decades ago. We had interfaces in early '90s. But before that we had catches which actually catch errors, and functions which run without huge performance hit; we had both in the '80s.
So how about fixing the disfunctional basics, the flaky catch and the slow UDF, before moving on to develop exciting new features?
Greg, what is the value of giving the return value a name, or requiring that one be given? Since I often see:
RETURN 0;
or just:
RETURN;
Can you explain exactly what would be gained from having a contract where the name of the return value was known? Or even of specifying its data type (what do you expect to gain from allowing data types other than INT)?
Lars, I agree, the UDF should certainly be a high-level work item, and I can only imagine there are improvements coming for the half-baked TRY/CATCH.
Hi Lars,
We all agree that there are other problematic areas of the product. But that's not an argument for fixing the ones that can be fixed.
Regards,
Greg
Hi Aaron,
Yes, I often see procs that just return a zero or nothing but I've also seen procs where the return value has a specific meaning ie: where people have used it as a form of output parameter. If that's the case, then you should be able to specify what on earth it is.
For example, I've seen insert procs where (for better or worse) the newly-inserted ID was returned as the return value of the proc. In that case, it would be good to have the metadata reflect that that's what the value actually is. I've seen others where update procs return the number of rows updated and yet others where a positive value is good and a negative value indicates some form of failure.
Regards,
Greg
Greg, sorry but I still disagree with you about return values. Just because you've seen them used to return data (which is what OUTPUT parameteres are for), doesn't make it a good idea, one we should encourage, and certainly not one which we should change the product to accommodate. IMHO. I have always believed that return values are used solely and explicitly for returning execution status / error codes and still don't agree that we should modify metadata functionality to suit the way they are misused by a portion of users.
Hi Aaron,
We'll have to differ on that one. Even if you only use them for returning an execution status, wouldn't it be good if the metadata indicated that that's what it was?
Regards,
Greg
Hi Greg,
I hear what you are saying: "fixing the ones that can be fixed".
Do you imply that the unfinished error handling cannot be finished at all?
If yes, why?
If RETURN were used consistently the way it was designed, instead of being misused, there wouldn't be any need to indicate that it is returning status, because there shouldn't be any other options. Your use of RETURN OrderCount indicates that it is returning *data* and to me this is not appropriate. Why not use an OUTPUT parameter for that data?
Hi Aaron,
Do you think there should be a consistent standard for indicating execution status?
There are two schools of thought on returning execution status. The problem with having an execution status that indicates a problem is that it can be ignored. In general, I prefer to have an exception raised when an error occurs as the client can't just ignore that.
Regardless, if the return value represents *something*, even if it's an execution status, I'd prefer it to have a name that says what it is.
Regards,
Greg
Greg,
I don't think RETURN always has to be used. If you prefer always using a RAISERROR, then you can do that, and you haven't changed your need around naming RETURN values. I like the flexibility that you can use RETURN values to consume and report errors from the parent level, instead of having to include all error logging etc. in every single module.