Fix a number of broken connection detection scenarios.

This commit is contained in:
Akshay Joshi 2018-03-21 08:38:18 +00:00 committed by Dave Page
parent d358369ed5
commit 637f3b9d1a
12 changed files with 191 additions and 110 deletions

View File

@ -444,19 +444,26 @@ define('pgadmin.dashboard', [
pgAdmin.Dashboard.render_chart(container, data, dataset, sid, did, url, options, counter, refresh);
},
error: function(xhr) {
var err = $.parseJSON(xhr.responseText),
msg = err.errormsg,
cls;
let err = '';
let msg = '';
let cls = 'info';
if (xhr.readyState === 0) {
msg = gettext('Not connected to the server or the connection to the server has been closed.');
} else {
err = JSON.parse(xhr.responseText);
msg = err.errormsg;
// If we get a 428, it means the server isn't connected
if (xhr.status == 428) {
if (xhr.status === 428) {
if (_.isUndefined(msg) || _.isNull(msg)) {
msg = gettext('Please connect to the selected server to view the graph.');
}
cls = 'info';
} else {
msg = gettext('An error occurred whilst rendering the graph.');
cls = 'danger';
}
}
$(container).addClass('graph-error');
$(container).html(
@ -576,19 +583,26 @@ define('pgadmin.dashboard', [
filter.search();
},
error: function(model, xhr) {
var err = $.parseJSON(xhr.responseText),
msg = err.errormsg,
cls;
let err = '';
let msg = '';
let cls = 'info';
if (xhr.readyState === 0) {
msg = gettext('Not connected to the server or the connection to the server has been closed.');
} else {
err = JSON.parse(xhr.responseText);
msg = err.errormsg;
// If we get a 428, it means the server isn't connected
if (xhr.status == 428) {
if (xhr.status === 428) {
if (_.isUndefined(msg) || _.isNull(msg)) {
msg = gettext('Please connect to the selected server to view the table.');
}
cls = 'info';
} else {
msg = gettext('An error occurred whilst rendering the table.');
cls = 'danger';
}
}
// Replace the content with the error, if not already present. Always update the message
if (!$(container).hasClass('grid-error')) {

View File

@ -52,7 +52,7 @@ class ExecuteQuery {
}, self.sqlServerObject.POLL_FALLBACK_TIME());
}
execute(sqlStatement, explainPlan) {
execute(sqlStatement, explainPlan, connect) {
// If it is an empty query, do nothing.
if (sqlStatement.length <= 0) return;
@ -63,11 +63,8 @@ class ExecuteQuery {
const sqlStatementWithAnalyze = ExecuteQuery.prepareAnalyzeSql(sqlStatement, explainPlan);
self.initializeExecutionOnSqlEditor(sqlStatementWithAnalyze);
service.post(
url_for('sqleditor.query_tool_start', {
'trans_id': self.sqlServerObject.transId,
}),
this.generateURLReconnectionFlag(connect),
JSON.stringify(sqlStatementWithAnalyze),
{headers: {'Content-Type': 'application/json'}})
.then(function (result) {
@ -90,11 +87,22 @@ class ExecuteQuery {
self.sqlServerObject._highlight_error(httpMessageData.data.result);
}
}).catch(function (error) {
self.onExecuteHTTPError(error.response.data);
self.onExecuteHTTPError(error);
}
);
}
generateURLReconnectionFlag(shouldReconnect) {
let url = url_for('sqleditor.query_tool_start', {
'trans_id': this.sqlServerObject.transId,
});
if (shouldReconnect) {
url += '?connect=1';
}
return url;
}
poll() {
const self = this;
let service = axios.create({});
@ -129,18 +137,21 @@ class ExecuteQuery {
}
).catch(
error => {
const errorData = error.response.data;
// Enable/Disable query tool button only if is_query_tool is true.
self.sqlServerObject.resetQueryHistoryObject(self.sqlServerObject);
self.loadingScreen.hide();
if (self.sqlServerObject.is_query_tool) {
self.enableSQLEditorButtons();
}
if (ExecuteQuery.wasConnectionLostToServer(errorData)) {
if (ExecuteQuery.wasConnectionLostToPythonServer(error.response)) {
self.handleConnectionToServerLost();
return;
}
const errorData = error.response.data;
if (self.userManagement.is_pga_login_required(errorData)) {
return self.userManagement.pga_login();
}
@ -159,7 +170,7 @@ class ExecuteQuery {
$('#btn-flash').prop('disabled', true);
this.sqlServerObject.query_start_time = new Date();
if(typeof sqlStatement === 'object') {
if (typeof sqlStatement === 'object') {
this.sqlServerObject.query = sqlStatement['sql'];
} else {
this.sqlServerObject.query = sqlStatement;
@ -182,39 +193,36 @@ class ExecuteQuery {
this.loadingScreen.hide();
this.enableSQLEditorButtons();
if (ExecuteQuery.wasConnectionLostToServer(httpMessage)) {
if (ExecuteQuery.wasConnectionLostToPythonServer(httpMessage.response)) {
this.handleConnectionToServerLost();
return;
}
if (this.userManagement.is_pga_login_required(httpMessage)) {
if (this.userManagement.is_pga_login_required(httpMessage.response)) {
this.sqlServerObject.save_state('execute', [this.explainPlan]);
this.userManagement.pga_login();
}
if (transaction.is_new_transaction_required(httpMessage)) {
if (transaction.is_new_transaction_required(httpMessage.response)) {
this.sqlServerObject.save_state('execute', [this.explainPlan]);
this.sqlServerObject.init_transaction();
}
let msg = httpMessage.errormsg;
if (httpMessage.responseJSON !== undefined) {
if (httpMessage.responseJSON.errormsg !== undefined) {
msg = httpMessage.responseJSON.errormsg;
}
if (httpMessage.status === 503 && httpMessage.responseJSON.info !== undefined &&
httpMessage.responseJSON.info === 'CONNECTION_LOST') {
setTimeout(function () {
if (this.wasDatabaseConnectionLost(httpMessage)) {
this.sqlServerObject.save_state('execute', [this.explainPlan]);
this.sqlServerObject.handle_connection_lost(false, httpMessage);
});
}
}
let msg = httpMessage.response.data.errormsg;
this.sqlServerObject.update_msg_history(false, msg);
}
wasDatabaseConnectionLost(httpMessage) {
return httpMessage.response.status === 503 &&
httpMessage.response.data.info !== undefined &&
httpMessage.response.data.info === 'CONNECTION_LOST';
}
removeGridViewMarker() {
if (this.sqlServerObject.gridView.marker) {
this.sqlServerObject.gridView.marker.clear();
@ -236,8 +244,8 @@ class ExecuteQuery {
$('#btn-cancel-query').prop('disabled', false);
}
static wasConnectionLostToServer(errorMessage) {
return errorMessage.readyState === 0;
static wasConnectionLostToPythonServer(httpResponse) {
return _.isUndefined(httpResponse) || _.isUndefined(httpResponse.data);
}
handleConnectionToServerLost() {

View File

@ -8,6 +8,15 @@
//////////////////////////////////////////////////////////////////////////
export function is_new_transaction_required(xhr) {
/* If responseJSON is undefined then it could be object of
* axios(Promise HTTP) response, so we should check accordingly.
*/
if (xhr.responseJSON === undefined && xhr.data !== undefined) {
return xhr.status === 404 && xhr.data &&
xhr.data.info &&
xhr.data.info === 'DATAGRID_TRANSACTION_REQUIRED';
}
return xhr.status === 404 && xhr.responseJSON &&
xhr.responseJSON.info &&
xhr.responseJSON.info === 'DATAGRID_TRANSACTION_REQUIRED';

View File

@ -298,8 +298,10 @@ def start_query_tool(trans_id):
request.data, request.args, request.form
)
connect = 'connect' in request.args and request.args['connect'] == '1'
return StartRunningQuery(blueprint, current_app).execute(
sql, trans_id, session
sql, trans_id, session, connect
)

View File

@ -1847,13 +1847,21 @@ define('tools.querytool', [
},
handle_connection_lost: function(create_transaction, xhr) {
var self= this;
if (xhr.responseJSON.data && !xhr.responseJSON.data.conn_id) {
/* If responseJSON is undefined then it could be object of
* axios(Promise HTTP) response, so we should check accordingly.
*/
if (xhr.responseJSON !== undefined &&
xhr.responseJSON.data && !xhr.responseJSON.data.conn_id) {
// if conn_id is null then this is maintenance db.
// so attempt connection connect without prompt.
self.init_connection(create_transaction);
this.init_connection(create_transaction);
} else if (xhr.data !== undefined &&
xhr.data.data && !xhr.data.data.conn_id) {
// if conn_id is null then this is maintenance db.
// so attempt connection connect without prompt.
this.init_connection(create_transaction);
} else {
self.warn_before_continue();
this.warn_before_continue();
}
},
warn_before_continue: function() {
@ -3730,7 +3738,7 @@ define('tools.querytool', [
// This function will fetch the sql query from the text box
// and execute the query.
execute: function(explain_prefix) {
execute: function(explain_prefix, shouldReconnect=false) {
var self = this,
sql = '';
@ -3747,7 +3755,7 @@ define('tools.querytool', [
sql = self.gridView.query_tool_obj.getValue();
const executeQuery = new ExecuteQuery.ExecuteQuery(this, pgAdmin.Browser.UserManagement);
executeQuery.execute(sql, explain_prefix);
executeQuery.execute(sql, explain_prefix, shouldReconnect);
},
/* This function is used to highlight the error line and

View File

@ -42,6 +42,6 @@ class StartQueryTool(BaseTestGenerator):
self.assertEquals(response.status, '200 OK')
self.assertEquals(response.data, b'some result')
StartRunningQuery_execute_mock \
.assert_called_with('transformed sql', 1234, ANY)
.assert_called_with('transformed sql', 1234, ANY, False)
extract_sql_from_network_parameters_mock \
.assert_called_with(b'"some sql statement"', ANY, ANY)

View File

@ -36,7 +36,7 @@ class StartRunningQuery:
self.connection_id = str(random.randint(1, 9999999))
self.logger = logger
def execute(self, sql, trans_id, http_session):
def execute(self, sql, trans_id, http_session, connect=False):
session_obj = StartRunningQuery.retrieve_session_information(
http_session,
trans_id
@ -68,7 +68,7 @@ class StartRunningQuery:
return internal_server_error(errormsg=str(e))
# Connect to the Server if not connected.
if not conn.connected():
if connect and not conn.connected():
status, msg = conn.connect()
if not status:
self.logger.error(msg)
@ -108,7 +108,6 @@ class StartRunningQuery:
self.connection_id = conn_id
def __execute_query(self, conn, session_obj, sql, trans_id, trans_obj):
if conn.connected():
# on successful connection set the connection id to the
# transaction object
trans_obj.set_connection_id(self.connection_id)
@ -136,11 +135,7 @@ class StartRunningQuery:
if StartRunningQuery.is_rollback_statement_required(trans_obj,
conn):
conn.execute_void("ROLLBACK;")
else:
status = False
result = gettext(
'Not connected to server or connection with the server has '
'been closed.')
return result, status
@staticmethod

View File

@ -21,6 +21,7 @@ else:
from unittest.mock import patch, MagicMock
get_driver_exception = Exception('get_driver exception')
get_connection_lost_exception = Exception('Unable to connect to server')
class StartRunningQueryTest(BaseTestGenerator):
@ -38,6 +39,7 @@ class StartRunningQueryTest(BaseTestGenerator):
),
pickle_load_return=None,
get_driver_exception=False,
get_connection_lost_exception=False,
manager_connection_exception=None,
is_connected_to_server=False,
@ -67,6 +69,7 @@ class StartRunningQueryTest(BaseTestGenerator):
),
pickle_load_return=None,
get_driver_exception=False,
get_connection_lost_exception=False,
manager_connection_exception=None,
is_connected_to_server=False,
@ -97,6 +100,7 @@ class StartRunningQueryTest(BaseTestGenerator):
),
pickle_load_return=None,
get_driver_exception=False,
get_connection_lost_exception=False,
manager_connection_exception=None,
is_connected_to_server=False,
@ -131,6 +135,7 @@ class StartRunningQueryTest(BaseTestGenerator):
pickle_load_return=MagicMock(conn_id=1,
update_fetched_row_cnt=MagicMock()),
get_driver_exception=True,
get_connection_lost_exception=False,
manager_connection_exception=None,
is_connected_to_server=False,
@ -161,6 +166,7 @@ class StartRunningQueryTest(BaseTestGenerator):
update_fetched_row_cnt=MagicMock()
),
get_driver_exception=False,
get_connection_lost_exception=False,
manager_connection_exception=ConnectionLost('1', '2', '3'),
is_connected_to_server=False,
@ -188,6 +194,7 @@ class StartRunningQueryTest(BaseTestGenerator):
update_fetched_row_cnt=MagicMock()
),
get_driver_exception=False,
get_connection_lost_exception=True,
manager_connection_exception=None,
is_connected_to_server=False,
@ -202,7 +209,7 @@ class StartRunningQueryTest(BaseTestGenerator):
expect_internal_server_error_called_with=dict(
errormsg='Unable to connect to server'
),
expected_logger_error='Unable to connect to server',
expected_logger_error=get_connection_lost_exception,
expect_execute_void_called_with='some sql',
)),
('When server is connected and start query async request, '
@ -223,6 +230,7 @@ class StartRunningQueryTest(BaseTestGenerator):
can_filter=lambda: True
),
get_driver_exception=False,
get_connection_lost_exception=False,
manager_connection_exception=None,
is_connected_to_server=True,
@ -265,6 +273,7 @@ class StartRunningQueryTest(BaseTestGenerator):
can_filter=lambda: True
),
get_driver_exception=False,
get_connection_lost_exception=False,
manager_connection_exception=None,
is_connected_to_server=True,
@ -307,6 +316,7 @@ class StartRunningQueryTest(BaseTestGenerator):
can_filter=lambda: True
),
get_driver_exception=False,
get_connection_lost_exception=False,
manager_connection_exception=None,
is_connected_to_server=True,
@ -349,6 +359,7 @@ class StartRunningQueryTest(BaseTestGenerator):
can_filter=lambda: True
),
get_driver_exception=False,
get_connection_lost_exception=False,
manager_connection_exception=None,
is_connected_to_server=True,
@ -431,6 +442,8 @@ class StartRunningQueryTest(BaseTestGenerator):
manager = self.__create_manager()
if self.get_driver_exception:
get_driver_mock.side_effect = get_driver_exception
elif self.get_connection_lost_exception:
get_driver_mock.side_effect = get_connection_lost_exception
else:
get_driver_mock.return_value = MagicMock(
connection_manager=lambda session_id: manager)

View File

@ -130,9 +130,18 @@ define([
},
is_pga_login_required(xhr) {
return xhr.status == 401 && xhr.responseJSON &&
/* If responseJSON is undefined then it could be object of
* axios(Promise HTTP) response, so we should check accordingly.
*/
if (xhr.responseJSON === undefined && xhr.data !== undefined) {
return xhr.status === 401 && xhr.data &&
xhr.data.info &&
xhr.data.info === 'PGADMIN_LOGIN_REQUIRED';
}
return xhr.status === 401 && xhr.responseJSON &&
xhr.responseJSON.info &&
xhr.responseJSON.info == 'PGADMIN_LOGIN_REQUIRED';
xhr.responseJSON.info === 'PGADMIN_LOGIN_REQUIRED';
},
// Callback to draw pgAdmin4 login dialog.

View File

@ -19,6 +19,7 @@ from flask_babel import gettext
from pgadmin.utils.crypto import decrypt
from .connection import Connection
from pgadmin.model import Server
from pgadmin.utils.exception import ConnectionLost
class ServerManager(object):

View File

@ -42,6 +42,7 @@ describe('ExecuteQuery', () => {
'_init_polling_flags',
'save_state',
'init_transaction',
'handle_connection_lost',
]);
sqlEditorMock.transId = 123;
sqlEditorMock.rows_affected = 1000;
@ -49,6 +50,10 @@ describe('ExecuteQuery', () => {
isNewTransactionRequiredMock = spyOn(transaction, 'is_new_transaction_required');
});
afterEach(() => {
networkMock.restore();
});
describe('#poll', () => {
let cancelButtonSpy;
let response;
@ -62,9 +67,6 @@ describe('ExecuteQuery', () => {
executeQuery.delayedPoll = jasmine.createSpy('ExecuteQuery.delayedPoll');
});
afterEach(() => {
});
context('when SQLEditor is the query tool', () => {
beforeEach(() => {
sqlEditorMock.is_query_tool = true;
@ -569,8 +571,7 @@ describe('ExecuteQuery', () => {
describe('when cannot reach the Python Server', () => {
beforeEach(() => {
response = {readyState: 0};
networkMock.onGet('/sqleditor/query_tool/poll/123').reply(401, response);
networkMock.onGet('/sqleditor/query_tool/poll/123').reply(404, undefined);
executeQuery.poll();
});
@ -972,8 +973,7 @@ describe('ExecuteQuery', () => {
describe('when cannot reach the Python Server', () => {
beforeEach(() => {
response = {readyState: 0};
networkMock.onGet('/sqleditor/query_tool/poll/123').reply(401, response);
networkMock.onGet('/sqleditor/query_tool/poll/123').reply(404, undefined);
executeQuery.poll();
});
@ -1143,9 +1143,9 @@ describe('ExecuteQuery', () => {
describe('when HTTP return 200', () => {
describe('when backend informs that query started successfully', () => {
beforeEach(() => {
networkMock.onAny('/sqleditor/query_tool/start/123').reply(200, response);
networkMock.onPost('/sqleditor/query_tool/start/123?connect=1').reply(200, response);
pollSpy = spyOn(executeQuery, 'delayedPoll');
executeQuery.execute('some sql query', '');
executeQuery.execute('some sql query', '', true);
});
it('should changes the loading message to "Waiting for the query execution to complete"', (done) => {
@ -1310,10 +1310,7 @@ describe('ExecuteQuery', () => {
describe('when cannot reach the Python Server', () => {
beforeEach(() => {
response = {
readyState: 0,
};
networkMock.onAny('/sqleditor/query_tool/start/123').reply(500, response);
networkMock.onAny('/sqleditor/query_tool/start/123').reply(500, undefined);
executeQuery.execute('some sql query', '');
@ -1657,7 +1654,32 @@ describe('ExecuteQuery', () => {
}, 0);
});
});
describe('when connection to database is lost', () => {
beforeEach(() => {
isNewTransactionRequiredMock.and.returnValue(false);
response.info = 'CONNECTION_LOST';
networkMock.onAny('/sqleditor/query_tool/start/123').reply(503, response);
executeQuery.execute('some sql query', '');
});
it('saves state', () => {
setTimeout(() => {
expect(sqlEditorMock.save_state).toHaveBeenCalledWith(
'execute',
['']
);
}, 0);
});
it('calls handle_connection_lost', () => {
setTimeout(() => {
expect(sqlEditorMock.handle_connection_lost).toHaveBeenCalled();
}, 0);
});
});
});
});
});

View File

@ -19,7 +19,7 @@ describe('#is_new_transaction_required', () => {
});
describe('when status is 404', () => {
describe('when responseJSON is not present', () => {
describe('when data is not present', () => {
it('should return false', () => {
expect(is_new_transaction_required({
status: 404,
@ -27,22 +27,22 @@ describe('#is_new_transaction_required', () => {
});
});
describe('when responseJSON is present', () => {
describe('when info is not present inside responseJSON', () => {
describe('when data is present', () => {
describe('when info is not present inside data', () => {
it('should return false', () => {
expect(is_new_transaction_required({
status: 404,
responseJSON: {},
data: {},
})).toBeFalsy();
});
});
describe('when info is present inside responseJSON', () => {
describe('when info is present inside data', () => {
describe('when info value is not "DATAGRID_TRANSACTION_REQUIRED"', () => {
it('should return false', () => {
expect(is_new_transaction_required({
status: 404,
responseJSON: {
data: {
info: 'some information',
},
})).toBe(false);
@ -53,7 +53,7 @@ describe('#is_new_transaction_required', () => {
it('should return false', () => {
expect(is_new_transaction_required({
status: 404,
responseJSON: {
data: {
info: 'DATAGRID_TRANSACTION_REQUIRED',
},
})).toBe(true);