-
Notifications
You must be signed in to change notification settings - Fork 445
Analytics request loader and game version validation #6064
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
Merged
+132
−7
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
e51ed38
Analytics request loader and game version validation
aecsocket eb710ab
tweak agents
aecsocket b75e2a4
factor tags into its own util
aecsocket f506876
lock cache refresh to avoid cache stampede
aecsocket a1849b4
Make analytics fields opptional
aecsocket File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| - Use `ApiError` as the error type for API routes | ||
| - Prefer `ApiError::Internal` and `ApiError::Request` over `ApiError::InvalidInput` | ||
| - Use `eyre!` to construct a value for `Internal` and `Request` variants | ||
| - Error messages (both for errors and exceptions) must be formatted as per the Rust API guidelines: | ||
| - lowercase message | ||
| - no trailing punctuation | ||
| - wrap code items e.g. type names in backticks | ||
| - Prefer `wrap_internal_err`, `wrap_request_err` when attaching context to an existing error (like Anyhow `context` or Eyre `wrap_err`) | ||
| - All operations should ideally have some context attached | ||
| - Database operations can have a message like `.wrap_internal_err("failed to fetch XYZ")` | ||
| - You can perform real-time queries against the databases in the Docker Compose | ||
| - `docker exec labrinth-postgres psql -c "select 1"` | ||
| - `docker exec labrinth-redis redis-cli flushall` | ||
| - `docker exec labrinth-clickhouse clickhouse-client "select 1"` | ||
| - On some machines, you may have to use `podman` instead of `docker` - check which one is available first | ||
| - Hardcoded credentials for admin: | ||
| - `Authorization: Bearer mra_admin` for default admin user | ||
| - `Authorization: Bearer mra_user` for a regular user | ||
| - `Modrinth-Admin: feedbeef` as admin key | ||
| - If some steps require you to create a project/mod or version for testing, ask the user to go into the web frontend and manually create a project/version |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| use crate::database::PgPool; | ||
| use crate::database::models::legacy_loader_fields::MinecraftGameVersion; | ||
| use crate::database::models::loader_fields::Loader; | ||
| use crate::database::redis::RedisPool; | ||
| use crate::routes::ApiError; | ||
| use crate::util::error::Context; | ||
| use arc_swap::ArcSwapOption; | ||
| use std::collections::HashSet; | ||
| use std::sync::Arc; | ||
| use std::time::{Duration, Instant}; | ||
| use tokio::sync::Mutex; | ||
|
|
||
| /// Cached set of valid loaders and game version tags. | ||
| /// | ||
| /// Fetched using [`valid_download_tags`]. | ||
| #[derive(Debug)] | ||
| pub struct DownloadTagsCache { | ||
| expires: Instant, | ||
| pub loaders: HashSet<String>, | ||
| pub game_versions: HashSet<String>, | ||
| } | ||
|
|
||
| /// Fetches download tags from the database or returns a cached version. | ||
| /// | ||
| /// We cache tags since we get a large volume of download ingests, and querying | ||
| /// the database or even Redis for each request is too expensive. | ||
| pub async fn valid_download_tags( | ||
| pool: &PgPool, | ||
| redis: &RedisPool, | ||
| ) -> Result<Arc<DownloadTagsCache>, ApiError> { | ||
| const DOWNLOAD_TAGS_CACHE_TTL: Duration = Duration::from_secs(60 * 5); | ||
|
|
||
| static DOWNLOAD_TAGS_CACHE: ArcSwapOption<DownloadTagsCache> = | ||
| ArcSwapOption::const_empty(); | ||
| static DOWNLOAD_TAGS_CACHE_REFRESH_LOCK: Mutex<()> = Mutex::const_new(()); | ||
|
|
||
| let now = Instant::now(); | ||
| let cached = DOWNLOAD_TAGS_CACHE.load(); | ||
| if let Some(cached) = &*cached | ||
| && cached.expires > now | ||
| { | ||
| return Ok(cached.clone()); | ||
| } | ||
|
|
||
| let _refresh_lock = DOWNLOAD_TAGS_CACHE_REFRESH_LOCK.lock().await; | ||
|
|
||
| let now = Instant::now(); | ||
| let cached = DOWNLOAD_TAGS_CACHE.load(); | ||
| if let Some(cached) = &*cached | ||
| && cached.expires > now | ||
| { | ||
| return Ok(cached.clone()); | ||
| } | ||
|
|
||
| let loaders = Loader::list(pool, redis) | ||
| .await | ||
| .wrap_internal_err("failed to fetch loaders")? | ||
| .into_iter() | ||
| .map(|loader| loader.loader) | ||
| .collect(); | ||
| let game_versions = MinecraftGameVersion::list(None, None, pool, redis) | ||
| .await | ||
| .wrap_internal_err("failed to fetch game versions")? | ||
| .into_iter() | ||
| .map(|game_version| game_version.version) | ||
| .collect(); | ||
|
|
||
| let cache = Arc::new(DownloadTagsCache { | ||
| expires: now + DOWNLOAD_TAGS_CACHE_TTL, | ||
| loaders, | ||
| game_versions, | ||
| }); | ||
| DOWNLOAD_TAGS_CACHE.store(Some(cache.clone())); | ||
|
|
||
| Ok(cache) | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would update the cache asynchronously or at least use cache locking through a read/write lock to avoid cache stampede