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.

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

Implements room tagging. #694

Open
wants to merge 53 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@SUMUKHA-PK
Copy link
Contributor

commented Mar 13, 2019

Solves issue #657

@SUMUKHA-PK

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Wow the notification was lost in a bunch of emails.
Getting to this today!

@SUMUKHA-PK

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

@Cnly @anoadragon453 further ?

@Cnly
Copy link
Collaborator

left a comment

Not big problems; looking forward to seeing support for room tagging!

Show resolved Hide resolved clientapi/routing/room_tagging.go Outdated
Show resolved Hide resolved clientapi/routing/room_tagging.go Outdated
Show resolved Hide resolved clientapi/routing/room_tagging.go
}

if len(data) > 0 {
dataByte, err := json.Marshal(data)

This comment has been minimized.

Copy link
@Cnly

Cnly Jul 13, 2019

Collaborator

Then it should probably be called byteData... It's like accountDB, httpRequest, etc. But still dataBytes for this variable makes the most sense to me.

Show resolved Hide resolved clientapi/routing/room_tagging.go Outdated
Show resolved Hide resolved clientapi/routing/room_tagging.go Outdated
Show resolved Hide resolved clientapi/routing/room_tagging.go Outdated
Show resolved Hide resolved clientapi/routing/routing.go Outdated
return err
}
if err = accountDB.SaveAccountData(
req.Context(), localpart, roomID, "tag", string(newTagData),

This comment has been minimized.

Copy link
@Cnly

Cnly Jul 13, 2019

Collaborator

Perhaps we should call the account data type "tags" instead of "tag"?

This comment has been minimized.

Copy link
@SUMUKHA-PK

SUMUKHA-PK Jul 13, 2019

Author Contributor

Well, delete and put are still operating on a single tag right? Maybe @anoadragon453 has an opinion?

This comment has been minimized.

Copy link
@Cnly

Cnly Jul 13, 2019

Collaborator

There's no conflict though, because Put and Delete take a list out from DB, modify the list, and put a list back into DB. BTW I'm talking about the type column in the database table, if I've created any confusion.

This comment has been minimized.

Copy link
@SUMUKHA-PK

SUMUKHA-PK Jul 13, 2019

Author Contributor

It was clear.

This comment has been minimized.

Copy link
@SUMUKHA-PK

SUMUKHA-PK Jul 13, 2019

Author Contributor

I meant, "tag" kinda signifies a single tag being operated on and not necessarily multiple in the case of Put and Delete which is not in the case of Get. So a plural is not necessary is what I was thinking.

This comment has been minimized.

Copy link
@Cnly

Cnly Jul 18, 2019

Collaborator

Did some more investigation and probably this can only be m.tag: #694 (comment)

This comment has been minimized.

Copy link
@SUMUKHA-PK

SUMUKHA-PK Jul 18, 2019

Author Contributor

As I said, the tests seem to be still failing. I'll need some help in digging up the problems maybe.

Show resolved Hide resolved clientapi/routing/room_tagging.go Outdated
@Cnly
Copy link
Collaborator

left a comment

Still some tiny little non-critical things but generally good to go I think.

Show resolved Hide resolved clientapi/routing/room_tagging.go Outdated
Show resolved Hide resolved clientapi/routing/room_tagging.go Outdated
Show resolved Hide resolved clientapi/routing/room_tagging.go Outdated
Show resolved Hide resolved clientapi/routing/room_tagging.go Outdated
Show resolved Hide resolved clientapi/routing/room_tagging.go Outdated
Show resolved Hide resolved clientapi/routing/routing.go Outdated
Show resolved Hide resolved go.mod Outdated

SUMUKHA-PK added some commits Jul 14, 2019

@Cnly
Copy link
Collaborator

left a comment

lgtm!

@Cnly Cnly dismissed their stale review Jul 15, 2019

Something doesn't seem right... Checking for it.

@Cnly

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

It seems that the order parameter in the PUT request is not implemented by this PR. Is there a reason for this? Nevermind, this is implemented indeed.

Also, these tags-related tests are not passing, whereas they should:

not ok 466 (expected fail) Can list tags for a room # TODO expected fail
# HTTP Request failed ( 500 Internal Server Error https://localhost:8800/_matrix/client/r0/user/@anon-20190714_142835-493:localhost:8800/rooms/!92ycOLCrToQgN4Xp:localhost:8800/tags?access_token=QSp9UmQwctNDGyBmBh3X0ZfU9Aqt59XPWf2qpcOZ6fI ) from GET https://localhost:8800/_matrix/client/r0/user/@anon-20190714_142835-493:localhost:8800/rooms/!92ycOLCrToQgN4Xp:localhost:8800/tags?...
# {"errcode":"M_UNKNOWN","error":"Internal Server Error"}
not ok 470 (expected fail) Tags appear in an initial v2 /sync # TODO expected fail
# Expected a m.tag event at tests/42tags.pl line 226.
# 0.068206: Registered new user @anon-20190714_142835-497:localhost:8800
# 0.128654: Private User Data:
# [
#   {
#     content => { tags => { test_tag => { order => JSON::number(1) } } },
#     event_id => "",
#     origin_server_ts => JSON::number(0),
#     sender => "",
#     type => "tag",
#   },
# ]
not ok 472 (expected fail) Deleted tags appear in an incremental v2 /sync # TODO expected fail
# HTTP Request failed ( 500 Internal Server Error https://localhost:8800/_matrix/client/r0/user/@anon-20190714_142835-499:localhost:8800/rooms/!jsdVN08DQTOKr91z:localhost:8800/tags/test_tag?access_token=nrFGqdpBhhwWqNwacBtV23_rc6EDlrRC6StoknPBtPo ) from DELETE https://localhost:8800/_matrix/client/r0/user/@anon-20190714_142835-499:localhost:8800/rooms/!jsdVN08DQTOKr91z:localhost:8800/tags/test_tag?...
# {"errcode":"M_UNKNOWN","error":"Internal Server Error"}

(Note: For test 472, it failed when deleting a tag, which implies some possible problems in tag deletion.)

Should have found and pointed out these earlier :/ Sorry!

(Note to myself: We should track in an issue that account data doesn't have its own sync stream position yet. And once that's fixed, we should add /sync support for room tagging.)

@SUMUKHA-PK

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

Id love to know more about /sync so I'd like to look into it. I'll get back to this in a while and sort the test issues out, currently travelling back to college.

@Cnly

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

Id love to know more about /sync so I'd like to look into it. I'll get back to this in a while and sort the test issues out, currently traveling back to college.

I spent some time reading the current /sync implementation for account data, and it turns out you shouldn't have to wait for account data's own stream position to add /sync support for room tags.

So if you'd like to add that, you basically just need to call syncProducer.SendData to notify syncapi about tag modifications, which shouldn't be hard.

@SUMUKHA-PK

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

Id love to know more about /sync so I'd like to look into it. I'll get back to this in a while and sort the test issues out, currently traveling back to college.

What exactly does this mean?

I spent some time reading the current /sync implementation for account data, and it turns out you shouldn't have to wait for account data's own stream position to add /sync support for room tags.

So if you'd like to add that, you basically just need to call syncProducer.SendData to notify syncapi about tag modifications, which shouldn't be hard.
Ah, so there's a queue-ish thing where you push all the changes to sync to work on (is it something like this) ?

@SUMUKHA-PK

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

Summary :

  1. Testing if: Can list tags for a room... FAIL:
  2. Testing if: Tags appear in the v1 /events stream... FAIL:
  3. Testing if: Tags appear in an initial v2 /sync... FAIL:
  4. Testing if: Newly updated tags appear in an incremental v2 /sync... FAIL:
  5. Testing if: Deleted tags appear in an incremental v2 /sync... FAIL:

So, 1 is a fault of GET tags?!

Not really sure of 2,3,4,5....

@Cnly

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

Ah, so there's a queue-ish thing where you push all the changes to sync to work on (is it something like this) ?

Yep, there is a message queue between clientapi and syncapi. When clients modify their room tags through clientapi, you need to push the changes over to syncapi.

I'm not able to exactly reproduce these errors in sytest. What exactly did you do?

I looked at the test results in the latest CI build for this PR, which should include running all SyTest tests. You can run sytest with ./run-tests.pl -I Dendrite::Monolith -d path/to/dendrite/bin tests/42tags.pl for only tests related to room tagging (add -C if you want to see the actual HTTP traffic between sytest and dendrite).

@Cnly

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

Testing if: Tags appear in the v1 /events stream... FAIL:

I believe there's no support for the legacy /events ednpoint at all now, so you'll probably just ignore this one.

@SUMUKHA-PK

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

So if you'd like to add that, you basically just need to call syncProducer.SendData to notify syncapi about tag modifications, which shouldn't be hard.

That's all? Itll pass 3,4,5 tests(numbered as above summary) ?

@Cnly

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2019

That's all? Itll pass 3,4,5 tests(numbered as above summary) ?

With other fixes I expect it should. If you look at the log for Tags appear in an initial v2 /sync, you'll see the failure is actually caused by a wrong type field while the event itself has appeared:

# Expected a m.tag event at tests/42tags.pl line 226.
# 0.068206: Registered new user @anon-20190714_142835-497:localhost:8800
# 0.128654: Private User Data:
# [
#   {
#     content => { tags => { test_tag => { order => JSON::number(1) } } },
#     event_id => "",
#     origin_server_ts => JSON::number(0),
#     sender => "",
#     type => "tag",
#   },
# ]
@SUMUKHA-PK

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

You mean the dataType field that was mentioned in this link must be set as "tag" ?

I did that for now and the error persists.

I seem to not have understood it. So, everytime the clientapi does something relevant to /sync's requirements, it is sent to /syncapi by

if err := syncProducer.SendData(userID, roomID, dataType); err != nil {
		return httputil.LogThenError(req, err)
}

which'll add the data to the syncapi's queue and technically the tests 3,4,5 are supposed to pass, yes?

Please do correct me if Im wrong.

@Cnly

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2019

You mean the dataType field that was mentioned in this link must be set as "tag" ?

If I haven't mistaken It should be set to m.tag.

It turns out that this dataType you'll send to syncapi, as well as the account data type in the DB we talked about earlier, should both be set to m.tag because this value is used as the event type for the account data, and tag events require an m.tag type.

@SUMUKHA-PK

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

It turns out that this dataType you'll send to syncapi, as well as the account data type in the DB we talked about earlier, should both be set to m.tag because this value is used as the event type for the account data, and tag events require an m.tag type.

Ah! So the /sync searches for m.tag and doesnt find them in the DB similar to /sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.