Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Cache empty responses from /user/devices #11587

Merged
DMRobertson merged 20 commits into from
Jan 5, 2022
Merged

Cache empty responses from /user/devices #11587

DMRobertson merged 20 commits into from
Jan 5, 2022

Conversation

@DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Dec 15, 2021

Fixes #11586. Or it will fix, anyway.

@DMRobertson
Copy link
Contributor Author

DMRobertson commented Dec 16, 2021

cc @squahtx

@DMRobertson DMRobertson marked this pull request as ready for review December 16, 2021 15:53
@DMRobertson DMRobertson requested a review from a team as a code owner December 16, 2021 15:53
@DMRobertson DMRobertson force-pushed the dmr/caching-empty-device-lists branch from 96b1932 to 51c3434 Compare December 16, 2021 17:09
clokep
clokep reviewed Dec 16, 2021
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this generally looks OK, but I'm not confident the usage of the stream IDs makes sense.

@DMRobertson
Copy link
Contributor Author

DMRobertson commented Dec 16, 2021

I think this generally looks OK, but I'm not confident the usage of the stream IDs makes sense.

FWIW I'm not interested in the specific stream ID as such. I'm using it in the query to detect that we've queried a homeserver for this user's devices before.

I agree that it's not great though. I have a bit of an icky feeling about the whole change the more I look at it. Perhaps sleeping on it will help.

@DMRobertson
Copy link
Contributor Author

DMRobertson commented Dec 16, 2021

Perhaps sleeping on it will help.

To be explicit, perhaps sleeping on it will help me to later rework this and make it clearer.

@DMRobertson
Copy link
Contributor Author

DMRobertson commented Jan 4, 2022

Perhaps sleeping on it will help.

Having hibernated over the Christmas break, I think I should

  • do away with the all-in-one query
  • use get_device_list_last_stream_id_for_remote instead to fetch the stream_id, which is cached in-application
  • only do so when we are considering updating the cache
    • so a generic read from the cache won't return None.

David Robertson added 5 commits January 4, 2022 16:19
Revert "Return None from get_cached_devices..."
ca7ede2 
This reverts commit 98d99e6.
Improve wording in comments and changelog
3c3b913
Add a couple of type hints
a9ee0a3
Correct and improve the tests
6afd197 
- Check the results are consistent and have no failures
- Better mocking of the response format
- Fix errors relating to mocking (which I fear will break on 3.7)
Cache responses which say a user has no devices
a8828d4
@DMRobertson DMRobertson marked this pull request as draft January 4, 2022 18:23
@DMRobertson
Copy link
Contributor Author

DMRobertson commented Jan 4, 2022

The commit history is a bit noisy, but I think this is a lot better now. I'll gamble that CI passes and stick it up for review.

@DMRobertson DMRobertson requested a review from clokep January 4, 2022 19:14
clokep
clokep reviewed Jan 4, 2022
View reviewed changes
@DMRobertson DMRobertson force-pushed the dmr/caching-empty-device-lists branch from 5f91be7 to 0999ed8 Compare January 5, 2022 11:53
@DMRobertson DMRobertson requested a review from clokep January 5, 2022 12:15
clokep
clokep reviewed Jan 5, 2022
View reviewed changes
Don't overwrite the stream_id from the remote HS
6fbf316 
or else we might try to write a NULL stream id. That would be bad.
@DMRobertson DMRobertson marked this pull request as ready for review January 5, 2022 13:19
clokep
clokep approved these changes Jan 5, 2022
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good.

@DMRobertson DMRobertson enabled auto-merge (squash) January 5, 2022 13:31
@DMRobertson DMRobertson merged commit 88a78c6 into develop Jan 5, 2022
@DMRobertson DMRobertson deleted the dmr/caching-empty-device-lists branch January 5, 2022 13:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@clokep clokep clokep approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

We don't cache the fact that a user has no devices

3 participants

@DMRobertson @clokep