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

Ability to search entire room history after upgrading room #4415

Merged
merged 9 commits into from Jan 25, 2019

Conversation

Projects
None yet
4 participants
@anoadragon453
Copy link
Member

anoadragon453 commented Jan 18, 2019

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off

When searching a subset of rooms, we check each one for any predecessor rooms, and those for any predecessor rooms and so on, and return all results to the client. No change when searching 'all rooms' as predecessor rooms are already included in this (though it shows for "My Room", "My Room" and "My Room". Maybe it should say "My Room - v1", "My Room - v2" and "My Room - v3" instead).

Closes #4410

anoadragon453 added some commits Jan 18, 2019

Search for messages across predecessor rooms
Signed-off-by: Andrew Morgan <andrew@amorgan.xyz>

@anoadragon453 anoadragon453 force-pushed the anoa/full_search_upgraded_rooms branch from 6221cd7 to cb80db8 Jan 18, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 18, 2019

Codecov Report

Merging #4415 into develop will decrease coverage by 0.06%.
The diff coverage is 10.71%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4415      +/-   ##
===========================================
- Coverage    73.68%   73.62%   -0.07%     
===========================================
  Files          300      300              
  Lines        29703    29731      +28     
  Branches      4882     4887       +5     
===========================================
+ Hits         21887    21888       +1     
- Misses        6386     6412      +26     
- Partials      1430     1431       +1
Impacted Files Coverage Δ
synapse/storage/state.py 84.97% <11.11%> (-1.6%) ⬇️
synapse/api/filtering.py 95.42% <25%> (-1.9%) ⬇️
synapse/handlers/search.py 74.01% <6.66%> (-6.24%) ⬇️
synapse/state/v1.py 90.69% <0%> (-1.56%) ⬇️
synapse/app/homeserver.py 57% <0%> (-0.34%) ⬇️
synapse/handlers/user_directory.py 71.38% <0%> (+0.3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bfa735...a383289. Read the comment docs.

@anoadragon453 anoadragon453 requested a review from matrix-org/synapse-core Jan 21, 2019

@@ -444,6 +444,9 @@ def lazy_load_members(self):
def include_redundant_members(self):
return self.filter_json.get("include_redundant_members", False)

def add_room_ids(self, room_ids):

This comment has been minimized.

@richvdh

richvdh Jan 21, 2019

Member

Filters are currently immutable, and I think that's a good thing for being able to reason about them. One way to do this would be to have a with_room_ids method which returns a new Filter, having replaced the room list.

Needs a docstring too, please.

@@ -139,6 +187,27 @@ def search(self, user, content, batch=None):

room_ids = search_filter.filter_rooms(room_ids)

This comment has been minimized.

@richvdh

richvdh Jan 21, 2019

Member

should this not happen after we add the historical rooms to the filter?

@@ -139,6 +187,27 @@ def search(self, user, content, batch=None):

room_ids = search_filter.filter_rooms(room_ids)

# If doing a subset of all rooms seearch, check if any of the rooms
# are from an upgraded room, and search their contents as well
# XXX: There is the possibility that we don't have a create event for

This comment has been minimized.

@richvdh

richvdh Jan 21, 2019

Member

we ought to have the create event for any room that the user has ever been a member of, so I don't think this is relevant

historical_room_ids = []

while True:
state_ids = yield self.store.get_current_state_ids(room_id)

This comment has been minimized.

@richvdh

richvdh Jan 21, 2019

Member

I'd suggest adding a get_room_predecessor to StateGroupWorkerStore alongside get_room_version.

room_id (str): The ID of the room to search through.
Returns:
dict of past room IDs as strings

This comment has been minimized.

@richvdh

richvdh Jan 21, 2019

Member

I don't think it's a dict...

Itym Deferred[list[str]]: predecessor room ids, or better yet, Deferred[iterable[str]]: predecessor room ids

@@ -448,6 +448,7 @@ def get_current_state_ids(self, room_id):
Returns:
deferred: dict of (type, state_key) -> event_id
"""

This comment has been minimized.

@richvdh

richvdh Jan 21, 2019

Member

please try to avoid unrelated reformatting in your PRs; it makes git history harder to read.

@anoadragon453 anoadragon453 force-pushed the anoa/full_search_upgraded_rooms branch from 6ca6d12 to c2bbe85 Jan 22, 2019

Fix a bug with single-room search searching all rooms
* Create a new method for getting predecessor rooms
* Remove formatting change

@anoadragon453 anoadragon453 force-pushed the anoa/full_search_upgraded_rooms branch from c2bbe85 to c9bfb05 Jan 22, 2019

return None

# Return predecessor if present
return create_event.content.get("predecessor", None)

This comment has been minimized.

@hawkowl

hawkowl Jan 22, 2019

Contributor

@anoadragon453 you can't return in an inlinecallbacks

This comment has been minimized.

@anoadragon453

anoadragon453 Jan 22, 2019

Author Member

Ah, you're right, thank you. Still getting used to this execution model.

@anoadragon453 anoadragon453 requested a review from matrix-org/synapse-core Jan 22, 2019

@richvdh
Copy link
Member

richvdh left a comment

lgtm modulo some nits about types in docstrings.

Question though: do we have any sytests?

Show resolved Hide resolved synapse/api/filtering.py Outdated
Show resolved Hide resolved synapse/storage/state.py Outdated
Show resolved Hide resolved synapse/handlers/search.py Outdated
# Retrieve the room's create event
create_event = yield self.get_event(create_id)

if not create_event:

This comment has been minimized.

@richvdh

richvdh Jan 24, 2019

Member

this is redundant fwiw: get_event cannot return None.

richvdh and others added some commits Jan 24, 2019

Update synapse/api/filtering.py
Co-Authored-By: anoadragon453 <1342360+anoadragon453@users.noreply.github.com>
Apply suggestions from code review
Co-Authored-By: anoadragon453 <1342360+anoadragon453@users.noreply.github.com>
Merge branch 'anoa/full_search_upgraded_rooms' of github.com:matrix-o…
…rg/synapse into anoa/full_search_upgraded_rooms

@anoadragon453 anoadragon453 merged commit b1b6dba into develop Jan 25, 2019

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment