Fix user add and update api endpoints #3040

LuukBlankenstijn · 2025-07-20T10:04:01Z

Found some bugs while using the API, and since I need the endpoints, fixed them.

Problem I encountered:
Endpoint /api/v4/users/{id} PUT request returned{ 400, Body data exceeded php.ini's 'post_max_size' directive (currently set to 512M)}.
Could not get it to work whatever I tried. When looking at the code I also discovered some other issues in the case of updating the user with App\Controller\API\UserController.php addOrUpdateUser().
What I did is changing the content type of the api-doc attribute to application/json, and separated the add and update method in to dedicated methods. I realize this is a breaking change in the api spec and not ideal.

I got the errors on both the DOMjudge instance running on the GEHACK server in Eindhoven, and my own local instance.

This exception is generated in App\EventListener\BodyToBigListener.php. This happens because the amount of parameters on the request is 0. (I get this even when using /api/doc on a running domjudge instance). I first tried fixing the request, but could not get it to work. Because it made more sense to me for a request like this to accept json instead of formdata I decided to change it to that. After I did this the request now worked, but i found some flaws in the update logic for user.

For example: name, username and id are required to even make an update by the documentation. I would have expected to just need an id and being able to update any field. Then, on line 150 in App\Controller\Api\Usercontroler:

protected function addOrUpdateUser(AddUser $addUser, Request $request): Response
    {
        if ($addUser instanceof UpdateUser && !$addUser->id) {
            throw new BadRequestHttpException('`id` field is required');
        }

        if ($this->em->getRepository(User::class)->findOneBy(['username' => $addUser->username])) {
            throw new BadRequestHttpException(sprintf("User %s already exists", $addUser->username));
        }
        $user = new User();
              if ($addUser instanceof UpdateUser) {
                  $existingUser = $this->em->getRepository(User::class)->findOneBy(['externalid' => $addUser->id]);
                  if ($existingUser) {
                      $user = $existingUser;
                  }
              }

        $user
            ->setUsername($addUser->username)
            ->setName($addUser->name)
            ->setEmail($addUser->email)
            ->setIpAddress($addUser->ip)
            ->setPlainPassword($addUser->password)
            ->setEnabled($addUser->enabled ?? true);

       // rest of method
}

So if the user already exists we get a BadRequestException. This effectively means that for every update we want to do to a user, the username has to be changed.
Then also we cannot do partial update by leaving fields in the request null, since all properties of the UpdateUser dto are assigned to the existing user, even when null.

vmcj · 2025-07-20T10:11:28Z

Can you explain which problem you encountered?

LuukBlankenstijn · 2025-07-20T10:50:40Z

Updated the description

vmcj · 2025-07-20T15:01:23Z

Found some bugs while using the API, and since I need the endpoints, fixed them.

Problem I encountered: Endpoint /api/v4/users/{id} PUT request returned{ 400, Body data exceeded php.ini's 'post_max_size' directive (currently set to 512M)}.

Let's discuss in Slack how to fix this one, I assume you posted something which is larger than 512M? Did you raise the post_max_size? You can check for the value detected in the config checker.

... When looking at the code I also discovered some other issues in the case of updating the user with App\Controller\API\UserController.php addOrUpdateUser(). What I did is changing the content type of the api-doc attribute to application/json,

I can see that you did this, but I'm not sure yet why it wants a form but I suspect this is legacy.

and separated the add and update method in to dedicated methods. I realize this is a breaking change in the api spec and not ideal.

So we're working on a major release so now would be the time to make such choices but let's see if we can prevent it still.

webapp/src/Controller/API/UserController.php

webapp/src/DataTransferObject/AddUser.php

vmcj · 2025-07-20T15:08:02Z

@LuukBlankenstijn can you provide the files you were using which hit this error?

eldering · 2025-07-20T21:33:26Z

I've not looked into the changes in detail (and I guess @nickygerritsen might be a better reviewer), but from the description it's not clear to me what these changes are fixing and why/how. Because this seems to have nothing to do with a maximum post size of a request. Can you clarify that?

LuukBlankenstijn · 2025-07-21T07:53:20Z

I updated the description again, I'm sorry for lack initial lack of explanation

nickygerritsen · 2025-07-21T09:44:37Z

I think we can do something smart to still have both update and create in one action, will think about it.

Although that is not very REST, so maybe we shouldn't?

As form json vs formdata, ideally it'd accept both, so I will investigate if we can get that to work.

LuukBlankenstijn · 2025-07-21T16:49:32Z

I think we can do something smart to still have both update and create in one action, will think about it.

I have a, imo, nice idea for this, I can implement it tommorrow if you want.

nickygerritsen · 2025-07-21T17:04:06Z

I think we can do something smart to still have both update and create in one action, will think about it.

I have a, imo, nice idea for this, I can implement it tommorrow if you want.

Sounds good

eldering · 2025-07-21T20:29:17Z

Some general comments:

Our API follows the Contest API specification, specifically following https://ccs-specs.icpc.io/draft/contest_api#http-methods partial updates should only be allowed using PATCH, not PUT.
Nitpick: we don't use semantic commit messages, so just as well leave fix(api) out
Would be good to also write the details of what was broken and how it's changed/fixed in the commit message, so it gets recorded in git history.

fixed. - Server did not accept the data in the format of the api spec. fixed by changing to json. - Updating using required username and name always to be set, even if when not changing one of them. - When updating a user (with the mandatory username), the server would return a 404 saying this username is already in use.

LuukBlankenstijn · 2025-08-08T17:34:55Z

Sorry for the delay, but I finally found some time to work on it. I cannot seem to figure out how to run the tests and how to fix code-style, any help on that would be appreciated.

vmcj · 2025-08-08T17:43:57Z

webapp/src/Controller/API/UserController.php

+<?php
+
+declare(strict_types=1);


Any reason you wanted to change this? I don't think we want the newlines there.

vmcj · 2025-08-08T17:47:37Z

webapp/src/Controller/API/UserController.php

-             ($jsonFile && ($result = $this->importExportService->importJson('groups', $jsonFile, $message)))) &&
-            $result >= 0) {
-             return "$result new group(s) successfully added.";
+                ($jsonFile && ($result = $this->importExportService->importJson('groups', $jsonFile, $message)))) &&


I don't think you change anything here. Can either first run your linter on the file and commit or remove the changes your linter made (my preference)?

vmcj · 2025-08-08T17:51:31Z

webapp/src/Controller/API/UserController.php

+        if ($user === null) {
+            throw new NotFoundHttpException(sprintf("User with id %s not found", $id));
+        }
+        return $this->addOrUpdateUser($mutateUser, $user, $request);


The name addOrUpdate is now misleading. You only allow for update here.

vmcj · 2025-08-08T17:53:58Z

Sorry for the delay, but I finally found some time to work on it. I cannot seem to figure out how to run the tests and how to fix code-style, any help on that would be appreciated.

I dropped you a message in the DOMjudge slack to help. The tests will only run when we press the button as you haven't merged anything yet.

LuukBlankenstijn marked this pull request as ready for review July 20, 2025 10:06

LuukBlankenstijn marked this pull request as draft July 20, 2025 10:42

vmcj requested changes Jul 20, 2025

View reviewed changes

webapp/src/Controller/API/UserController.php Outdated Show resolved Hide resolved

webapp/src/DataTransferObject/AddUser.php Show resolved Hide resolved

LuukBlankenstijn changed the title ~~fix(api): seperate and fix user add and update endpoint logic~~ Fix user add and update api endpoints Aug 8, 2025

LuukBlankenstijn force-pushed the fix/api-fixes branch from 088c538 to 7873dfa Compare August 8, 2025 17:32

LuukBlankenstijn force-pushed the fix/api-fixes branch from 7873dfa to 7d5bcfe Compare August 8, 2025 17:33

vmcj requested changes Aug 8, 2025

View reviewed changes

		<?php

		declare(strict_types=1);

Fix user add and update api endpoints #3040

Are you sure you want to change the base?

Fix user add and update api endpoints #3040

Conversation

LuukBlankenstijn commented Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vmcj commented Jul 20, 2025

Uh oh!

LuukBlankenstijn commented Jul 20, 2025

Uh oh!

vmcj commented Jul 20, 2025

Uh oh!

Uh oh!

Uh oh!

vmcj commented Jul 20, 2025

Uh oh!

eldering commented Jul 20, 2025

Uh oh!

LuukBlankenstijn commented Jul 21, 2025

Uh oh!

nickygerritsen commented Jul 21, 2025

Uh oh!

LuukBlankenstijn commented Jul 21, 2025

Uh oh!

nickygerritsen commented Jul 21, 2025

Uh oh!

eldering commented Jul 21, 2025

Uh oh!

LuukBlankenstijn commented Aug 8, 2025

Uh oh!

vmcj Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

vmcj Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

vmcj Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

vmcj commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

LuukBlankenstijn commented Jul 20, 2025 •

edited

Loading

vmcj commented Aug 8, 2025 •

edited

Loading