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

Leave out optional keys from /sync #9919

Merged
merged 10 commits into from
May 5, 2021
Merged

Conversation

deepbluev7
Copy link
Contributor

@deepbluev7 deepbluev7 commented May 1, 2021

This leaves out all optional keys from /sync. This should be fine for all clients tested against conduit already, but it may break some clients, as such we should check, that at least most of them don't break horribly and maybe back out some of the individual changes. (We can probably always leave out groups for example, while the others may cause more issues.)

Clients I have tested:

  • Nheko
  • Element (slightly outdated version)
  • FluffyChat (old version)

This also needs an updated sytest, see matrix-org/sytest#1040

In my testing this decreases the sync json response to about half, which saves about 30% of data on every /sync. It also makes debugging a lot easier, since the response json is easier to read. :3

Tell me if the news entry is descriptive enough, since some people may want to be aware of this change, if they are client developers or so. Also, my python skills suck, I'm sure there is a tidier way to do this. Also, no idea how to run against the newer sytest version? Is that automatic, after it is merged?

Pull Request Checklist

  • Pull request is based on the develop branch
  • 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.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

Signed-off-by: Nicolas Werner nicolas.werner@hotmail.de

@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented May 2, 2021

Please note this addresses #6579, see if it also fixes it.


Also, thank you for taking this on, I made #8336 in the past, but i more-or-less left that one to bitrot because I couldn't figure out how to fix the sytests, thanks :D

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@clokep
Copy link
Member

clokep commented May 3, 2021

I'm going to set the review flag on this even though it has failing tests, since I believe they should be fixed by matrix-org/sytest#1040.

@clokep clokep requested a review from a team May 3, 2021 11:29
@deepbluev7
Copy link
Contributor Author

Seems like some integration tests are failing, that I didn't see, when I ran them locally (probably overlooked them). I'll try to fix that soonish.

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@deepbluev7
Copy link
Contributor Author

Should be fine now, although the tests would now fail, if you don't exclude the keys from /sync.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

looks good! a couple of suggestions here.

changelog.d/9919.feature Outdated Show resolved Hide resolved
tests/rest/client/v2_alpha/test_sync.py Outdated Show resolved Hide resolved
deepbluev7 and others added 2 commits May 5, 2021 13:08
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm

@richvdh richvdh enabled auto-merge (squash) May 5, 2021 13:15
@richvdh
Copy link
Member

richvdh commented May 5, 2021

thanks very much!

@deepbluev7
Copy link
Contributor Author

Thank you for the review! <3

@richvdh
Copy link
Member

richvdh commented May 6, 2021

heads-up that this is being reverted in #9940

erikjohnston added a commit that referenced this pull request May 6, 2021
anoadragon453 pushed a commit that referenced this pull request Jun 18, 2021
This leaves out all optional keys from /sync. This should be fine for all clients tested against conduit already, but it may break some clients, as such we should check, that at least most of them don't break horribly and maybe back out some of the individual changes. (We can probably always leave out groups for example, while the others may cause more issues.)

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
anoadragon453 added a commit that referenced this pull request Jun 23, 2021
@bmarty
Copy link

bmarty commented Jul 26, 2021

Hi,
You said that you've tested Element

Element (slightly outdated version)

It would be nice to be more precise. Element Web, Element Android and Element iOS are three different implementations, so may have different bugs :/

@deepbluev7
Copy link
Contributor Author

@bmarty My bad, I missed to put Web. I only tested Element Web, since I have no Android or iOS device. That wasn't intentional, I just missed it! Sorry!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants