From b4118a4662b8f3665fe4ef5c46dca1a61f5d9f13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Ri=C3=9Fe?= <9308656+matrss@users.noreply.github.com> Date: Wed, 31 Jul 2024 08:17:05 +0200 Subject: [PATCH 1/2] Fix inconsistent SocketsManager objects (#2444) On import of mslib.mscolab.server initialize_managers was called once, hooking events up to handlers in a SocketsManager instance and initializing the Flask app with the Flask-SocketIO extension. Then, in handle_start, initialize_managers was called a second time, creating another SocketsManager object, overwriting the instance saved with the SocketIO object, and trying to initialize the Flask app with it again. The last step does not work though, a Flask app can only be initialized once with each extension (i.e. the sockio.init_app(app) call can only be done once per Flask app object). What this led to is that the Flask-SocketIO extension was using the first SocketsManager object to handle incoming events, while the server side was using the second SocketsManager object when it called methods on sockio.sm. This changes the startup process to do the initialization just once. --- mslib/mscolab/mscolab.py | 6 ++---- mslib/mscolab/server.py | 8 ++++---- mslib/mscolab/sockets_manager.py | 2 +- tests/_test_mscolab/test_server.py | 8 ++++---- tests/_test_mscolab/test_sockets_manager.py | 4 ++-- tests/fixtures.py | 7 +++---- 6 files changed, 16 insertions(+), 19 deletions(-) diff --git a/mslib/mscolab/mscolab.py b/mslib/mscolab/mscolab.py index ad6dfe46b..b0a79969d 100644 --- a/mslib/mscolab/mscolab.py +++ b/mslib/mscolab/mscolab.py @@ -45,15 +45,13 @@ def handle_start(args): - from mslib.mscolab.server import APP, initialize_managers, start_server + from mslib.mscolab.server import APP, sockio, cm, fm, start_server setup_logging(args) logging.info("MSS Version: %s", __version__) logging.info("Python Version: %s", sys.version) logging.info("Platform: %s (%s)", platform.platform(), platform.architecture()) logging.info("Launching MSColab Server") - - app, sockio, cm, fm = initialize_managers(APP) - start_server(app, sockio, cm, fm) + start_server(APP, sockio, cm, fm) def confirm_action(confirmation_prompt): diff --git a/mslib/mscolab/server.py b/mslib/mscolab/server.py index 7e6651161..cf5575874 100644 --- a/mslib/mscolab/server.py +++ b/mslib/mscolab/server.py @@ -46,7 +46,7 @@ from mslib.mscolab.conf import mscolab_settings, setup_saml2_backend from mslib.mscolab.models import Change, MessageType, User -from mslib.mscolab.sockets_manager import setup_managers +from mslib.mscolab.sockets_manager import _setup_managers from mslib.mscolab.utils import create_files, get_message_dict from mslib.utils import conditional_decorator from mslib.index import create_app @@ -124,8 +124,8 @@ def confirm_token(token, expiration=3600): return email -def initialize_managers(app): - sockio, cm, fm = setup_managers(app) +def _initialize_managers(app): + sockio, cm, fm = _setup_managers(app) # initializing socketio and db app.wsgi_app = socketio.Middleware(socketio.server, app.wsgi_app) sockio.init_app(app) @@ -133,7 +133,7 @@ def initialize_managers(app): return app, sockio, cm, fm -_app, sockio, cm, fm = initialize_managers(APP) +_app, sockio, cm, fm = _initialize_managers(APP) def check_login(emailid, password): diff --git a/mslib/mscolab/sockets_manager.py b/mslib/mscolab/sockets_manager.py index 85bf6b569..20b1e6b82 100644 --- a/mslib/mscolab/sockets_manager.py +++ b/mslib/mscolab/sockets_manager.py @@ -262,7 +262,7 @@ def emit_operation_delete(self, op_id): socketio.emit("operation-deleted", json.dumps({"op_id": op_id})) -def setup_managers(app): +def _setup_managers(app): """ takes app as parameter to extract config data, initializes ChatManager, FileManager, SocketManager and return them diff --git a/tests/_test_mscolab/test_server.py b/tests/_test_mscolab/test_server.py index 680364a4b..9ffb47a0c 100644 --- a/tests/_test_mscolab/test_server.py +++ b/tests/_test_mscolab/test_server.py @@ -30,7 +30,7 @@ from mslib.mscolab.conf import mscolab_settings from mslib.mscolab.models import User, Operation -from mslib.mscolab.server import initialize_managers, check_login, register_user +from mslib.mscolab.server import check_login, register_user from mslib.mscolab.file_manager import FileManager from mslib.mscolab.seed import add_user, get_user @@ -43,9 +43,9 @@ def setup(self, mscolab_app): with self.app.app_context(): yield - def test_initialize_managers(self): - app, sockio, cm, fm = initialize_managers(self.app) - assert app.config['MSCOLAB_DATA_DIR'] == mscolab_settings.MSCOLAB_DATA_DIR + def test_initialized_managers(self, mscolab_managers): + sockio, cm, fm = mscolab_managers + assert self.app.config['MSCOLAB_DATA_DIR'] == mscolab_settings.MSCOLAB_DATA_DIR assert 'Create a Flask-SocketIO server.' in sockio.__doc__ assert 'Class with handler functions for chat related functionalities' in cm.__doc__ assert 'Class with handler functions for file related functionalities' in fm.__doc__ diff --git a/tests/_test_mscolab/test_sockets_manager.py b/tests/_test_mscolab/test_sockets_manager.py index 574730c6e..12b28d8a7 100644 --- a/tests/_test_mscolab/test_sockets_manager.py +++ b/tests/_test_mscolab/test_sockets_manager.py @@ -43,7 +43,8 @@ class Test_Socket_Manager: @pytest.fixture(autouse=True) def setup(self, mscolab_app, mscolab_managers, mscolab_server): self.app = mscolab_app - _, self.cm, self.fm = mscolab_managers + sockio, self.cm, self.fm = mscolab_managers + self.sm = sockio.sm self.url = mscolab_server self.sockets = [] self.userdata = 'UV10@uv10', 'UV10', 'uv10' @@ -57,7 +58,6 @@ def setup(self, mscolab_app, mscolab_managers, mscolab_server): self.anotheruser = get_user(self.anotheruserdata[0]) self.token = self.user.generate_auth_token() self.operation = get_operation(self.operation_name) - self.sm = SocketsManager(self.cm, self.fm) yield for sock in self.sockets: sock.disconnect() diff --git a/tests/fixtures.py b/tests/fixtures.py index 4005225ec..a6a90d330 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -35,7 +35,7 @@ from PyQt5 import QtWidgets from contextlib import contextmanager from mslib.mscolab.conf import mscolab_settings -from mslib.mscolab.server import APP, initialize_managers +from mslib.mscolab.server import APP, sockio, cm, fm from mslib.mscolab.mscolab import handle_db_init, handle_db_reset from mslib.utils.config import modify_config_file from tests.utils import is_url_response_ok @@ -104,7 +104,7 @@ def mscolab_session_managers(mscolab_session_app): This fixture should not be used in tests. Instead use :func:`mscolab_managers`, which handles per-test cleanup as well. """ - return initialize_managers(mscolab_session_app)[1:] + return sockio, cm, fm @pytest.fixture(scope="session") @@ -141,8 +141,7 @@ def mscolab_app(mscolab_session_app, reset_mscolab): def mscolab_managers(mscolab_session_managers, reset_mscolab): """Fixture that provides the MSColab managers and does cleanup actions. - :returns: A tuple (SocketIO, ChatManager, FileManager) as returned by - initialize_managers. + :returns: A tuple (SocketIO, ChatManager, FileManager). """ return mscolab_session_managers From c35c31c2288b5fe341f9c40b5f214936ddcf6523 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Ri=C3=9Fe?= <9308656+matrss@users.noreply.github.com> Date: Thu, 1 Aug 2024 08:14:23 +0200 Subject: [PATCH 2/2] Use Flask-SocketIO test client (#2445) Previously test_sockets_manager.py communicated with a MSColab server running in a different process. This made it impossible to assert anything on the server state (e.g. on attributes of the SocketsManager object) due to the process boundary. Using a test client also simplifies the test setup quite a bit. --- tests/_test_mscolab/test_sockets_manager.py | 74 ++++++--------------- 1 file changed, 20 insertions(+), 54 deletions(-) diff --git a/tests/_test_mscolab/test_sockets_manager.py b/tests/_test_mscolab/test_sockets_manager.py index 12b28d8a7..84152f219 100644 --- a/tests/_test_mscolab/test_sockets_manager.py +++ b/tests/_test_mscolab/test_sockets_manager.py @@ -26,11 +26,7 @@ """ import os import pytest -import socket -import socketio import datetime -import requests -from urllib.parse import urljoin, urlparse from mslib.msui.icons import icons from mslib.mscolab.conf import mscolab_settings @@ -41,11 +37,10 @@ class Test_Socket_Manager: @pytest.fixture(autouse=True) - def setup(self, mscolab_app, mscolab_managers, mscolab_server): + def setup(self, mscolab_app, mscolab_managers): self.app = mscolab_app - sockio, self.cm, self.fm = mscolab_managers - self.sm = sockio.sm - self.url = mscolab_server + self.sockio, self.cm, self.fm = mscolab_managers + self.sm = self.sockio.sm self.sockets = [] self.userdata = 'UV10@uv10', 'UV10', 'uv10' self.anotheruserdata = 'UV20@uv20', 'UV20', 'uv20' @@ -62,23 +57,9 @@ def setup(self, mscolab_app, mscolab_managers, mscolab_server): for sock in self.sockets: sock.disconnect() - def _can_ping_server(self): - parsed_url = urlparse(self.url) - host, port = parsed_url.hostname, parsed_url.port - try: - sock = socket.create_connection((host, port)) - success = True - except socket.error: - success = False - finally: - sock.close() - return success - def _connect(self): - sio = socketio.Client(reconnection_attempts=5) + sio = self.sockio.test_client(self.app) self.sockets.append(sio) - assert self._can_ping_server() - sio.connect(self.url, transports='polling') sio.emit('connect') return sio @@ -88,13 +69,8 @@ def _new_operation(self, operation_name, description): return operation def test_handle_connect(self): - sio = socketio.Client() - assert sio.sid is None - self.sockets.append(sio) - assert self._can_ping_server() - sio.connect(self.url, transports='polling') - sio.emit('connect') - assert len(sio.sid) > 5 + sio = self._connect() + assert len(sio.eio_sid) > 5 def test_join_creator_to_operatiom(self): sio = self._connect() @@ -122,14 +98,12 @@ def test_join_collaborator_to_operation(self): def test_remove_collaborator_from_operation(self): pytest.skip("get_session_id has None result") - sio = self._connect() operation = self._new_operation('new_operation', "example description") sm = SocketsManager(self.cm, self.fm) sm.join_collaborator_to_operation(self.anotheruser.id, operation.id) perms = Permission(self.anotheruser.id, operation.id, "collaborator") assert perms is not None sm.remove_collaborator_from_operation(self.anotheruser.id, operation.id) - sio.sleep(1) perms = Permission(self.anotheruser.id, operation.id, "collaborator") assert perms is None @@ -158,7 +132,6 @@ def test_send_message(self): "message_text": "® non ascii", "reply_id": -1 }) - sio.sleep(1) with self.app.app_context(): message = Message.query.filter_by(text="message from 1").first() @@ -185,7 +158,6 @@ def test_get_messages(self): "message_text": "message from 1", "reply_id": -1 }) - sio.sleep(5) with self.app.app_context(): messages = self.cm.get_messages(1) assert messages[0]["text"] == "message from 1" @@ -216,7 +188,6 @@ def test_get_messages_api(self): "message_text": "message from 1", "reply_id": -1 }) - sio.sleep(1) token = self.token data = { @@ -224,15 +195,14 @@ def test_get_messages_api(self): "op_id": self.operation.id, "timestamp": datetime.datetime(1970, 1, 1, tzinfo=datetime.timezone.utc).isoformat() } - # returns an array of messages - url = urljoin(self.url, 'messages') - res = requests.get(url, data=data, timeout=(2, 10)).json() - assert len(res["messages"]) == 2 + with self.app.test_client() as c: + res = c.get("/messages", data=data) + assert len(res.json["messages"]) == 2 - data["token"] = "dummy" - # returns False due to bad authorization - r = requests.get(url, data=data, timeout=(2, 10)) - assert r.text == "False" + data["token"] = "dummy" + # returns False due to bad authorization + r = c.get("/messages", data=data) + assert r.text == "False" def test_edit_message(self): sio = self._connect() @@ -244,7 +214,6 @@ def test_edit_message(self): "message_text": "Edit this message", "reply_id": -1 }) - sio.sleep(1) with self.app.app_context(): message = Message.query.filter_by(text="Edit this message").first() sio.emit('edit-message', { @@ -253,16 +222,14 @@ def test_edit_message(self): "op_id": message.op_id, "token": self.token }) - sio.sleep(1) token = self.token data = { "token": token, "op_id": self.operation.id, "timestamp": datetime.datetime(1970, 1, 1, tzinfo=datetime.timezone.utc).isoformat() } - # returns an array of messages - url = urljoin(self.url, 'messages') - res = requests.get(url, data=data, timeout=(2, 10)).json() + with self.app.test_client() as c: + res = c.get("messages", data=data).json assert len(res["messages"]) == 1 messages = res["messages"][0] assert messages["text"] == "I have updated the message" @@ -277,7 +244,7 @@ def test_delete_message(self): "message_text": "delete this message", "reply_id": -1 }) - sio.sleep(1) + with self.app.app_context(): message = Message.query.filter_by(text="delete this message").first() sio.emit('delete-message', { @@ -285,7 +252,6 @@ def test_delete_message(self): 'op_id': self.operation.id, 'token': self.token }) - sio.sleep(1) with self.app.app_context(): assert Message.query.filter_by(text="delete this message").count() == 0 @@ -293,14 +259,14 @@ def test_delete_message(self): def test_upload_file(self): sio = self._connect() sio.emit('start', {'token': self.token}) - files = {'file': open(icons('16x16'), 'rb')} data = { "token": self.token, "op_id": self.operation.id, - "message_type": int(MessageType.IMAGE) + "message_type": int(MessageType.IMAGE), + "file": open(icons('16x16'), 'rb'), } - url = urljoin(self.url, 'message_attachment') - requests.post(url, data=data, files=files, timeout=(2, 10)) + with self.app.test_client() as c: + c.post("message_attachment", data=data, content_type="multipart/form-data") upload_dir = os.path.join(mscolab_settings.UPLOAD_FOLDER, str(self.user.id)) assert os.path.exists(upload_dir) file = os.listdir(upload_dir)[0]