synapse icon indicating copy to clipboard operation
synapse copied to clipboard

Add `via` param to hierarchy enpoint

Open kfiven opened this issue 10 months ago • 5 comments

Pull Request Checklist

Implementation of MSC4235 as per suggestion in pull request 17750.

  • [x] Pull request is based on the develop branch
  • [x] Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • [x] Code style is correct (run the linters)

kfiven avatar Jan 09 '25 09:01 kfiven

@anoadragon453 pinging you for visibility since you reviewed the previous PR to fix this.

kfiven avatar Feb 18 '25 11:02 kfiven

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Mar 23 '25 05:03 CLAassistant

Hi @sandhose,

I'm experiencing some workflow failures and I'm unsure if they're related to my recent changes. Could you take a look and help me troubleshoot the issue?

Thanks!

kfiven avatar Apr 13 '25 08:04 kfiven

I'm experiencing some workflow failures and I'm unsure if they're related to my recent changes. Could you take a look and help me troubleshoot the issue?

This is likely a test being flaky, I kicked it off again

sandhose avatar Apr 13 '25 08:04 sandhose

@sandhose heads up that the last commit caused unit tests to fail:

Click to see unit test failure logs
[FAIL]
Traceback (most recent call last):
  File "/home/runner/work/synapse/synapse/tests/handlers/test_room_summary.py", line 1130, in test_fed_remote_room_hosts
    result = self.get_success(
  File "/home/runner/work/synapse/synapse/tests/unittest.py", line 693, in get_success
    return self.successResultOf(deferred)
  File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/trial/_synctest.py", line 732, in successResultOf
    self.fail(
twisted.trial.unittest.FailTest: Success result expected on <Deferred at 0x7f9d0fd0a970 current result: None>, found failure result instead:
Traceback (most recent call last):
  File "/home/runner/work/synapse/synapse/tests/unittest.py", line 691, in get_success
    deferred: Deferred[TV] = ensureDeferred(d)  # type: ignore[arg-type]
  File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/internet/defer.py", line 1358, in ensureDeferred
    return Deferred.fromCoroutine(coro)
  File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/internet/defer.py", line 1325, in fromCoroutine
    return _cancellableInlineCallbacks(coro)
  File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/internet/defer.py", line 2197, in _cancellableInlineCallbacks
    _inlineCallbacks(None, gen, status, _copy_context())
--- <exception caught here> ---
  File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.9/lib/python3.9/site-packages/twisted/internet/defer.py", line 2014, in _inlineCallbacks
    result = context.run(gen.send, result)
  File "/home/runner/work/synapse/synapse/synapse/handlers/room_summary.py", line 166, in get_room_hierarchy
    return await self._pagination_response_cache.wrap(
  File "/home/runner/work/synapse/synapse/synapse/util/caches/response_cache.py", line 254, in wrap
    entry = self._get(key)
  File "/home/runner/work/synapse/synapse/synapse/util/caches/response_cache.py", line 146, in _get
    entry = self._result_cache.get(key)
builtins.TypeError: unhashable type: 'list'


tests.handlers.test_room_summary.SpaceSummaryTestCase.test_fed_remote_room_hosts
-------------------------------------------------------------------------------
Ran 4018 tests in 300.576s

FAILED (skips=417, failures=1, successes=3600)

anoadragon453 avatar May 06 '25 09:05 anoadragon453

@sandhose Hi, do you want me to revert the changes that broke the tests? It seems like param as a list doesn’t seem to work with caching.

kfiven avatar Jun 28 '25 10:06 kfiven

Hi @kfiven. As weekly maintainer I've taken over this PR.

I've reverted the change from tuple -> list as directed by @sandhose, added a comment as to why it must be a tuple, and fixed the merge conflicts.

This change now LGTM!

anoadragon453 avatar Jun 30 '25 12:06 anoadragon453