Skip to content
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

Jump to bottom

add game hash query param to patch api calls #384

Closed
wants to merge 6 commits into from
Closed

add game hash query param to patch api calls #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
612b3b6
add game hash query param to patch api calls
wescopeland Nov 13, 2024
e581c18
fix tests
wescopeland Nov 13, 2024
6f04b04
more fixes
wescopeland Nov 13, 2024
cf54efc
add missing change
wescopeland Nov 13, 2024
e261cec
fix more stuff
wescopeland Nov 13, 2024
ff95368
final test fix
wescopeland Nov 13, 2024
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
2 changes: 2 additions & 0 deletions include/rc_api_runtime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ typedef struct rc_api_fetch_game_data_request_t {
const char* api_token;
/* The unique identifier of the game */
uint32_t game_id;
/* The hash associated to the game being played */
const char* game_hash;
}
rc_api_fetch_game_data_request_t;

Expand Down
4 changes: 4 additions & 0 deletions src/rapi/rc_api_runtime.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ int rc_api_init_fetch_game_data_request(rc_api_request_t* request, const rc_api_
rc_url_builder_init(&builder, &request->buffer, 48);
if (rc_api_url_build_dorequest(&builder, "patch", api_params->username, api_params->api_token)) {
rc_url_builder_append_unum_param(&builder, "g", api_params->game_id);

if (api_params->game_hash && *api_params->game_hash)
rc_url_builder_append_str_param(&builder, "m", api_params->game_hash);

request->post_data = rc_url_builder_finalize(&builder);
request->content_type = RC_CONTENT_TYPE_URLENCODED;
}
Expand Down
1 change: 1 addition & 0 deletions src/rc_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2371,6 +2371,7 @@ static void rc_client_begin_fetch_game_data(rc_client_load_state_t* load_state)
fetch_game_data_request.username = client->user.username;
fetch_game_data_request.api_token = client->user.token;
fetch_game_data_request.game_id = load_state->hash->game_id;
fetch_game_data_request.game_hash = load_state->hash->hash;

result = rc_api_init_fetch_game_data_request(&request, &fetch_game_data_request);
if (result != RC_OK) {
Expand Down
38 changes: 38 additions & 0 deletions test/rapi/test_rc_api_runtime.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,42 @@ static void test_init_fetch_game_data_request() {
rc_api_destroy_request(&request);
}

static void test_init_fetch_game_data_request_game_hash() {
rc_api_fetch_game_data_request_t fetch_game_data_request;
rc_api_request_t request;

memset(&fetch_game_data_request, 0, sizeof(fetch_game_data_request));
fetch_game_data_request.username = "Username";
fetch_game_data_request.api_token = "API_TOKEN";
fetch_game_data_request.game_id = 1234;
fetch_game_data_request.game_hash = "ABCDEF0123456789";

ASSERT_NUM_EQUALS(rc_api_init_fetch_game_data_request(&request, &fetch_game_data_request), RC_OK);
ASSERT_STR_EQUALS(request.url, DOREQUEST_URL);
ASSERT_STR_EQUALS(request.post_data, "r=patch&u=Username&t=API_TOKEN&g=1234&m=ABCDEF0123456789");
Copy link
Member

Choose a reason for hiding this comment

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

Are g and m mutually exclusive?

What will the server return if m isn't a known hash for game g?

Will the server changes allow fetching game data without a g parameter? (see RetroAchievements/RAWeb#618)

Copy link
Member Author

Choose a reason for hiding this comment

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

It probably makes sense for m to always take precedence over g if m is provided.

In https://github.com/RetroAchievements/RAWeb/blob/40cc59a25a99c0b5abb82bc140cd5c4b8a15240f/app/Platform/Actions/ResolveAchievementSetsAction.php, the game ID for each set is also returned. This is achieved via the attachCoreGameIds() function:

Screenshot 2024-11-13 at 7 15 41 PM

Note in the screenshot above, I've attached a subset to game ID 1 which in the real world has no place being there.

Given this, maybe it makes sense to remove g from the patch API call long-term? My gut feeling is it shouldn't be removed until multiset is actually generally available to the public, that way we still have game ID to fall back to use legacy logic if something goes wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Server-side, we can't realistically ever remove it since we have no clear path to deprecate old clients.

Client side, I'd prefer to keep it as well, as some clients may not be using rc_client yet, and removing the field would be a breaking change.

I was thinking something more along these lines:

    if (api_params->game_hash && *api_params->game_hash)
      rc_url_builder_append_str_param(&builder, "m", api_params->game_hash);
    else
      rc_url_builder_append_unum_param(&builder, "g", api_params->game_id);

Copy link
Member Author

Choose a reason for hiding this comment

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

That proposed change definitely makes sense and I'd be happy to do it. The only lingering question in the back of my mind is what if the server for some reason does not respect the m param, ie:

  • The code to do so is not yet deployed.
  • The code to do so is deployed but, for some reason, disabled.

I'm wondering if we may always want to keep g on to serve as a fallback value.

Copy link
Member

Choose a reason for hiding this comment

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

Generally, when I make API changes, I develop the client and server code at the same time to ensure it's functioning the way I want it to, then open the server PR, and only open the client PR after the server code is merged under the assumption that it will be deployed before the client PR gets merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

It sounds like it may make sense to close this PR until the work in RAWeb is complete, and only re-open it once that work is merged.

ASSERT_STR_EQUALS(request.content_type, RC_CONTENT_TYPE_URLENCODED);

rc_api_destroy_request(&request);
}

static void test_init_fetch_game_data_request_game_hash_empty(void) {
rc_api_fetch_game_data_request_t fetch_game_data_request;
rc_api_request_t request;

memset(&fetch_game_data_request, 0, sizeof(fetch_game_data_request));
fetch_game_data_request.username = "Username";
fetch_game_data_request.api_token = "API_TOKEN";
fetch_game_data_request.game_id = 1234;
fetch_game_data_request.game_hash = "";

ASSERT_NUM_EQUALS(rc_api_init_fetch_game_data_request(&request, &fetch_game_data_request), RC_OK);
ASSERT_STR_EQUALS(request.url, DOREQUEST_URL);
ASSERT_STR_EQUALS(request.post_data, "r=patch&u=Username&t=API_TOKEN&g=1234");
ASSERT_STR_EQUALS(request.content_type, RC_CONTENT_TYPE_URLENCODED);

rc_api_destroy_request(&request);
}

static void test_init_fetch_game_data_request_no_id() {
rc_api_fetch_game_data_request_t fetch_game_data_request;
rc_api_request_t request;
Expand Down Expand Up @@ -1270,6 +1306,8 @@ void test_rapi_runtime(void) {

/* patch */
TEST(test_init_fetch_game_data_request);
TEST(test_init_fetch_game_data_request_game_hash);
TEST(test_init_fetch_game_data_request_game_hash_empty);
TEST(test_init_fetch_game_data_request_no_id);

TEST(test_process_fetch_game_data_response_empty);
Expand Down
Loading
Loading