Issue 165 port api to python

[adrian-golian]

Re-write of the game engine in Python.

[maciej-pomykala]

From what I've seen so far, to preserve the behaviour of the engine, we need to:

• report hands with a separate object for each player
• allow users to set round to -1 when querying games (note: the old engine also allowed 0, but that's not intended)
• return error messages as JSON objects (e.g. {"error":"Nickname already taken"} instead of Nickname already taken) within valid HTTP responses

Less important things (that don't seem to break anything but we can watch out for them):

• give cp_nickname a value of {} instead of null when game is not yet started

[adrian-golian]

I was kind of surprised to see hands as a flat array, would be curious to see what R-to-Python quirk let this mistranslation happen.

You say

the old engine also allowed 0, but that's not intended

Well, as you see yourself, we are fully in position to make the calls on what's "not intended" and can be changed. I would use this reasoning to get rid of round=-1 and cp_nickname={} as either is a weird idiosyncrasy - what should actually be happening in a JSON payload is either 1) omit the key in the request, or 2) set it to null.

I would try to convince you to use our power to decide what is "not intended" and choose the bright, sunlit uplands of good JSON.

EDIT (23.06.2021 10:49 UTC+3): It at least makes sense if we had agreed on round=-1, as it's a "neither" kind of integer, but an empty object as a placeholder for a missing string is a different matter.

[maciej-pomykala]

I was kind of surprised to see hands as a flat array, would be curious to see what R-to-Python quirk let this mistranslation happen.

In the definition of draw_cards (line 83 in start_game.py and line 232 of play.py), I think we're looking to use append instead of extend to make more hierarchy 🦞.

cp_nicknames definitely makes more sense than null, let's just watch out for clients complaining after we change it.

Yeah, let's drop round = -1 too.

[adrian-golian]

append instead of extend

It should actually be an object, like:

hands[player_nickname] = [{"value": value, "colour": colour}, {"value": value, "colour": colour}]

to achieve the behaviour you described.

[maciej-pomykala]

In R, hands was an unnamed list (to make it a series instead of an object), where each player was a named list with two elements, their nickname and their cards, where the former was just a string and the latter was a data frame (data frames get translated into series of objects).

[adrian-golian]

each player was a named list

Yeah, there's not such class in Python. Object with player keys - is what the JSON you described is, right?

EDIT: Or was it an array of objects like this?

[{"nickname": [card1, card2, card3]}, {"nickname2": [card4]}]

[maciej-pomykala]

This is an example of a game in the old engine, with spaces added by me:

{
  "admin_nickname":"Annoying_Alligator",
  "public":false,
  "status":"Running",
  "round_number":2,
  "max_cards":11,
  "players":[
    {"nickname":"Annoying_Alligator","n_cards":2},
    {"nickname":"Woolly_Wolverine","n_cards":1}
  ],
  "hands":[
    {"nickname":"Annoying_Alligator","hand":[{"value":5,"colour":2},{"value":2,"colour":1}]},
    {"nickname":"Woolly_Wolverine","hand":[{"value":1,"colour":1}]}
  ],
  "cp_nickname":{},
  "history":[
    {"player":"Annoying_Alligator","action_id":0},
    {"player":"Woolly_Wolverine","action_id":88},
    {"player":"Annoying_Alligator","action_id":89}
  ]
}

[maciej-pomykala]

Other things:

• when the round is over, the history is incorect. Action 88 (check) should have been mentioned and, for the player with action 89, number of cards and UUID have been unnecessarily added. Example below. [RESOLVED]

{"admin_nickname": "Normal_Newt", "public": false, "status": "Running", "round_number": 1, "max_cards": 11, "players": [{"nickname": "Normal_Newt", "n_cards": 1}, {"nickname": "Nano_Nightelf", "n_cards": 1}], "hands": [], "cp_nickname": null, "history": [{"action_id": 0, "player": "Normal_Newt"}, {"action_id": 1, "player": "Nano_Nightelf"}, {"action_id": 2, "player": "Normal_Newt"}, {"action_id": 3, "player": "Nano_Nightelf"}, {"action_id": 4, "player": "Normal_Newt"}, {"action_id": 7, "player": "Nano_Nightelf"}, {"action_id": 10, "player": "Normal_Newt"}, {"action_id": 89, "player": {"nickname": "Normal_Newt", "n_cards": 1, "uuid": "10733c7f-57b0-49ed-92b6-9186052cbc56"}}]}

• the play endpoint is now giving status code 201 (Created) instead of 200 (OK) when it's happy, which is a change from the old engine and its documentation. I assume this is not to introduce more rigour, becuase then the create endpoint would have used 201 too. [RESOLVED]
• when a round is finished, any hands are hidden to users who don't authenticate themselves as players but, when players authenticate themselves, all hands are correctly revealed. This is strange and I can't see where the mistake is by just looking at the code [RESOLVED]

[adrian-golian]

As to wrapping error payloads in {"error": "message..."}, I'm already not that happy that I'm doing a lot of:

response_payload(400, "This action not allowed right now")

Instead of

parameter_error_payload("action_id", "This action not allowed right now")

So we can replace all these cases. While inside parameter_error_payload, instead of:

def parameter_error_payload(param_key, param_value, message=None):
    body = "Bad input value in '{}': {}".format(param_key, param_value)
    if message:
        body = "{}\n{}".format(body, message)
    return response_payload(400, body)

do:

def parameter_error_payload(param_key, param_value, message=None):
    body = "Bad input value in '{}': {}".format(param_key, param_value)
    if message:
        body = {"error": "{}\n{}".format(body, message)}             #  <------ CHANGED HERE
    return response_payload(400, body)

And same (wrapper) goes for request_error_payload, and internal_error_payload.

[adrian-golian]

For now (maybe indefinitely), while we want to use the Lambda editor to make code changes, each Lambda needs all these functions in its own module, so lots of functions are duplicated (risking inconsistency). If we refactor common functions, we'll need to zip and upload each lambda package individually.

[maciej-pomykala]

A public games request consumes 15 reads from DynamoDB. There are currently 196 game objects, with 4 games public. It might be because the whole database gets scanned whenever public games are requested, and every 4KB counts as one read. That would make sense, given that 196 games (many of them quite empty) would be around 60KB.

If that's true, we have at least these options:

• Automatically archive games
• Reform the public games mechanic so that it doesn't scan the whole table. Maybe have a single public games object that gets expanded (with new UUID and time of expiry) by the make public endpoint and reduced by the make private and public games endpoints.
• EDIT: We can also make a secondary index of games with public as the primary key. We can also add a property saying when the game was made public. Then, the public games endpoint could, in addition to what it's doing now, privatise games that have been public for more than 3 minutes. This would make the public games mechanic only assist in finding games that are searching for players, which is arguably the primary use case of the mechanic.

The third option sounds best to me.

[adrian-golian]

All looks good to me as well. 🚀