Removing redundant method in connection service. #169
Conversation
| sys.stderr.write( | ||
| u'\nIncorrect json rpc request. Connection not successful.') | ||
| return None | ||
|
|
There was a problem hiding this comment.
These output the connection error messages to the terminal. Without these the user would'nt know why the connection did not succeed if unable to connect
There was a problem hiding this comment.
Sure, I can surface error messages for the client to output to the console. For this particular error how do we know the lack of a result means the json rpc request was invalid? also this message doesn't seem to be actionable for the user since we are forming the request.
There was a problem hiding this comment.
We would get back a result if toolsservice was able to understand our request. If we don't have a result parameter it means something went wrong in jsonrpc request.
In reply to: 168625939 [](ancestors = 168625939)
mssqlcli/mssqlcliclient.py
Outdated
| if response.error_message: | ||
| error_messages.append(u'Error message: {}'.format(response.error_message)) | ||
| if response.messages: | ||
| error_messages.append(u'Additional message: {}'.format(response.messages)) |
There was a problem hiding this comment.
Do not add additional messages. This has the entire call stack of SqlClient. That is why previously we just output the error and only logged the message.
|
Did you try this with an incorrect login to see what is output to the user? |
| @@ -173,8 +173,6 @@ def verify_connection_service_response(self, | |||
| elif isinstance(response, connectionservice.ConnectionCompleteEvent): | |||
| if response.connection_id: | |||
| complete_event += 1 | |||
There was a problem hiding this comment.
can you assert the connection_id here? #Closed
There was a problem hiding this comment.
* Removing redundant method in connection service. * Updating mssqlcliclient to surface error messages to it's callers. * Removed copyright statement in test and flake8 format. * Converting output of stack trace to be logged instead.
* Removing redundant method in connection service. * Updating mssqlcliclient to surface error messages to it's callers. * Removed copyright statement in test and flake8 format. * Converting output of stack trace to be logged instead.
* Removing redundant method in connection service. * Updating mssqlcliclient to surface error messages to it's callers. * Removed copyright statement in test and flake8 format. * Converting output of stack trace to be logged instead.
Connection service should maintain a single responsibility of supplying connection requests and handling responses. It should not output any error messages to the console as that should be the responsibility of the client (mssqlcliclient).
After reviewing the handle_connection_response method it only served as a logging/console writing function. This was discovered during bug bash that if any errors happened on the completion refresher thread, the error would be populated to the user which is not useful.
The background thread for completion refresher is allowed to fail and should not interrupt the user's session if it does.