-
Notifications
You must be signed in to change notification settings - Fork 175
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
Calling a stored procedure in MSSQL - special handling of OUT params #219
Conversation
…in a separate list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a very small review. Looks like a good plan for implementing it. From what I read it seemed like the major difference was instead of using a CURSOR like param similar to Mysql's implementation, you went with an optional array of Output params to append onto the existing param list.
I think this probably avoids additional complexity for the user who's only using non-output params, and it avoids something like polluting the namespace in params. At least in theory it's quite hard to try and insert the word CURSOR into a Mysql db using a stored procedure in a robot framework, so avoiding that in this implementation may be good. The only con is being a bit prescriptive about where in the order of variables outputs have to go, but I believe that's probably worth it in a majority of cases.
| ${result sets} = [[('Franz Allan',), ('Jerry',)], [('See',), ('Schneider',)]] | ||
|
||
=== MS SQL procedure returning result sets AND OUT params === | ||
This case is *not fully supported* by the library - the OUT params won't be fetched. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is inaccurate. Output params are fetched, as seen in the first value with the result:
${param values} = ('give me 1', 1)
In lines 576-577
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Fixed the line 776 - the returned result sets
are empty here.
if db_connection.module_name == "pymssql": | ||
mssql = importlib.import_module("pymssql") | ||
spParams = spParams.copy() | ||
for param in additional_output_params: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that output params always being spread into the param list at the end of the list means that only SQL stored procedures which have their outputs at the end are usable here. That's generally best practice for writing a stored procedure, but do you want to be requiring library users to refactor their stored procedures to follow best practices in order to be testable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is a good point.
Do you mean a case when a procedure takes, let's say, 4 params and they're not listed like IN, IN, OUT, OUT
, but ÌN, OUT, IN, OUT
?
Do you have any ideas how to solve it?
The main reason, why I decided to put the output params in a separate argument, is that you need to know the data types. When using just a prefix like CURSOR or OUT etc., they are all strings. |
The pymssql driver doesn't natively support getting the OUT parameter values after calling a procedure.
This requires special handling of OUT parameters using the
additional_output_params
argument.Calling the procedure in Robot Framework requires putting the IN parameters as usual in the
spParams
argument, but the sample values of OUT parameters must be put in the argumentadditional_output_params
.The library uses the sample values in the
additional_output_params
list to determine the number and the type of OUT parameters - so they are type-sensitive, the type must be the same as in the procedure itself.