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

Search relies on optional count field in response #27517

Closed
olivia-fl opened this issue Jun 4, 2024 · 3 comments · Fixed by matrix-org/matrix-react-sdk#12575
Closed

Search relies on optional count field in response #27517

olivia-fl opened this issue Jun 4, 2024 · 3 comments · Fixed by matrix-org/matrix-react-sdk#12575
Assignees
Labels
A-Search A-Timeline-Search S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect Z-Spec-Compliance An area where Element doesn't correctly implement the spec

Comments

@olivia-fl
Copy link

Steps to reproduce

  1. Spin up a server that does not return a value for the optional count field in the response body for the /client/v3/search endpoint.
  2. Use the search function in a non-E2EE room

For a server implementation reproducing the element bug, you can use commit 5c39c7c5 of grapevine, conduit df16b2ba (before a workaround for element was added in edfd3c1f), or conduwuit a2ee6b41 with the element-hacks feature disabled (before the workaround was made unconditional in 0214caea).

Outcome

What did you expect?

Either some search results appear or it shows that there were no results. The count field is marked as optional in the spec, so it's absence should not break search client-side.

What happened instead?

Hangs indefinitely, showing the loading animation:

image

Returning any value in the count field causes search to function as expected.

Operating system

NixOS (linux)

Browser information

Firefox 126.0

URL for webapp

app.element.io

Application version

Element version: 1.11.67 Crypto version: Rust SDK 0.7.0 (61b175b), Vodozemac 0.5.1

Homeserver

Grapevine 5c39c7c5fff0e1a59999f0c13bf4aa48cb159129

Will you send logs?

Yes

@dosubot dosubot bot added A-New-Search-Experience The new search dialog available in Labs S-Major Severely degrades major functionality or product features, with no satisfactory workaround labels Jun 4, 2024
@t3chguy
Copy link
Member

t3chguy commented Jun 4, 2024

Will you send logs?
Yes

Not seeing any logs from you

@t3chguy t3chguy added X-Needs-Info This issue is blocked awaiting information from the reporter A-Timeline-Search Z-Spec-Compliance An area where Element doesn't correctly implement the spec A-Search and removed A-New-Search-Experience The new search dialog available in Labs X-Needs-Info This issue is blocked awaiting information from the reporter labels Jun 4, 2024
@t3chguy t3chguy self-assigned this Jun 4, 2024
@olivia-fl
Copy link
Author

Will you send logs?
Yes

Not seeing any logs from you

Ah, sorry, I forgot to do that yesterday. I've uploaded logs now.

Thanks for putting together a fix so quickly! I'll be able to test it later today :)

@richvdh
Copy link
Member

richvdh commented Jun 5, 2024

For links, the result of the /search endpoint is specified at https://spec.matrix.org/v1.10/client-server-api/#_matrixclientv3search_results, and the count property is a member of Result Room Events.

count is indeed optional. It seems unfortunate (and probably not really intended?) that all of the properties in that structure, including results, are optional, but that's what the spec says.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Search A-Timeline-Search S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect Z-Spec-Compliance An area where Element doesn't correctly implement the spec
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants