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

Implement client single event retrieval - #671 #693

Merged
merged 21 commits into from Aug 9, 2019

Conversation

@Cnly
Copy link
Collaborator

commented Mar 10, 2019

This should fix #671.
Implemented spec: https://matrix.org/docs/spec/client_server/r0.4.0.html#get-matrix-client-r0-rooms-roomid-event-eventid

When the event requested is not found locally, a federation query is done.

Signed-off-by: Alex Chen minecnly@gmail.com

@APwhitehat APwhitehat self-assigned this Mar 14, 2019

@APwhitehat
Copy link
Collaborator

left a comment

Looks good otherwise.
I am rusty with federation things, but this seems like the right way.

@APwhitehat APwhitehat removed their assignment Mar 14, 2019

@Cnly Cnly force-pushed the Cnly:single-event-retrieval-671 branch from 51b16b9 to d134c2d May 22, 2019

@Cnly Cnly force-pushed the Cnly:single-event-retrieval-671 branch from d134c2d to f46d41b Jun 28, 2019

All issues from this review have been resolved.

@Cnly

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 28, 2019

This PR is now complete and ready for review.

@anoadragon453 anoadragon453 requested a review from matrix-org/dendrite-core Jul 5, 2019

clientapi/routing/getevent.go Outdated Show resolved Hide resolved
clientapi/routing/getevent.go Outdated Show resolved Hide resolved
clientapi/routing/getevent.go Outdated Show resolved Hide resolved
clientapi/routing/getevent.go Outdated Show resolved Hide resolved
clientapi/routing/getevent.go Outdated Show resolved Hide resolved
clientapi/routing/routing.go Outdated Show resolved Hide resolved
clientapi/routing/routing.go Outdated Show resolved Hide resolved

@anoadragon453 anoadragon453 added this to In progress in Homeserver Task Board via automation Jul 9, 2019

@anoadragon453 anoadragon453 moved this from In progress to Community PRs in Homeserver Task Board Jul 9, 2019

Cnly and others added some commits Jul 10, 2019

Apply suggestions from code review
Co-Authored-By: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Use common.URLDecodeMapValues
Signed-off-by: Alex Chen <minecnly@gmail.com>
Extracting domain from event ID won't work for rooms >= v3
Signed-off-by: Alex Chen <minecnly@gmail.com>

@anoadragon453 anoadragon453 self-assigned this Jul 11, 2019

@anoadragon453 anoadragon453 requested a review from matrix-org/dendrite-core Jul 11, 2019

Cnly added some commits Jul 18, 2019

Better error for unexpected "room doesn't exist"
Signed-off-by: Alex Chen <minecnly@gmail.com>
Remove federation queries for event and missing states
Signed-off-by: Alex Chen <minecnly@gmail.com>
@anoadragon453

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

This is getting there, but some of the keys returned are reserved for federation usage, and shouldn't be returned to the client.

He's an example of an event from #synapse-dev retrieved from this endpoint:

{
  "content": {
    "body": "It doesn't get into this detail on sampling.",
    "msgtype": "m.text"
  },
  "event_id": "$1563457856313896kMvOF:matrix.org",
  "origin_server_ts": 1563457856029,
  "age": 135746,
  "unsigned": {
    "age": 135746
  },
  "room_id": "!yZHTGeDKZUeKaqeTeU:matrix.org",
  "type": "m.room.message",
  "user_id": "@jorik:matrix.org",
  "sender": "@jorik:matrix.org"
}

Whereas what you're currently returning is:

{
  "auth_events": [
    [
      "$gZA0cqOQsKZDeO4m:localhost",
      {
        "sha256": "SX2ZVFcyQDIxQeeRQKiHW5u2bvS9a9nzQm4Kdpqe9IA"
      }
    ],
    [
      "$EMEVWuH7ngPcDBbR:localhost",
      {
        "sha256": "SfEUZUKMPxRfz\/1b0yGVudGIwxqal2HE5skAa76rrGA"
      }
    ],
    [
      "$YsfhmpnILNfbbdGs:localhost",
      {
        "sha256": "UkTK9vypi2evIq7D\/DCznvuMPhqMeG\/cMpj5LIeZcLo"
      }
    ]
  ],
  "content": {
    "body": "sfgd",
    "msgtype": "m.text"
  },
  "depth": 105,
  "event_id": "$RI8BkqLoZSwkk4cv:localhost",
  "hashes": {
    "sha256": "5fYsiRuMg\/IzPnAeTu2f5JjaCLaG7lFUFMCqkcCyr\/g"
  },
  "origin": "localhost",
  "origin_server_ts": 1562943972009,
  "prev_events": [
    [
      "$4bSB3hgis0mVqTSZ:localhost",
      {
        "sha256": "MAdcZ\/46AZZl45ONkrHBS0k17YwX7c8RurL4AxqbJ38"
      }
    ]
  ],
  "room_id": "!MijZB0lPxJATZzgf:localhost",
  "sender": "@admin:localhost",
  "signatures": {
    "localhost": {
      "ed25519:yFTe": "+e4OL8bUQjrZ0cyHSlu6RONp3PqWMG3KUpDC2sYmaWar2P8LxAI41VB2gwsF5hss9uanVxXqGCgdxLm6\/40PDg"
    }
  },
  "type": "m.room.message"
}
@Cnly

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2019

The linting issue for this PR is fixed in #752, so we can see if that can be merged first.

Return the requested events in client event format
Signed-off-by: Alex Chen <minecnly@gmail.com>
@Cnly

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2019

This is getting there, but some of the keys returned are reserved for federation usage, and shouldn't be returned to the client.

gomatrixserverlib.ToClientEvent should help.

All issues resolved.

@Cnly

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 25, 2019

Re-requesting review from @anoadragon453. Doing this manually because GitHub keeps giving me 404 when I click the re-request button...

@anoadragon453
Copy link
Member

left a comment

Nearly! One case got caught in testing though. Also errors typically don't have punctuation.

clientapi/routing/getevent.go Outdated Show resolved Hide resolved
clientapi/routing/getevent.go Outdated Show resolved Hide resolved
clientapi/routing/getevent.go Outdated Show resolved Hide resolved
clientapi/routing/getevent.go Outdated Show resolved Hide resolved

Cnly and others added some commits Jul 26, 2019

Apply suggestions from code review
Co-Authored-By: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Simplify code
Signed-off-by: Alex Chen <minecnly@gmail.com>
Fix unhandled error
Signed-off-by: Alex Chen <minecnly@gmail.com>
Get the correct subset of state needed at the event
Signed-off-by: Alex Chen <minecnly@gmail.com>
@Cnly

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 26, 2019

Umm, now we have yet another string join has 3 occurrences, make it a constant.

@Cnly

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 26, 2019

Linting for this PR won't pass perhaps until #765.
Three tests are blocked from passing by matrix-org/sytest#653 and matrix-org/sytest#652.

Cnly added some commits Aug 6, 2019

Add newly passing tests
Signed-off-by: Alex Chen <minecnly@gmail.com>
clientapi/routing/getevent.go Outdated Show resolved Hide resolved

return util.JSONResponse{
Code: http.StatusNotFound,
JSON: jsonerror.NotFound("The event was not found or you do not have permission to read this event."),

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Aug 6, 2019

Member
Suggested change
JSON: jsonerror.NotFound("The event was not found or you do not have permission to read this event."),
JSON: jsonerror.NotFound("The event was not found or you do not have permission to read this event"),

It might be nice to consolidate this error text as well, lest we want to change it.

Cnly added some commits Aug 6, 2019

Lint
Signed-off-by: Alex Chen <minecnly@gmail.com>
Merge branch 'master' into single-event-retrieval-671
Signed-off-by: Alex Chen <minecnly@gmail.com>

@Cnly Cnly merged commit aa0d22b into matrix-org:master Aug 9, 2019

8 checks passed

buildkite/dendrite Build #220 passed (3 minutes, 11 seconds)
Details
buildkite/dendrite/build-slash-go-1-dot-11 Passed (52 seconds)
Details
buildkite/dendrite/build-slash-go-1-dot-12 Passed (50 seconds)
Details
buildkite/dendrite/lint-slash-go-1-dot-12 Passed (1 minute, 39 seconds)
Details
buildkite/dendrite/pipeline Passed (7 seconds)
Details
buildkite/dendrite/unit-tests-slash-go-1-dot-11 Passed (55 seconds)
Details
buildkite/dendrite/unit-tests-slash-go-1-dot-12 Passed (1 minute, 16 seconds)
Details
ci/circleci: dendrite Your tests passed on CircleCI!
Details

Homeserver Task Board automation moved this from Community PRs to Done Aug 9, 2019

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