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

Return attrs for more media repo APIs. #16611

Merged
merged 5 commits into from
Nov 9, 2023
Merged

Return attrs for more media repo APIs. #16611

merged 5 commits into from
Nov 9, 2023

Conversation

clokep
Copy link
Member

@clokep clokep commented Nov 8, 2023

More on my war to kill of unneeded dicts.

A few spots in the media repo that we returned dictionaries are easy to convert to attr, so do that. 🎉 I don't think there's much surprising here.

@clokep clokep marked this pull request as ready for review November 8, 2023 18:59
@clokep clokep requested a review from a team as a code owner November 8, 2023 18:59
last_access_ts: int
quarantined_by: Optional[str]
safe_from_quarantine: bool


@attr.s(slots=True, frozen=True, auto_attribs=True)
class RemoteMedia:
Copy link
Member Author

Choose a reason for hiding this comment

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

#16545 covers the nullable columns for these.

@@ -54,11 +52,32 @@ class LocalMedia:
media_length: int
upload_name: str
created_ts: int
url_cache: Optional[str]
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this and consolidated the (now) two spots we use it to return the same type, which I find easier to grok than having two separate, but really similar types.

" ORDER BY download_ts DESC LIMIT 1"
)
sql = """
SELECT response_code, expires_ts, og
Copy link
Member Author

Choose a reason for hiding this comment

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

Most of this was unused, so it was simpler to not return it.

@@ -267,23 +267,23 @@ def _assert_if_mxc_uris_purged(
def _assert_mxc_uri_purge_state(mxc_uri: MXCUri, expect_purged: bool) -> None:
"""Given an MXC URI, assert whether it has been purged or not."""
if mxc_uri.server_name == self.hs.config.server.server_name:
found_media_dict = self.get_success(
self.store.get_local_media(mxc_uri.media_id)
found_media = bool(
Copy link
Member Author

Choose a reason for hiding this comment

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

Casting to a boolean was easier than trying to tell mypy that it was a union of two different types, especially when we just care if a result was returned for tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. (Casting would be my preference, anyway.)

class UrlCache:
response_code: int
expires_ts: int
og: Union[str, bytes]
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a bit icky---does it cause any pain for the consumers of this type?

Copy link
Member Author

Choose a reason for hiding this comment

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

This feels a bit icky---does it cause any pain for the consumers of this type?

The only consumer does an "if str, decode to bytes". I'm a bit skeptical of the comment there which says something about postgres vs. sqlite.

Copy link
Contributor

Choose a reason for hiding this comment

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

matrix=> \d local_media_repository_url_cache
     Table "matrix.local_media_repository_url_cache"
    Column     │  Type   │ Collation │ Nullable │ Default 
═══════════════╪═════════╪═══════════╪══════════╪═════════
 url           │ text    │           │          │ 
 response_code │ integer │           │          │ 
 etag          │ text    │           │          │ 
 expires_ts    │ bigint  │           │          │ 
 og            │ text    │           │          │ 
 media_id      │ text    │           │          │ 
 download_ts   │ bigint  │           │          │ 
Indexes:
    "local_media_repository_url_cache_by_url_download_ts" btree (url, download_ts)
    "local_media_repository_url_cache_expires_idx" btree (expires_ts)
    "local_media_repository_url_cache_media_idx" btree (media_id

And the schema dumps say

CREATE TABLE IF NOT EXISTS "local_media_repository_url_cache"( url TEXT, response_code INTEGER, etag TEXT, expires_ts BIGINT, og TEXT, media_id TEXT, download_ts BIGINT );

for sqlite.

Looks like it should always be a string to me, too. (Maybe this is some ancient py2 str vs unicode thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it should always be a string to me, too. (Maybe this is some ancient py2 str vs unicode thing?

This is my guess, but didn't want to break it. Maybe in a follow-up?

@clokep clokep merged commit ff716b4 into develop Nov 9, 2023
38 of 40 checks passed
@clokep clokep deleted the clokep/media-attr branch November 9, 2023 16:00
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.

2 participants