add game hash query param to patch api calls #384
Conversation
|
Had a lot of trouble getting the tests to run on my ARM machine. This is a comment just to help anyone else in the future (or myself again) who may be in a similar boat. To finally repro the failures occurring in CI, I created a Dockerfile at root with the following content: FROM --platform=linux/amd64 ubuntu:latest
RUN apt-get update && \
apt-get install -y gcc-multilib make
WORKDIR /rcheevos/test
CMD make clean && make BUILD=c89 test && ./testThen to execute the tests: docker run --platform=linux/amd64 -v "$(pwd):/rcheevos" -e ARCH=x86 rcheevos-test |
|
|
||
| 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"); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
What sort of trouble? Was the makefile not running? Perhaps some of these changes might be applicable? |
The makefile was indeed running, but I was experiencing something like these workflow runs: Locally, everything was passing similar to that one passing job. Then when I actually pushed, I was surprised to find so many things were failing. Only when I set up the Dockerfile locally could I repro those failures on my ARM machine. |
|
This also could just stem from me having a very limited understanding of the rcheevos project. This was a big learning experience for me. |
Required for multiset.