fix: empty payload returned as list #4050

geisterfurz007 · 2022-10-02T10:31:39Z

What does this PR do?

This PR adds a setPayload call to the DELETE /user/:userId/sessions/:sessionId endpoint which was previously missing causing the error described in #3916.

Test Plan

An e2e test was written for the case to prevent regressions.

Related PRs and Issues

#3916

Have you read the Contributing Guidelines on issues?

Yes.

stnguyen90

@geisterfurz007 Would you be able to either include a test case or a screenshot of before and after in the PR description?

geisterfurz007 · 2022-10-06T19:11:27Z

@stnguyen90 I could write a testcase in one of two ways:

Mock Redis (I would need a little guidance on that, I don't daily drive PHP; I also couldn't find any other code that mocks out Redis) and write a test for the full Realtime::send function
Move the serialization logic for the payload in a tiny separate function and unit test that

I suppose 2 would be significantly simpler but I wasn't sure that is sufficient for you.

A before and after of the serialization is demonstrated in the linked PHP sandbox; if a screenshot of the code / output is fine, I can certainly provide it from there, but I assume you mean from the actual backend? In case of the latter, I would opt for the test approach since I would otherwise build a full reproduction of the issue which would take quite some time, I think.

Let me know which of the options works for you!

stnguyen90 · 2022-10-06T19:22:33Z

@geisterfurz007, a test case would be an e2e test. I don't think you need to mock redis because our e2e tests run the full Appwrite stack

geisterfurz007 · 2022-10-06T20:22:08Z

Oh perfect, I will see what I can do then, thanks!

geisterfurz007 · 2022-10-25T16:06:39Z

I fiddled around with this for a while but I am not really sure what to do with this, to be honest. With the scenario described in the linked issue, I could not replicate the issue in the tests (there was an assertion for a non-empty payload in there even!) and I made an ugly attempt to manually send an empty payload but the receive call times out:

    public function testPayloadFormatOnDisconnect(): void
    {
        $client = $this->getWebsocket(['accounts']);
        $response = $client->receive();
        Realtime::send($this->getProject()['$id'], array(), array(), ['accounts'], ['role']);

        $response = $client->receive();

        $this->assertEquals('baked beans', $response); // Desperate way to get the content of $response into the console for debugging
    }

I would appreciate some guidance here; as said, I don't daily drive PHP and I have been struggling with this for some time now 😕

stnguyen90 · 2022-10-25T17:26:52Z

I would appreciate some guidance here; as said, I don't daily drive PHP and I have been struggling with this for some time now 😕

@geisterfurz007, here's a test where:

subscription is started:

appwrite/tests/e2e/Services/Realtime/RealtimeConsoleClientTest.php

Lines 384 to 397 in 4942ba9

    
           $client = $this->getWebsocket(['console'], [ 
        
               'origin' => 'http://localhost', 
        
               'cookie' => 'a_session_console=' . $this->getRoot()['session'], 
        
           ], $projectId); 
        
           $response = json_decode($client->receive(), true); 
        
           $this->assertArrayHasKey('type', $response); 
        
           $this->assertArrayHasKey('data', $response); 
        
           $this->assertEquals('connected', $response['type']); 
        
           $this->assertNotEmpty($response['data']); 
        
           $this->assertCount(1, $response['data']['channels']); 
        
           $this->assertContains('console', $response['data']['channels']); 
        
           $this->assertNotEmpty($response['data']['user']);

API call is made to delete an attribute:

appwrite/tests/e2e/Services/Realtime/RealtimeConsoleClientTest.php

Lines 402 to 408 in 4942ba9

    
           $attribute = $this->client->call(Client::METHOD_DELETE, '/databases/' . $databaseId . '/collections/' . $data['actorsId'] . '/attributes/name', array_merge([ 
        
               'content-type' => 'application/json', 
        
               'x-appwrite-project' => $this->getProject()['$id'], 
        
           ], $this->getHeaders())); 
        
           $this->assertEquals($attribute['headers']['status-code'], 204); 
        
           $attributeKey = 'name';

The realtime event is verified:

appwrite/tests/e2e/Services/Realtime/RealtimeConsoleClientTest.php

Lines 409 to 426 in 4942ba9

    
           $response = json_decode($client->receive(), true); 
        
           $this->assertArrayHasKey('type', $response); 
        
           $this->assertArrayHasKey('data', $response); 
        
           $this->assertEquals('event', $response['type']); 
        
           $this->assertNotEmpty($response['data']); 
        
           $this->assertArrayHasKey('timestamp', $response['data']); 
        
           $this->assertCount(1, $response['data']['channels']); 
        
           $this->assertContains('console', $response['data']['channels']); 
        
           $this->assertContains("databases.{$databaseId}.collections.{$actorsId}.attributes.*.delete", $response['data']['events']); 
        
           $this->assertContains("databases.{$databaseId}.collections.{$actorsId}.attributes.*", $response['data']['events']); 
        
           $this->assertContains("databases.{$databaseId}.collections.{$actorsId}", $response['data']['events']); 
        
           $this->assertContains("databases.{$databaseId}.collections.*.attributes.*.delete", $response['data']['events']); 
        
           $this->assertContains("databases.{$databaseId}.collections.*.attributes.*", $response['data']['events']); 
        
           $this->assertContains("databases.{$databaseId}.collections.*", $response['data']['events']); 
        
           $this->assertNotEmpty($response['data']['payload']); 
        
           $client->close();

Maybe you can use this for guidance.

geisterfurz007 · 2022-10-25T18:11:53Z

Right @stnguyen90, and there is one testcase that runs what I understood to be the problematic call in #3916:

appwrite/tests/e2e/Services/Realtime/RealtimeCustomClientTest.php

Lines 501 to 528 in 4942ba9

    
           $this->client->call(Client::METHOD_DELETE, '/account/sessions/' . $sessionNewId, array_merge([ 
        
               'origin' => 'http://localhost', 
        
               'content-type' => 'application/json', 
        
               'x-appwrite-project' => $projectId, 
        
               'cookie' => 'a_session_' . $projectId . '=' . $sessionNew, 
        
           ])); 
        
           $response = json_decode($client->receive(), true); 
        
           $this->assertArrayHasKey('type', $response); 
        
           $this->assertArrayHasKey('data', $response); 
        
           $this->assertEquals('event', $response['type']); 
        
           $this->assertNotEmpty($response['data']); 
        
           $this->assertCount(2, $response['data']['channels']); 
        
           $this->assertArrayHasKey('timestamp', $response['data']); 
        
           $this->assertContains('account', $response['data']['channels']); 
        
           $this->assertContains('account.' . $userId, $response['data']['channels']); 
        
           $this->assertContains("users.{$userId}.sessions.{$sessionNewId}.delete", $response['data']['events']); 
        
           $this->assertContains("users.{$userId}.sessions.{$sessionNewId}", $response['data']['events']); 
        
           $this->assertContains("users.{$userId}.sessions.*.delete", $response['data']['events']); 
        
           $this->assertContains("users.{$userId}.sessions.*", $response['data']['events']); 
        
           $this->assertContains("users.{$userId}", $response['data']['events']); 
        
           $this->assertContains("users.*.sessions.{$sessionNewId}.delete", $response['data']['events']); 
        
           $this->assertContains("users.*.sessions.{$sessionNewId}", $response['data']['events']); 
        
           $this->assertContains("users.*.sessions.*.delete", $response['data']['events']); 
        
           $this->assertContains("users.*.sessions.*", $response['data']['events']); 
        
           $this->assertContains("users.*", $response['data']['events']); 
        
           $this->assertNotEmpty($response['data']['payload']);

However in that testcase, there is an explicit assertion against an empty (deserialized) payload and indeed I could not reproduce an empty payload which is why I attempted to manually send one since I could not find any action that sends out an empty payload.

stnguyen90 · 2022-10-25T21:00:23Z

@geisterfurz007, ah....looks like the problem happens specifically if 1 session is deleted from the Console.

geisterfurz007 · 2022-10-25T21:16:27Z

@stnguyen90 How about this approach then to make this tested in a bit more isolation (before I go about and more or less randomly throw PHP around):

I move the json encoding of the payload / realtime JSON body to its own function and unit test that.

It might not be a test for the full problem case specifically in the issue but should still cover what ended up crashing the JSON parser in flutter (returning an empty list instead of an empty object).

stnguyen90 · 2022-10-26T15:39:45Z

How about this approach then to make this tested in a bit more isolation (before I go about and more or less randomly throw PHP around)

So, I may have gone down the wrong rabbit hole on this problem. Apologies for that. The payload not be empty for a real event. In this case, it should be the deleted session. We do it for account.deleteSession():

appwrite/app/controllers/api/account.php

Lines 1823 to 1827 in 73bce1d

    
           $events 
        
               ->setParam('userId', $user->getId()) 
        
               ->setParam('sessionId', $session->getId()) 
        
               ->setPayload($response->output($session, Response::MODEL_SESSION)) 
        
           ;

but the problem is it's missing from the Users API:

appwrite/app/controllers/api/users.php

Lines 1005 to 1007 in 73bce1d

    
           $events 
        
               ->setParam('userId', $user->getId()) 
        
               ->setParam('sessionId', $sessionId);

Maybe you can add a test like

appwrite/tests/e2e/Services/Realtime/RealtimeCustomClientTest.php

Lines 501 to 528 in 4942ba9

    
           $this->client->call(Client::METHOD_DELETE, '/account/sessions/' . $sessionNewId, array_merge([ 
        
               'origin' => 'http://localhost', 
        
               'content-type' => 'application/json', 
        
               'x-appwrite-project' => $projectId, 
        
               'cookie' => 'a_session_' . $projectId . '=' . $sessionNew, 
        
           ])); 
        
           $response = json_decode($client->receive(), true); 
        
           $this->assertArrayHasKey('type', $response); 
        
           $this->assertArrayHasKey('data', $response); 
        
           $this->assertEquals('event', $response['type']); 
        
           $this->assertNotEmpty($response['data']); 
        
           $this->assertCount(2, $response['data']['channels']); 
        
           $this->assertArrayHasKey('timestamp', $response['data']); 
        
           $this->assertContains('account', $response['data']['channels']); 
        
           $this->assertContains('account.' . $userId, $response['data']['channels']); 
        
           $this->assertContains("users.{$userId}.sessions.{$sessionNewId}.delete", $response['data']['events']); 
        
           $this->assertContains("users.{$userId}.sessions.{$sessionNewId}", $response['data']['events']); 
        
           $this->assertContains("users.{$userId}.sessions.*.delete", $response['data']['events']); 
        
           $this->assertContains("users.{$userId}.sessions.*", $response['data']['events']); 
        
           $this->assertContains("users.{$userId}", $response['data']['events']); 
        
           $this->assertContains("users.*.sessions.{$sessionNewId}.delete", $response['data']['events']); 
        
           $this->assertContains("users.*.sessions.{$sessionNewId}", $response['data']['events']); 
        
           $this->assertContains("users.*.sessions.*.delete", $response['data']['events']); 
        
           $this->assertContains("users.*.sessions.*", $response['data']['events']); 
        
           $this->assertContains("users.*", $response['data']['events']); 
        
           $this->assertNotEmpty($response['data']['payload']);

but use the delete user session API with something like:

appwrite/tests/e2e/Services/Realtime/RealtimeCustomClientTest.php

Lines 1323 to 1328 in 4942ba9

    
           // Cleanup : Delete function 
        
           $response = $this->client->call(Client::METHOD_DELETE, '/functions/' . $functionId, [ 
        
               'content-type' => 'application/json', 
        
               'x-appwrite-project' => $this->getProject()['$id'], 
        
               'x-appwrite-key' => $this->getProject()['apiKey'], 
        
           ], []);

geisterfurz007 · 2022-10-26T16:31:17Z

@stnguyen90 I'm super sorry but I am getting more and more confused 😅 The API you linked appears to be the one used in the test I referred to (targeting '/account/sessions/' . $sessionNewId as endpoint). The error also does not seem to be reproducible with a DELETE request to /account/sessions either.

P.S. If that makes things easier to coordinate, we can also move this discussion to Discord; I am geisterfurz007#5952 over there (also on the Appwrite server).

stnguyen90 · 2022-10-26T17:15:22Z

I'm super sorry but I am getting more and more confused

So sorry! Yes, the Delete Account Session test is working as expected. I was suggesting creating a new test that would delete a session using the Users API like:

 $response = $this->client->call(Client::METHOD_DELETE, '/users/' . $userId . '/sessions' . $sessionId, [ 
     'content-type' => 'application/json', 
     'x-appwrite-project' => $this->getProject()['$id'], 
     'x-appwrite-key' => $this->getProject()['apiKey'], 
 ], []);

Meldiron · 2022-10-31T16:14:49Z

Thank you so much for the PR 🤩. We're adding the hacktoberfest-accepted label to ensure this PR counts towards your Hacktoberfest contributions count. With that said, please stay active on this PR to address any comments once you receive a review. Happy Hacktoberfest! 🎃

stnguyen90

@geisterfurz007 something seems off with the CI...Perhaps syncing with master would fix this?

geisterfurz007 · 2022-10-31T21:05:07Z

They ran through at least but a failure on a method I didn't touch 😕 I don't know if that's something that is looking familiar or if I should give it another shot locally tomorrow. What do you think @stnguyen90 ?

stnguyen90

Seems like it was a flaky test. LGTM now! Thanks so much for this! 🙏🏼

stnguyen90 · 2022-11-17T18:13:54Z

@TorstenDittmann or @christyjacob4, would you please merge when you get a chance?

christyjacob4 · 2022-12-20T08:07:27Z

THANK YOU! All changes merged 🥳

Please reach out to me on our Discord server if you would like to claim your Appwrite swags! As a way of saying thank you, we would also love to invite you to join the Appwrite organization on GitHub. Please share your GitHub username with us on Discord.

You can accept the invite by visiting https://github.com/orgs/appwrite/invitation. By joining our team, you will officially be an Appwrite maintainer on GitHub.

You can change your membership visibility settings, so your new Appwrite team membership badge will show up on your personal GitHub profile.

Please feel free to look for more PRs you might be interested in helping with on our long list of Hacktoberfest friendly issues and help make Appwrite better :)

stnguyen90 requested changes Oct 5, 2022

View reviewed changes

geisterfurz007 force-pushed the fix-3916-empty-payload-returned-as-object branch 2 times, most recently from 75ddaaa to 1bd2249 Compare October 27, 2022 17:38

stnguyen90 self-requested a review October 29, 2022 04:57

Meldiron added the hacktoberfest-accepted Accepted for Hacktoberfest, will be merged later label Oct 31, 2022

stnguyen90 requested changes Oct 31, 2022

View reviewed changes

fix: empty payload deleting single user session

e891424

geisterfurz007 force-pushed the fix-3916-empty-payload-returned-as-object branch from 1bd2249 to e891424 Compare October 31, 2022 20:02

stnguyen90 approved these changes Nov 2, 2022

View reviewed changes

stnguyen90 linked an issue Nov 2, 2022 that may be closed by this pull request

🐛 Bug Report: Incorrect Realtime Payload when Deleting Session from Console #3916

Closed

2 tasks

stnguyen90 requested review from TorstenDittmann and christyjacob4 November 17, 2022 18:13

christyjacob4 merged commit cb0a927 into appwrite:master Dec 20, 2022

geisterfurz007 deleted the fix-3916-empty-payload-returned-as-object branch December 20, 2022 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: empty payload returned as list #4050

fix: empty payload returned as list #4050

geisterfurz007 commented Oct 2, 2022 •

edited

Loading

stnguyen90 left a comment

geisterfurz007 commented Oct 6, 2022

stnguyen90 commented Oct 6, 2022

geisterfurz007 commented Oct 6, 2022

geisterfurz007 commented Oct 25, 2022 •

edited

Loading

stnguyen90 commented Oct 25, 2022

geisterfurz007 commented Oct 25, 2022 •

edited

Loading

stnguyen90 commented Oct 25, 2022

geisterfurz007 commented Oct 25, 2022

stnguyen90 commented Oct 26, 2022 •

edited

Loading

geisterfurz007 commented Oct 26, 2022 •

edited

Loading

stnguyen90 commented Oct 26, 2022

Meldiron commented Oct 31, 2022

stnguyen90 left a comment

geisterfurz007 commented Oct 31, 2022

stnguyen90 left a comment

stnguyen90 commented Nov 17, 2022

christyjacob4 commented Dec 20, 2022

fix: empty payload returned as list #4050

fix: empty payload returned as list #4050

Conversation

geisterfurz007 commented Oct 2, 2022 • edited Loading

What does this PR do?

Test Plan

Related PRs and Issues

Have you read the Contributing Guidelines on issues?

stnguyen90 left a comment

Choose a reason for hiding this comment

geisterfurz007 commented Oct 6, 2022

stnguyen90 commented Oct 6, 2022

geisterfurz007 commented Oct 6, 2022

geisterfurz007 commented Oct 25, 2022 • edited Loading

stnguyen90 commented Oct 25, 2022

geisterfurz007 commented Oct 25, 2022 • edited Loading

stnguyen90 commented Oct 25, 2022

geisterfurz007 commented Oct 25, 2022

stnguyen90 commented Oct 26, 2022 • edited Loading

geisterfurz007 commented Oct 26, 2022 • edited Loading

stnguyen90 commented Oct 26, 2022

Meldiron commented Oct 31, 2022

stnguyen90 left a comment

Choose a reason for hiding this comment

geisterfurz007 commented Oct 31, 2022

stnguyen90 left a comment

Choose a reason for hiding this comment

stnguyen90 commented Nov 17, 2022

christyjacob4 commented Dec 20, 2022

geisterfurz007 commented Oct 2, 2022 •

edited

Loading

geisterfurz007 commented Oct 25, 2022 •

edited

Loading

geisterfurz007 commented Oct 25, 2022 •

edited

Loading

stnguyen90 commented Oct 26, 2022 •

edited

Loading

geisterfurz007 commented Oct 26, 2022 •

edited

Loading