Include method in thumbnail media name

[deepbluev7]

This fixes an issue, where different crop methods overwrite one another.

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

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)

[deepbluev7]

I'm not sure, if this needs some kind of migration or similar, this is just the fix that worked for me, but I deleted my remote thumbnails manually... Most servers are probably not affected, since this only happens, when you have dynamic thumbnailing enabled or you have the same size added in your config to be cropped and scaled, which is not the default.

Interestingly I only noticed this issue, because synapse calculates the wrong response size, which in turn lead to my client failing the request, as synapse closed the connection before my client finished reading all the data, that was announced to be there in the response header. Curl also complains about a prematurely terminated connection.

[deepbluev7]

I think I also need to update the schema to include the method in the unique constraint, but I'm not really sure, how one would do that. It probably needs to look like mentioned in this comment: #2182 (comment)

Constraint is for example here: https://github.com/matrix-org/synapse/blob/master/synapse/storage/data_stores/main/schema/full_schemas/54/full.sql.postgres#L1393

This may also solve #2182 then. I think I already dropped that constraint manually at some point, which is why I experienced this issue in the first place probably.

So some help on how one would approach that would be nice. Do I just add a delta file to the schema directory that drops the old constraint and creates a new one?

[clokep]

I think I also need to update the schema to include the method in the unique constraint, but I'm not really sure, how one would do that. It probably needs to look like mentioned in this comment: #2182 (comment)

Migrations are handled via deltas (some examples: https://github.com/matrix-org/synapse/tree/master/synapse/storage/data_stores/main/schema/delta/57). Unfortunately I don't seem to find any good documentation about how those are handled.

So some help on how one would approach that would be nice. Do I just add a delta file to the schema directory that drops the old constraint and creates a new one?

I believe this is true, I think you'll need to create a new delta directory and add the appropriate sql in there. Also note whether a different query will need to be done for sqlite vs. postgresql.

[deepbluev7]

Thanks, @clokep , I'll take a look this weekend :D

[richvdh]

You don't need to create a new directory unless the change you are making will be incompatible with older versions of synapse. Just stick a delta file in the latest directory (currently https://github.com/matrix-org/synapse/tree/develop/synapse/storage/data_stores/main/schema/delta/58). Number it so that it runs in the expected order.

There are a few words about how it works at https://github.com/matrix-org/synapse/blob/develop/synapse/storage/prepare_database.py#L102, though tbh that looks like it could do with a bit of an update to reflect the fact that there are now multiple "data stores".

[deepbluev7]

I hope the db migrations are correct, but SQL really isn't my forte :D

[richvdh]

there's a problem here, which is that these tables may be large (tens of millions of rows), and adding a constraint like this may take a few minutes. As a general rule we try to avoid things that will cause synapse to stop responding for a long time during a restart.

The ideal solution, at least on postgres, is to use a background update to create an index concurrently via register_background_index_update, and then drop the constraint once that completes.

[deepbluev7]

@richvdh Thank you for your review. I'll see if I can make the migration run in the background. About the orphaning I have no idea and would like some input on how to handle that. (Also sorry about the potential reply spam, had some issues with Github 500ing me...)

[richvdh]

@deepbluev7 thanks very much for picking this up and working on it. As you can tell from the age of #2182, it's a very long-standing issue: I think lots of people will appreciate a fix!

[deepbluev7]

Well, it's scratch your own itch... I changed my client to display cropped images instead of scaled images and suddenly I had missing avatars, so I dug into it and stumbled on this :D

[clokep]

@deepbluev7 The current packaging issue is unrelated to this PR (merging develop in should fix the issue). Let us know when this is ready for another review!

[deepbluev7]

@clokep Well, I'm currently stuck on how to prevent regenerating all remote thumbnails as mentioned in #7124 (comment). If I figure out a solution on my own, I will let you know but I'd also appreciate some help on that topic. :D

Anyway, thanks for your ping. Always appreciated to know, that people care about what you are doing. :3

[richvdh]

@deepbluev7 sorry for taking a while to get back to you on this. I've tried to respond to your question: do you think you can proceed now?

[deepbluev7]

Yeah, I think I can. Although it may take a few days to figure that out, I'll try if I can :3

(I need to figure out, where the file gets accessed, since I only changed the function, that returns the filename. I need to figure out, where that filename is used and implement a fallback there.)

[clokep]

(I need to figure out, where the file gets accessed, since I only changed the function, that returns the filename. I need to figure out, where that filename is used and implement a fallback there.)

According to GitHub there's only a single caller:

synapse/synapse/rest/media/v1/media_storage.py

Lines 214 to 221 in 737b4a9

	return self.filepaths.remote_media_thumbnail_rel(
	server_name=file_info.server_name,
	file_id=file_info.file_id,
	width=file_info.thumbnail_width,
	height=file_info.thumbnail_height,
	content_type=file_info.thumbnail_type,
	method=file_info.thumbnail_method,
	)

[deepbluev7]

The issue is that that usage location is not actually, where the file is read. That just converts the path to a relative path. I need to modify the location where that path is read from though, so that if the read fails, it falls back to a different path. So I actually need to modify 2 locations:

synapse/synapse/rest/media/v1/media_storage.py

Line 143 in 737b4a9

path = self._file_info_to_path(file_info)

and

synapse/synapse/rest/media/v1/media_storage.py

Line 167 in 737b4a9

path = self._file_info_to_path(file_info)

from what I can tell. :3

[clokep]

@deepbluev7 Looks like this has some conflicts so we'll need to do something. We prefer if you would merge develop into your branch (instead of a rebase) since it makes it clearer what has changed.

I believe the conflicts are that we renamed self.db to self.db_pool. Hopefully those are straightforward to fix! 👍

[deepbluev7]

Thanks, i will do that. So I don't need to move the schemas?

[clokep]

You should not need to move the schemas! Sorry missed that question. 😄

[deepbluev7]

Thank you, I guess we'll see how much that broke

[clokep]

It doesn't seem that the python 3.5 failures are from this branch (I saw them another PR as well). I believe they're due to pypa/setuptools#2352.

[deepbluev7]

Yeah, I somehow doubt my changes could have broken everything on python 3.5 :D

[richvdh]

lgtm!

[richvdh]

thanks again!

[deepbluev7]

Huh, that went pretty quick now. Thank you for all the help!