-
Notifications
You must be signed in to change notification settings - Fork 342
Give priority to users connected to collaboration server (aka no websocket feature) #1093
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
Changes from all commits
963d5f9
e1409ae
88d27ab
1728b27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -30,6 +30,7 @@ | |||||
from rest_framework import response as drf_response | ||||||
from rest_framework.permissions import AllowAny | ||||||
from rest_framework.throttling import UserRateThrottle | ||||||
from sentry_sdk import capture_exception | ||||||
|
||||||
from core import authentication, enums, models | ||||||
from core.services.ai_services import AIService | ||||||
|
@@ -631,6 +632,82 @@ def perform_destroy(self, instance): | |||||
"""Override to implement a soft delete instead of dumping the record in database.""" | ||||||
instance.soft_delete() | ||||||
|
||||||
def _compute_no_websocket_cache_key(self, document_id): | ||||||
"""Compute the cache key for the no websocket cache.""" | ||||||
return f"docs:no-websocket:{document_id}" | ||||||
|
||||||
def _can_user_edit_document(self, document_id, set_cache=False): | ||||||
"""Check if the user can edit the document.""" | ||||||
try: | ||||||
connection_info = CollaborationService().get_document_connection_info( | ||||||
document_id, | ||||||
self.request.session.session_key, | ||||||
) | ||||||
except requests.HTTPError as e: | ||||||
capture_exception(e) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
No? |
||||||
connection_info = { | ||||||
"count": 0, | ||||||
"exists": False, | ||||||
} | ||||||
|
||||||
if connection_info["count"] == 0: | ||||||
# Nobody is connected to the websocket server | ||||||
logger.debug("update without connection found in the websocket server") | ||||||
cache_key = self._compute_no_websocket_cache_key(document_id) | ||||||
current_editor = cache.get(cache_key) | ||||||
|
||||||
if not current_editor: | ||||||
if set_cache: | ||||||
cache.set( | ||||||
cache_key, | ||||||
self.request.session.session_key, | ||||||
settings.NO_WEBSOCKET_CACHE_TIMEOUT, | ||||||
) | ||||||
return True | ||||||
|
||||||
if current_editor != self.request.session.session_key: | ||||||
return False | ||||||
|
||||||
if set_cache: | ||||||
cache.touch(cache_key, settings.NO_WEBSOCKET_CACHE_TIMEOUT) | ||||||
return True | ||||||
|
||||||
if connection_info["exists"]: | ||||||
# Current user is connected to the websocket server | ||||||
logger.debug("session key found in the websocket server") | ||||||
return True | ||||||
|
||||||
logger.debug( | ||||||
"Users connected to the websocket but current editor not connected to it. Can not edit." | ||||||
) | ||||||
|
||||||
return False | ||||||
|
||||||
def perform_update(self, serializer): | ||||||
"""Check rules about collaboration.""" | ||||||
if serializer.validated_data.get("websocket"): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Just to be explicit with the default behavior (I know |
||||||
return super().perform_update(serializer) | ||||||
|
||||||
if self._can_user_edit_document(serializer.instance.id, set_cache=True): | ||||||
return super().perform_update(serializer) | ||||||
Comment on lines
+691
to
+692
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may want to raise a specific error content to display a proper message to the user |
||||||
|
||||||
raise drf.exceptions.PermissionDenied( | ||||||
"You are not allowed to edit this document." | ||||||
) | ||||||
|
||||||
@drf.decorators.action( | ||||||
detail=True, | ||||||
methods=["get"], | ||||||
url_path="can-edit", | ||||||
) | ||||||
def can_edit(self, request, *args, **kwargs): | ||||||
"""Check if the current user can edit the document.""" | ||||||
document = self.get_object() | ||||||
|
||||||
return drf.response.Response( | ||||||
{"can_edit": self._can_user_edit_document(document.id)} | ||||||
) | ||||||
|
||||||
@drf.decorators.action( | ||||||
detail=False, | ||||||
methods=["get"], | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
"""Force session creation for all requests.""" | ||
|
||
|
||
class ForceSessionMiddleware: | ||
""" | ||
Force session creation for unauthenticated users. | ||
Must be used after Authentication middleware. | ||
""" | ||
|
||
def __init__(self, get_response): | ||
"""Initialize the middleware.""" | ||
self.get_response = get_response | ||
|
||
def __call__(self, request): | ||
"""Force session creation for unauthenticated users.""" | ||
|
||
if not request.user.is_authenticated and request.session.session_key is None: | ||
request.session.save() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why Is there a test somewhere to check this middleware behavior? |
||
|
||
response = self.get_response(request) | ||
return response |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,3 +41,31 @@ def reset_connections(self, room, user_id=None): | |
f"Failed to notify WebSocket server. Status code: {response.status_code}, " | ||
f"Response: {response.text}" | ||
) | ||
|
||
def get_document_connection_info(self, room, session_key): | ||
""" | ||
Get the connection info for a document. | ||
""" | ||
endpoint = "get-connections" | ||
querystring = { | ||
"room": room, | ||
"sessionKey": session_key, | ||
} | ||
endpoint_url = f"{settings.COLLABORATION_API_URL}{endpoint}/" | ||
|
||
headers = {"Authorization": settings.COLLABORATION_SERVER_SECRET} | ||
|
||
try: | ||
response = requests.get( | ||
endpoint_url, headers=headers, params=querystring, timeout=10 | ||
) | ||
except requests.RequestException as e: | ||
raise requests.HTTPError("Failed to get document connection info.") from e | ||
|
||
if response.status_code != 200: | ||
raise requests.HTTPError( | ||
f"Failed to get document connection info. Status code: {response.status_code}, " | ||
f"Response: {response.text}" | ||
) | ||
|
||
return response.json() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment (or a dataclass) to easily know the json is something like |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,219 @@ | ||
"""Test the can_edit endpoint in the viewset DocumentViewSet.""" | ||
|
||
from django.core.cache import cache | ||
|
||
import pytest | ||
import responses | ||
from rest_framework.test import APIClient | ||
|
||
from core import factories | ||
|
||
pytestmark = pytest.mark.django_db | ||
|
||
|
||
def test_api_documents_can_edit_anonymous(): | ||
"""Anonymous users can not edit documents.""" | ||
document = factories.DocumentFactory() | ||
client = APIClient() | ||
response = client.get(f"/api/v1.0/documents/{document.id!s}/can-edit/") | ||
assert response.status_code == 401 | ||
|
||
@responses.activate | ||
def test_api_documents_can_edit_authenticated_no_websocket(settings): | ||
""" | ||
A user not connected to the websocket and no other user have already updated the document, | ||
the document can be updated. | ||
""" | ||
user = factories.UserFactory(with_owned_document=True) | ||
client = APIClient() | ||
client.force_login(user) | ||
session_key = client.session.session_key | ||
|
||
document = factories.DocumentFactory(users=[(user, "editor")]) | ||
|
||
settings.COLLABORATION_API_URL = "http://example.com/" | ||
settings.COLLABORATION_SERVER_SECRET = "secret-token" | ||
endpoint_url = ( | ||
f"{settings.COLLABORATION_API_URL}get-connections/" | ||
f"?room={document.id}&sessionKey={session_key}" | ||
) | ||
|
||
ws_resp = responses.get(endpoint_url, json={"count": 0, "exists": False}) | ||
|
||
assert cache.get(f"docs:no-websocket:{document.id}") is None | ||
|
||
response = client.get( | ||
f"/api/v1.0/documents/{document.id!s}/can-edit/", | ||
) | ||
assert response.status_code == 200 | ||
|
||
assert response.json() == {"can_edit": True} | ||
assert ws_resp.call_count == 1 | ||
|
||
|
||
@responses.activate | ||
def test_api_documents_can_edit_authenticated_no_websocket_user_already_editing( | ||
settings, | ||
): | ||
""" | ||
A user not connected to the websocket and another user have already updated the document, | ||
the document can not be updated. | ||
""" | ||
user = factories.UserFactory(with_owned_document=True) | ||
client = APIClient() | ||
client.force_login(user) | ||
session_key = client.session.session_key | ||
|
||
document = factories.DocumentFactory(users=[(user, "editor")]) | ||
|
||
settings.COLLABORATION_API_URL = "http://example.com/" | ||
settings.COLLABORATION_SERVER_SECRET = "secret-token" | ||
endpoint_url = ( | ||
f"{settings.COLLABORATION_API_URL}get-connections/" | ||
f"?room={document.id}&sessionKey={session_key}" | ||
) | ||
ws_resp = responses.get(endpoint_url, json={"count": 0, "exists": False}) | ||
|
||
cache.set(f"docs:no-websocket:{document.id}", "other_session_key") | ||
|
||
response = client.get( | ||
f"/api/v1.0/documents/{document.id!s}/can-edit/", | ||
) | ||
assert response.status_code == 200 | ||
assert response.json() == {"can_edit": False} | ||
|
||
assert ws_resp.call_count == 1 | ||
|
||
|
||
@responses.activate | ||
def test_api_documents_can_edit_no_websocket_other_user_connected_to_websocket( | ||
settings, | ||
): | ||
""" | ||
A user not connected to the websocket and another user is connected to the websocket, | ||
the document can not be updated. | ||
""" | ||
user = factories.UserFactory(with_owned_document=True) | ||
client = APIClient() | ||
client.force_login(user) | ||
session_key = client.session.session_key | ||
|
||
document = factories.DocumentFactory(users=[(user, "editor")]) | ||
|
||
settings.COLLABORATION_API_URL = "http://example.com/" | ||
settings.COLLABORATION_SERVER_SECRET = "secret-token" | ||
endpoint_url = ( | ||
f"{settings.COLLABORATION_API_URL}get-connections/" | ||
f"?room={document.id}&sessionKey={session_key}" | ||
) | ||
ws_resp = responses.get(endpoint_url, json={"count": 3, "exists": False}) | ||
|
||
assert cache.get(f"docs:no-websocket:{document.id}") is None | ||
|
||
response = client.get( | ||
f"/api/v1.0/documents/{document.id!s}/can-edit/", | ||
) | ||
assert response.status_code == 200 | ||
assert response.json() == {"can_edit": False} | ||
assert cache.get(f"docs:no-websocket:{document.id}") is None | ||
assert ws_resp.call_count == 1 | ||
|
||
|
||
@responses.activate | ||
def test_api_documents_can_edit_user_connected_to_websocket(settings): | ||
""" | ||
A user connected to the websocket, the document can be updated. | ||
""" | ||
user = factories.UserFactory(with_owned_document=True) | ||
client = APIClient() | ||
client.force_login(user) | ||
session_key = client.session.session_key | ||
|
||
document = factories.DocumentFactory(users=[(user, "editor")]) | ||
|
||
settings.COLLABORATION_API_URL = "http://example.com/" | ||
settings.COLLABORATION_SERVER_SECRET = "secret-token" | ||
endpoint_url = ( | ||
f"{settings.COLLABORATION_API_URL}get-connections/" | ||
f"?room={document.id}&sessionKey={session_key}" | ||
) | ||
ws_resp = responses.get(endpoint_url, json={"count": 3, "exists": True}) | ||
|
||
assert cache.get(f"docs:no-websocket:{document.id}") is None | ||
|
||
response = client.get( | ||
f"/api/v1.0/documents/{document.id!s}/can-edit/", | ||
) | ||
assert response.status_code == 200 | ||
assert response.json() == {"can_edit": True} | ||
assert cache.get(f"docs:no-websocket:{document.id}") is None | ||
assert ws_resp.call_count == 1 | ||
|
||
|
||
@responses.activate | ||
def test_api_documents_can_edit_websocket_server_unreachable_fallback_to_no_websocket( | ||
settings, | ||
): | ||
""" | ||
When the websocket server is unreachable, the document can be updated like if the user was | ||
not connected to the websocket. | ||
""" | ||
user = factories.UserFactory(with_owned_document=True) | ||
client = APIClient() | ||
client.force_login(user) | ||
session_key = client.session.session_key | ||
|
||
document = factories.DocumentFactory(users=[(user, "editor")]) | ||
|
||
settings.COLLABORATION_API_URL = "http://example.com/" | ||
settings.COLLABORATION_SERVER_SECRET = "secret-token" | ||
endpoint_url = ( | ||
f"{settings.COLLABORATION_API_URL}get-connections/" | ||
f"?room={document.id}&sessionKey={session_key}" | ||
) | ||
ws_resp = responses.get(endpoint_url, status=500) | ||
|
||
assert cache.get(f"docs:no-websocket:{document.id}") is None | ||
|
||
response = client.get( | ||
f"/api/v1.0/documents/{document.id!s}/can-edit/", | ||
) | ||
assert response.status_code == 200 | ||
assert response.json() == {"can_edit": True} | ||
|
||
assert ws_resp.call_count == 1 | ||
|
||
|
||
@responses.activate | ||
def test_api_documents_can_edit_websocket_server_unreachable_fallback_to_no_websocket_other_users( | ||
settings, | ||
): | ||
""" | ||
When the websocket server is unreachable, the behavior fallback to the no websocket one. | ||
If an other user is already editing, the document can not be updated. | ||
""" | ||
user = factories.UserFactory(with_owned_document=True) | ||
client = APIClient() | ||
client.force_login(user) | ||
session_key = client.session.session_key | ||
|
||
document = factories.DocumentFactory(users=[(user, "editor")]) | ||
|
||
settings.COLLABORATION_API_URL = "http://example.com/" | ||
settings.COLLABORATION_SERVER_SECRET = "secret-token" | ||
endpoint_url = ( | ||
f"{settings.COLLABORATION_API_URL}get-connections/" | ||
f"?room={document.id}&sessionKey={session_key}" | ||
) | ||
ws_resp = responses.get(endpoint_url, status=500) | ||
|
||
cache.set(f"docs:no-websocket:{document.id}", "other_session_key") | ||
|
||
response = client.get( | ||
f"/api/v1.0/documents/{document.id!s}/can-edit/", | ||
) | ||
assert response.status_code == 200 | ||
assert response.json() == {"can_edit": False} | ||
|
||
assert cache.get(f"docs:no-websocket:{document.id}") == "other_session_key" | ||
assert ws_resp.call_count == 1 |
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 called only once, are you sure you need a dedicated method instead of an f-string where used?
(also this method should be static)