Skip to content

Fix response when friendship exists #55

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Fix response when friendship exists #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f69e656
[friendships] Fix response when creating an existing friendship
Indexu Nov 19, 2021
f1ab03f
[friendships] Fix adding friend twice test
Indexu Nov 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions driftbase/api/friendships.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
import marshmallow as ma
from drift.core.extensions.jwt import current_user
from drift.core.extensions.urlregistry import Endpoints
from flask import request, g, abort, url_for, jsonify
from flask import request, g, url_for, jsonify
from flask.views import MethodView
from flask_smorest import Blueprint
from flask_smorest import Blueprint, abort
import http.client as http_client
from sqlalchemy.exc import IntegrityError
from sqlalchemy.orm import aliased
Expand Down Expand Up @@ -63,7 +63,7 @@ def get(self, player_id):
List my friends
"""
if player_id != current_user["player_id"]:
abort(http_client.FORBIDDEN, description="That is not your player!")
abort(http_client.FORBIDDEN, message="That is not your player!")

left = g.db.query(Friendship.id, Friendship.player1_id, Friendship.player2_id).filter_by(player1_id=player_id,
status="active")
Expand Down Expand Up @@ -91,24 +91,24 @@ def post(self, args, player_id):
New friend
"""
if player_id != current_user["player_id"]:
abort(http_client.FORBIDDEN, description="That is not your player!")
abort(http_client.FORBIDDEN, message="That is not your player!")

invite_token = args.get("token")

# Get the first non-expired invite that matches the token
invite = g.db.query(FriendInvite).filter(FriendInvite.token == invite_token, FriendInvite.expiry_date > datetime.datetime.utcnow()).first()
if invite is None:
abort(http_client.NOT_FOUND, description="The invite was not found!")
abort(http_client.NOT_FOUND, message="The invite was not found!")

if invite.deleted:
abort(http_client.FORBIDDEN, description="The invite has been deleted!")
abort(http_client.FORBIDDEN, message="The invite has been deleted!")

friend_id = invite.issued_by_player_id
left_id = player_id
right_id = friend_id

if left_id == right_id:
abort(http_client.FORBIDDEN, description="You cannot befriend yourself!")
abort(http_client.FORBIDDEN, message="You cannot befriend yourself!")

if left_id > right_id:
left_id, right_id = right_id, left_id
Expand All @@ -122,7 +122,7 @@ def post(self, args, player_id):
if friendship.status == "deleted":
friendship.status = "active"
else:
return "{}", http_client.OK
abort(http_client.CONFLICT, message="You are already friends with this player!")
Copy link

@judgeaxl judgeaxl Nov 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an API change and we'd need to see how old clients deal with this situation

Copy link

@einarth einarth Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's assume I want to do just that, i.e. change an API;

  • I need a list of all the tenants, presumably from the live tier config
  • I find the repos for the clients and read the code; lets assume some of them look fine and should be ok with the change, but some would need minor updates
  • In either case, I'd want to test a client against a dev tier (or local) backend to see if all is good with the API change (with or without client modifications).
  • That means I'd need the full build environment for all tenants for local testing and to know how to direct them to the correct testing tier (is driftUrl=XXX in .ini the standard way and works everywhere)?
  • I'd also need to know the deployment pipeline to publish updates to live for all the other tenants.

Sounds like a lot of time consuming busywork.

Copy link

@judgeaxl judgeaxl Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you can't realistically check all clients, you might not even have access, the only proper way is to not break APIs, but to version, one way or another.

We know we want versioning, and I would prefer not having to operate multiple versions of every deployable forsaking multi-tenancy, as that suddenly requires versioning the deployment infrastructure instead, and you're basically just moving complexity from the API management to infrastructure management and ops.

So, what we should do, is to figure out API versioning. And until then, we need to simply avoid breaking APIs.

else:
friendship = Friendship(player1_id=left_id, player2_id=right_id)
g.db.add(friendship)
Expand Down Expand Up @@ -153,9 +153,9 @@ def delete(self, friendship_id):

friendship = g.db.query(Friendship).filter_by(id=friendship_id).first()
if friendship is None:
abort(http_client.NOT_FOUND, description="Unknown friendship")
abort(http_client.NOT_FOUND, message="Unknown friendship")
elif friendship.player1_id != player_id and friendship.player2_id != player_id:
abort(http_client.FORBIDDEN, description="You are not friends")
abort(http_client.FORBIDDEN, message="You are not friends")
elif friendship.status == "deleted":
return jsonify("{}"), http_client.GONE

Expand Down Expand Up @@ -213,9 +213,9 @@ def post(self, args):
valid_message = "no longer valid" if existing_invite.deleted or existing_invite.expiry_date <= datetime.datetime.utcnow() else "valid"
log.info(f"Generated duplicate wordlist invite token '{token}'. Existing token is {valid_message}. Re-generating...")
else:
abort(http_client.INTERNAL_SERVER_ERROR, description="Could not generate invite token")
abort(http_client.INTERNAL_SERVER_ERROR, message="Could not generate invite token")
else:
abort(http_client.BAD_REQUEST, description="Invalid token format")
abort(http_client.BAD_REQUEST, message="Invalid token format")

expires_seconds = args.get("expiration_time_seconds") or _get_tenant_config_value("invite_expiration_seconds")
expires_seconds = min(expires_seconds, MAX_INVITE_EXPIRATION_SECONDS)
Expand All @@ -232,7 +232,7 @@ def post(self, args):
g.db.add(invite)
g.db.commit()
except IntegrityError as e:
abort(http_client.BAD_REQUEST, description="Invalid player IDs provided with request.")
abort(http_client.BAD_REQUEST, message="Invalid player IDs provided with request.")

if receiving_player_id is not None:
self._post_friend_request_message(sending_player_id, receiving_player_id, token, expires_seconds)
Expand All @@ -248,7 +248,7 @@ def post(self, args):
def _validate_friend_request(receiving_player_id):
sending_player_id = int(current_user["player_id"])
if receiving_player_id == sending_player_id:
abort(http_client.CONFLICT, description="Cannot send friend requests to yourself")
abort(http_client.CONFLICT, message="Cannot send friend requests to yourself")

player1_id, player2_id = min(sending_player_id, receiving_player_id), max(sending_player_id,
receiving_player_id)
Expand All @@ -257,20 +257,20 @@ def _validate_friend_request(receiving_player_id):
Friendship.player2_id == player2_id
).first()
if existing_friendship and existing_friendship.status == "active":
abort(http_client.CONFLICT, description="You are already friends") # Already friends
abort(http_client.CONFLICT, message="You are already friends") # Already friends
pending_invite = g.db.query(FriendInvite). \
filter(FriendInvite.issued_by_player_id == sending_player_id,
FriendInvite.issued_to_player_id == receiving_player_id). \
filter(FriendInvite.expiry_date > datetime.datetime.utcnow(), FriendInvite.deleted.is_(False)). \
first()
if pending_invite:
abort(http_client.CONFLICT, description="Cannot issue multiple friend requests to the same receiver")
abort(http_client.CONFLICT, message="Cannot issue multiple friend requests to the same receiver")
reciprocal_invite = g.db.query(FriendInvite). \
filter(FriendInvite.issued_by_player_id == receiving_player_id,
FriendInvite.issued_to_player_id == sending_player_id). \
filter(FriendInvite.expiry_date > datetime.datetime.utcnow(), FriendInvite.deleted.is_(False)).first()
if reciprocal_invite:
abort(http_client.CONFLICT, description="The receiver has already sent you a friend request")
abort(http_client.CONFLICT, message="The receiver has already sent you a friend request")

@staticmethod
def _post_friend_request_message(sender_player_id, receiving_player_id, token, expiry):
Expand All @@ -294,10 +294,10 @@ def delete(self, invite_id):

invite = g.db.query(FriendInvite).filter_by(id=invite_id).first()
if not invite:
abort(http_client.NOT_FOUND, description="Invite not found")
abort(http_client.NOT_FOUND, message="Invite not found")
elif invite.issued_by_player_id != player_id and invite.issued_to_player_id != player_id:
# You may only delete invites sent by you or directly to you.
abort(http_client.FORBIDDEN, description="Not your invite")
abort(http_client.FORBIDDEN, message="Not your invite")
elif invite.deleted:
return jsonify("{}"), http_client.GONE

Expand Down
2 changes: 1 addition & 1 deletion driftbase/tests/test_friendships.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ def test_adding_same_friend_twice_changes_nothing(self):
# add a friend
self.post(self.endpoints["my_friends"], data={"token": token}, expected_status_code=http_client.CREATED)
# add same friend again
self.post(self.endpoints["my_friends"], data={"token": token}, expected_status_code=http_client.OK)
self.post(self.endpoints["my_friends"], data={"token": token}, expected_status_code=http_client.CONFLICT)

friends = self.get(self.endpoints["my_friends"]).json()
self.assertIsInstance(friends, list)
Expand Down