fix: consolidate price population scripts, remove broken Steam WebAPI#14
Merged
fix: consolidate price population scripts, remove broken Steam WebAPI#14
Conversation
- Removed broken Steam WebAPI fallback (GetAppList doesn't exist) - Removed steam.webapi dependency entirely - Based on async version with proper ITAD API integration - Uses /games/lookup/v1 and /games/prices/v3 endpoints correctly - Adaptive rate limiting with configurable concurrency - Batch database writes for safety
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request consolidates price-fetching by converting Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main (Orchestrator)
participant Sem as Semaphore (Concurrency Control)
participant API as Steam/ITAD APIs
participant DB as Database (Batch Writer)
rect rgba(100,150,200,0.5)
Note over Main,API: Phase 1 — Concurrent collection
Main->>Main: load game IDs
Main->>Sem: init (max_concurrent)
loop per game (asyncio.as_completed)
Main->>Sem: acquire
Main->>API: fetch_steam_price_single(app_id) / fetch_itad_price_single(app_id)
API-->>Main: (app_id, success, payload, error)
Main->>Sem: release
end
end
rect rgba(150,200,100,0.5)
Note over Main,DB: Phase 2 — Batched writes
Main->>DB: batch_write_steam_data(steam_data, batch_size)
alt Transaction succeeds
DB->>DB: commit batch
DB-->>Main: rows_written
else Batch fails
DB->>DB: rollback
loop per-record salvage
DB->>DB: write single record
end
DB-->>Main: partial_rows_written
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/populate_prices.py`:
- Around line 71-90: The rate limiter dictionary keys (current_delays,
min_delays, max_delays, error_counts, success_counts, last_adjustment,
rate_limit_locks, last_request_times) use "store_api"/"itad_api" but the rest of
the code and request calls pass "store"/"itad", causing KeyError; fix by
normalizing the keys to match the request paths (use "store" and "itad") or
vice‑versa — update the initializer dictionaries and any references inside
rate_limited_request and adaptive_rate_limit to use the same keys
("store"/"itad"), and also update related uses in the fetcher/error handling
code noted (around the mentioned ranges) so all accesses reference the unified
symbols.
- Around line 192-209: The code currently treats any non-429 response as
success; change the logic in the response handling block so only 2xx responses
are considered successful: check response.status_code in the 200-299 range, call
self.adaptive_rate_limit(api_type, True) and return the response only for 2xx;
for non-2xx (and non-429) responses increment retry handling the same as 429
(use backoff, logger warning/info, await asyncio.sleep, call
self.adaptive_rate_limit(api_type, False), and continue/retry up to
max_retries), and finally log and return None after max_retries; update the same
logic in both places called out (around the shown block and the 225-244 region)
and ensure handle_api_response receives None only when retries are exhausted.
- Around line 404-460: batch_write_steam_data is opening raw connections with
get_db_connection() and manually managing BEGIN/COMMIT/ROLLBACK, which bypasses
the process-level write-lock; replace those raw connections with the
write-locking context returned by get_write_connection() (use "with
get_write_connection() as conn:"), remove manual BEGIN/COMMIT/ROLLBACK calls
(let the context manage the lock/transaction) and ensure cache_game_details(...)
is called using that conn; apply the same change to the sibling function in the
462-538 range so both use get_write_connection() and proper context-scoped error
handling (on exceptions let the context handle rollback, and if you must retry
individual records, open a new get_write_connection() context for each retry and
avoid reusing the original raw conn).
- Around line 204-205: The code is building ITAD request URLs with inline query
strings (e.g., title={game_name} and the ITAD_API_KEY) and logging the full URL
(logger.warning("Max retries exceeded for %s", url)), which can leak secrets;
refactor the HTTP requests to pass query parameters as a separate params dict
(or use urllib.parse.urlencode) rather than concatenating them into the base
URL, ensure calls that build the URL variable do not include ITAD_API_KEY
inline, and change log calls (e.g., logger.warning and any retry logging around
self.adaptive_rate_limit) to log only the sanitized base URL and non-sensitive,
properly encoded parameter keys/values (or mask/hide API key) — apply the same
fix to the other occurrences in the file mentioned (lines ~291-312 and
~348-367).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f95d0b16-6551-4b9f-980e-e65ff073db1b
📒 Files selected for processing (3)
scripts/populate_prices.pyscripts/populate_prices_async.pyscripts/populate_prices_optimized.py
💤 Files with no reviewable changes (2)
- scripts/populate_prices_optimized.py
- scripts/populate_prices_async.py
- Update database connection import to use get_write_connection for clarity - Simplify adaptive rate limiting dictionary keys by removing _api suffix - Add params parameter to make_request_with_retry for query string support - Extend retry logic to handle any non-2xx HTTP responses, not just 429 - Improve logging to include HTTP status codes and cleaned URLs - Refactor Steam API request construction to use params dictionary
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/populate_prices.py (1)
734-749:⚠️ Potential issue | 🟡 MinorReject contradictory CLI mode combinations.
--steam-only --itad-onlycurrently exits successfully after doing nothing, and--refresh-current --itad-onlyprints a refresh banner even though the Steam path is skipped. These combinations should be rejected up front.🛠️ Minimal fix
- parser.add_argument( - "--steam-only", action="store_true", help="Only populate Steam Store prices" - ) - parser.add_argument( - "--itad-only", action="store_true", help="Only populate ITAD historical prices" - ) + target_group = parser.add_mutually_exclusive_group() + target_group.add_argument( + "--steam-only", action="store_true", help="Only populate Steam Store prices" + ) + target_group.add_argument( + "--itad-only", action="store_true", help="Only populate ITAD historical prices" + ) @@ args = parser.parse_args() + + if args.refresh_current and args.itad_only: + parser.error("--refresh-current cannot be combined with --itad-only")Also applies to: 770-776, 808-820
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/populate_prices.py` around lines 734 - 749, After parsing CLI args (the parser variable and resulting args), validate and reject incompatible flag combinations up front: if args.steam_only and args.itad_only call parser.error(...) (or sys.exit non‑zero) with a clear message; similarly if args.itad_only and (args.refresh_current or args.force_refresh) call parser.error(...) because refresh flags apply to Steam only. Also move or guard the Steam "refresh" banner/logic so it only runs when the Steam path will execute (e.g., check not args.itad_only before printing the refresh banner or performing Steam refresh in the code paths that reference args.refresh_current/args.force_refresh).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/populate_prices.py`:
- Around line 195-215: The current retry branch treats all 4xx responses like
transient errors; change the logic so only transient statuses are retried: keep
429 and server-side errors (HTTP 5xx, e.g., 500–599; optionally 408) in the
retry path, but treat permanent client errors (400–499 excluding 429) as
immediate failures. Update the condition around response.status_code in the
request loop (the block that references response.status_code, url, attempt,
max_retries and calls self.adaptive_rate_limit) to: if status is 429 or (500 <=
status < 600) [and/or status == 408 if desired] then perform backoff/retry,
otherwise log/return None immediately for 4xx client errors so bad keys or
malformed requests fail fast.
- Around line 65-107: The constructor uses max_concurrent unchecked which allows
0 or negative values to create an unusable asyncio.Semaphore; validate
max_concurrent in __init__ (e.g., raise ValueError if max_concurrent < 1) before
creating self.semaphore, referencing the __init__ method, self.semaphore and
max_concurrent; additionally validate the CLI input in main()/argparse (after
parse_args(), reference args.concurrent and the parser) and call
parser.error("--concurrent must be >= 1") (or equivalent) to fail fast with a
clear message.
---
Outside diff comments:
In `@scripts/populate_prices.py`:
- Around line 734-749: After parsing CLI args (the parser variable and resulting
args), validate and reject incompatible flag combinations up front: if
args.steam_only and args.itad_only call parser.error(...) (or sys.exit non‑zero)
with a clear message; similarly if args.itad_only and (args.refresh_current or
args.force_refresh) call parser.error(...) because refresh flags apply to Steam
only. Also move or guard the Steam "refresh" banner/logic so it only runs when
the Steam path will execute (e.g., check not args.itad_only before printing the
refresh banner or performing Steam refresh in the code paths that reference
args.refresh_current/args.force_refresh).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1fcca3e0-e54a-4f22-8baf-20b4511dd31e
📒 Files selected for processing (2)
justfilescripts/populate_prices.py
Add validation for max_concurrent parameter to ensure it's at least 1. Refine retry logic to only retry on transient errors (HTTP 429, 5xx, 408) and fail fast on client errors (4xx excluding 429/408). Add command-line argument validation to prevent invalid flag combinations and enforce constraints.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/populate_prices.py`:
- Around line 795-798: The check that rejects the combination args.itad_only &&
args.force_refresh should be removed so --force-refresh can be used with
--itad-only; update the CLI parsing logic to allow that combo and ensure
downstream code passes args.force_refresh into populate_itad_prices (referenced
function populate_itad_prices(..., force_refresh=args.force_refresh)) so ITAD
cache refresh is performed, and remove or adjust the parser.error call that
currently blocks this case (the conditional containing args.itad_only and
args.force_refresh).
- Around line 129-159: The adaptive counters (self.success_counts and
self.error_counts) are lifetime totals so transient spikes get diluted; after
you compute error_rate and adjust delays in the adjustment block inside the
function (where current_delays, max_delays, min_delays, last_adjustment and
error_rate are used), reset the per-api counters (e.g.,
self.success_counts[api_type] and self.error_counts[api_type]) to zero (or
implement a sliding-window buffer) immediately after applying an adjustment and
updating self.last_adjustment[api_type] so future adjustments are based on
recent activity.
- Around line 311-439: The fetch_itad_price_single function currently masks real
request/errors as "not_found"; change it to return an explicit error status when
ITAD requests fail: inside fetch_itad_price_single, after any
make_request_with_retry call that returns None treat that as a failure and
return (app_id, "error", {}, "itad_request_failed") instead of continuing
silently, and in the two except blocks that catch exceptions (the ones with
logger.debug "ITAD appid lookup failed..." and "ITAD name search failed...")
return (app_id, "error", {}, "itad_exception") immediately rather than
swallowing the exception; this uses the existing function name
fetch_itad_price_single and make_request_with_retry to locate the changes and
ensures the final return only covers true "not_found" cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f4e49c9-6e45-464d-ada4-1ec449133d4c
📒 Files selected for processing (1)
scripts/populate_prices.py
Comment on lines
+795
to
+798
| if args.itad_only and args.force_refresh: | ||
| parser.error( | ||
| "--force-refresh applies to Steam only, incompatible with --itad-only" | ||
| ) |
There was a problem hiding this comment.
Don’t reject --itad-only --force-refresh.
populate_itad_prices(..., force_refresh=args.force_refresh) already supports refreshing cached ITAD data, and the CLI help says --force-refresh applies to all price data. This guard blocks the only ITAD-only refresh path.
🛠️ Minimal fix
- if args.itad_only and args.force_refresh:
- parser.error(
- "--force-refresh applies to Steam only, incompatible with --itad-only"
- )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if args.itad_only and args.force_refresh: | |
| parser.error( | |
| "--force-refresh applies to Steam only, incompatible with --itad-only" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/populate_prices.py` around lines 795 - 798, The check that rejects
the combination args.itad_only && args.force_refresh should be removed so
--force-refresh can be used with --itad-only; update the CLI parsing logic to
allow that combo and ensure downstream code passes args.force_refresh into
populate_itad_prices (referenced function populate_itad_prices(...,
force_refresh=args.force_refresh)) so ITAD cache refresh is performed, and
remove or adjust the parser.error call that currently blocks this case (the
conditional containing args.itad_only and args.force_refresh).
Reset success/error counters when adjusting API rate limits to ensure accurate tracking. Add None checks for ITAD API responses to prevent errors and return explicit error statuses when requests fail or exceptions occur. Remove obsolete --force-refresh validation that conflicted with --itad-only usage.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What
Consolidated three duplicate price population scripts into one working script.
Why
The original populate_prices.py was crashing with:
AttributeError: 'WebAPIInterface' object has no attribute 'GetAppList'Turns out all three scripts (populate_prices.py, populate_prices_optimized.py, populate_prices_async.py) had the same broken code. The GetAppList method doesn't exist in the Steam WebAPI library - we verified against the actual docs.
What changed
What stays the same
Testing
Run with --dry-run first to verify it works:
or
Summary by CodeRabbit
New Features
Improvements
Removed