Add basic backend for chat #429

danxuliu · 2017-10-03T07:27:22Z

This pull request adds a basic backend for the chat; the frontend will be added in later pull requests.

The backend provides two controller methods: sendMessage and receiveMessages. receiveMessages uses a long-polling approach: it returns immediately only if there are messages available; otherwise it keeps the HTTP connection open and waits for new messages, returning them as soon as they are available (in which case a new HTTP request must be done by the client). The HTTP connection is not kept open indefinitely, though; the default timeout is 30 seconds, and the maximum timeout that can be set is 60 seconds (I have used those values because they felt right; they are not related to typical values for web server timeouts or anything like that).

Note that, at this time, chat messages are never removed; a later pull request will ensure that when a room is deleted all its chat messages are also deleted.

Internally for chat messages the Comments API is used; only a subset of its features is needed (for example, there is no need for parent-child relationships in chat messages), but it fits well with the chat messages anyway and is already mature code, so using it seemed like the best approach. The Comments API is not used directly by the ChatController, though; the ChatManager class sits between them and is the one that takes care of calling the Comments API as needed and also of waiting for new messages to be available when trying to receive them.

The ChatManager uses polling to the Comments API (and this, in turn, to the database): it queries the Comments API to see if there are available messages; if there are it returns them, and if not, it sleeps for a second and queries it again, repeating this process until there are messages or the timeout expires. The good thing about this approach is that it just works; there is no need to install specific extensions, or to configure anything. The bad thing is the stress caused to the server. Therefore, the idea is to create another ChatManager that will use interprocess communication to notify and wake up other PHP process when there are messages available, thus eliminating the need to poll the database.

That more advanced ChatManager will not be a replacement but a complement for the current ChatManager though; the more advanced one will be used if possible but, if not (for example if it requires PHP extensions that are not installed), it will fallback to the basic one. Given that other ChatManager related classes/interfaces/files will be added in the future the ChatManager was placed on its own Chat directory.

Finally, although unit and integration tests are provided for the backend, for the time being there are no tests that truly verify that the long-polling works as expected; I will try to come with something later, as asynchronous tests would be really useful for the more advanced ChatManager version.

nickvergessen · 2017-10-04T09:57:19Z

lib/Controller/ChatController.php

+
+		if ($this->userId === null) {
+			$actorType = 'guests';
+			$actorId = $this->session->get('spreed-session');


I'm pretty sure this is too long?
Our session is 255, the actor id is only 64

Oops, yes, you are totally right :-S

How could a guest be identified then? Maybe storing a 64 char long hash of the session? Any other (probably better :-P ) idea?

Either that, or use a hard-coded string instead. But I don't know what this implies? Can guests edit all users messages then?
Well they can't anyway in our chat, right?

The problem with a hard-coded string is that then there would be no way to know which guest sent which message; that is why I was using the session in first place.

Regarding the edition of messages no, they can not be edited in our chat interface. Although now that you mention it maybe they can be edited through the Comments DAV or something like that... I will have to check it.

nickvergessen · 2017-10-04T10:02:11Z

I think using the comments API is a good short cut for the first version.

danxuliu · 2017-10-17T21:01:35Z

I have fixed the issue of the spreed-session being too long to be stored as an actorId by hashing the spreed-session.

Please review again :-)

nickvergessen · 2017-10-19T07:34:32Z

lib/Controller/ChatController.php

+
+		$creationDateTime = new \DateTime('now', new \DateTimeZone('UTC'));
+
+		$this->chatManager->sendMessage($token, $actorType, $actorId, $message, $creationDateTime);


Please use $room->getId() instead of $token

nickvergessen · 2017-10-19T07:40:40Z

lib/Controller/ChatController.php

+	 */
+	public function receiveMessages($token, $offset = 0, $notOlderThanTimestamp = 0, $timeout = 30) {
+		try {
+			$room = $this->manager->getRoomForParticipantByToken($token, $this->userId);


We need to extend this, so it is only accessible when the user is listed, or has a valid session.
Otherwise you can read chat messages of password protected public rooms without the password.

So please use:

spreed/lib/Controller/CallController.php

Lines 83 to 99 in 86ec1dd

try {

$room = $this->manager->getRoomForSession($this->userId, $this->session->get('spreed-session'));

} catch (RoomNotFoundException $e) {

if ($this->userId === null) {

return new DataResponse([], Http::STATUS_NOT_FOUND);

}

// For logged in users we search for rooms where they are real participants

try {

$room = $this->manager->getRoomForParticipantByToken($token, $this->userId);

$room->getParticipant($this->userId);

} catch (RoomNotFoundException $e) {

return new DataResponse([], Http::STATUS_NOT_FOUND);

} catch (ParticipantNotFoundException $e) {

return new DataResponse([], Http::STATUS_NOT_FOUND);

}

}

Same for sending Messages

If that makes your integration tests fail try And user "participant" joins call "room" with 200

The chat manager handles sending and receiving chat messages. It uses basic polling; "receiveMessages()" will repeteadly query the database for new messages, waiting a little between each query, until messages are found or the timeout expires. The Comments API is used internally, but as this class is meant to be used directly and only by a Controller, "receiveMessages()" returns an array of IComments too instead of a custom interface to save the conversion of objects. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

The ChatController uses a long-polling approach: if there are currently no messages the response will not be sent immediately; instead, HTTP connection will be kept open waiting for new messages to arrive and, when they do, then the response will be sent. The technique is simply based on the fact that ChatManager will wait (hang) until there are some messages to be returned, or until the timeout expires. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

The character limit for actorId is 64, but the spreed-session is 256 characters long, so it has to be hashed to get an ID that fits. The sha1 algorithm is used as, from all the hash functions bundled with PHP that are always available (the Hash extension can be disabled using the "--disable-hash" switch, unlike those that are part of the string functions), it generates the longest hashes (40 characters) that fit in the actorId column of the database. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Before, any user was able to send and receive messages to and from public password protected rooms, even if they were not invited and they had not joined it; guests were not able to send, but were able to receive nevertheless. Now, only invited users or users/guests that have joined a public password protected room can send and receive messages to and from it. As a side effect that change affects too to regular public rooms (not password protected), but the new behaviour makes more sense than the old one anyway. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

danxuliu · 2017-10-20T01:57:27Z

@nickvergessen I have addressed your comments in the last two commits.

nickvergessen · 2017-10-20T08:18:11Z

lib/Controller/ChatController.php

+	 * @param string $token the token for the Room.
+	 * @return \OCA\Spreed\Room|null the Room, or null if none was found.
+	 */
+	private function getRoom($token) {


little remark for the future, please use protected rather then private, makes testing and other things a lot easier

I used private because this class is not meant to be extended and because, in general, I prefer to not change a design just to make testing easier (and if I am not mistaken this change would help only to be able to mock the method, as you could invoke it to explicitly test it by using reflection).

Having said that, I can change it to protected if you wish ;-)

nickvergessen · 2017-10-25T09:33:47Z

Lets merge this for now, if there are bugs we can still fix them later

…ader-2.0.0

danxuliu added 3. to review feature: chat 💬 Chat and system messages labels Oct 3, 2017

danxuliu added this to the 3.0 (Nextcloud 13) milestone Oct 3, 2017

danxuliu requested review from nickvergessen, schiessle and Ivansss October 3, 2017 07:27

nickvergessen reviewed Oct 4, 2017

View reviewed changes

danxuliu mentioned this pull request Oct 16, 2017

Chat integration #395

Closed

7 tasks

danxuliu mentioned this pull request Oct 17, 2017

Run integration tests on other databases besides SQLite #443

Closed

danxuliu force-pushed the add-basic-backend-for-chat branch from 383e4dc to 3f38a13 Compare October 17, 2017 23:35

nickvergessen reviewed Oct 19, 2017

View reviewed changes

nickvergessen requested changes Oct 19, 2017

View reviewed changes

danxuliu added 6 commits October 20, 2017 03:49

Add integration tests for the chat

2e73166

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Use the room ID instead of the token as the object ID of chat comments

943a85d

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

danxuliu force-pushed the add-basic-backend-for-chat branch from 3f38a13 to ce91552 Compare October 20, 2017 01:53

nickvergessen reviewed Oct 20, 2017

View reviewed changes

nickvergessen approved these changes Oct 25, 2017

View reviewed changes

nickvergessen merged commit a4fbd70 into master Oct 25, 2017

nickvergessen deleted the add-basic-backend-for-chat branch October 25, 2017 09:33

marcoambrosini pushed a commit that referenced this pull request Oct 9, 2019

Merge pull request #429 from nextcloud/dependabot/npm_and_yarn/url-lo…

65f167e

…ader-2.0.0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic backend for chat #429

Add basic backend for chat #429

danxuliu commented Oct 3, 2017

nickvergessen Oct 4, 2017

danxuliu Oct 4, 2017

nickvergessen Oct 4, 2017

danxuliu Oct 4, 2017

nickvergessen commented Oct 4, 2017

danxuliu commented Oct 17, 2017

nickvergessen Oct 19, 2017

nickvergessen Oct 19, 2017

danxuliu commented Oct 20, 2017

nickvergessen Oct 20, 2017

danxuliu Oct 20, 2017

nickvergessen commented Oct 25, 2017


		$creationDateTime = new \DateTime('now', new \DateTimeZone('UTC'));

		$this->chatManager->sendMessage($token, $actorType, $actorId, $message, $creationDateTime);

	try {
	$room = $this->manager->getRoomForSession($this->userId, $this->session->get('spreed-session'));
	} catch (RoomNotFoundException $e) {
	if ($this->userId === null) {
	return new DataResponse([], Http::STATUS_NOT_FOUND);
	}

	// For logged in users we search for rooms where they are real participants
	try {
	$room = $this->manager->getRoomForParticipantByToken($token, $this->userId);
	$room->getParticipant($this->userId);
	} catch (RoomNotFoundException $e) {
	return new DataResponse([], Http::STATUS_NOT_FOUND);
	} catch (ParticipantNotFoundException $e) {
	return new DataResponse([], Http::STATUS_NOT_FOUND);
	}
	}

Add basic backend for chat #429

Add basic backend for chat #429

Conversation

danxuliu commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Oct 4, 2017

danxuliu commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danxuliu commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Oct 25, 2017