understand-diff

What changed

apply_discount in price.py now rounds its result to 2 decimal places before returning, instead of returning the raw float.

# before
return price * (1 - percent / 100)
# after
return round(price * (1 - percent / 100), 2)

Potential issues

1. Precision loss for downstream callers — anything that relied on the exact unrounded value (e.g., further chained discounts, unit tests with exact float assertions, or aggregating many line items) will now see slightly different numbers. Tests comparing with == on floats are the most likely to break.

2. Rounding drift on aggregation — rounding per-item then summing can differ from summing then rounding (penny-level discrepancies). If totals are computed from multiple discounted prices, they may no longer match a total computed from the raw values.

3. Banker's rounding — Python's round() uses round-half-to-even, so round(2.675, 2) can give 2.67 due to float representation. If financial correctness matters, decimal.Decimal with explicit ROUND_HALF_UP is safer.

4. Type/contract change — the return is still a float, so no signature change, but callers expecting "exact math result" semantics now get a display-rounded value. Worth checking the docstring/comments reflect the new behavior.

Quick checks

• Run the existing tests for price.py and anything importing it.
• Grep for callers that sum or chain apply_discount results.
• If this is for money display, consider rounding only at the presentation layer instead.

Overall it's a low-risk, sensible change for currency display — the main risk is exact-value assertions in tests and penny-level aggregation drift.

What changed: In price.py, apply_discount now rounds its result to 2 decimal places before returning:

return round(price * (1 - percent / 100), 2)

Purpose: Likely to avoid floating-point artifacts (e.g., 19.999999999) in displayed/charged prices.

Could it break anything?

1. Callers expecting exact math — any code that compares results with == against unrounded values (e.g., apply_discount(100, 10) == 90.0 still passes, but apply_discount(33.33, 15) == 28.3305 would now fail). Tests asserting full-precision values will break.
2. Accumulation drift — if discounts are applied repeatedly or summed across many items, rounding each step compounds error vs. rounding only at the end. Financial code usually rounds only at final settlement.
3. Banker's rounding — Python's round() uses round-half-to-even: round(2.675, 2) → 2.67, not 2.68. If you expect "round half up" (common in pricing), results may differ by a cent. For money, decimal.Decimal with ROUND_HALF_UP is safer.
4. Float precision caveat — round() on a float can still be off by a cent due to binary representation; Decimal avoids this entirely.

Risk: Low blast radius (one line, pure function), but review all callers/tests of apply_discount for exact-value assertions and multi-step discount chains. If this feeds billing or invoicing, consider switching to Decimal.

This PR filters out inactive users from two database queries:

Changes:

• api.py — get_user(user_id) now only returns users where active = 1, so fetching a single user by ID will fail/return nothing for deactivated accounts.
• admin.py — list_users() now only lists active users, hiding deactivated accounts from the admin listing.

Affected components:

• The user lookup API endpoint (single-user fetch)
• The admin user listing view
• Anything downstream that relies on these queries (e.g., clients fetching inactive users' data, or admin tooling that needs to view/reactivate deactivated accounts — that functionality would effectively be lost unless handled elsewhere).

No knowledge graph is available in this environment, so here's a direct analysis of the diff:

What changed

Both changes add an active = 1 filter to user queries — i.e., inactive/deactivated users are now excluded from results.

1. api.py — get_user(user_id): The single-user lookup now requires active = 1. Previously it returned any user by ID; now it returns nothing (or an error/404, depending on handling) for inactive users.
2. admin.py — list_users(): The user listing now only returns active users, hiding deactivated accounts from the list.

Affected components

• get_user API endpoint — any client fetching a deactivated user's profile will now get an empty/missing result instead of data. Authentication, session resolution, or anything else that calls get_user by ID could be impacted if it needs to look up inactive users (e.g., to show "account deactivated" messages).
• list_users admin view — admins can no longer see or manage inactive accounts through this listing (no way to reactivate users if this is the only listing path).
• Downstream consumers of these functions (pagination counts, user caches, exports) may see changed result sets.

Risks to review

• Behavioral change, not just a filter: get_user returning nothing for inactive users may break flows that expect a user record (e.g., login, token validation). Consider whether callers need to distinguish "not found" from "inactive."
• Admin functionality loss: If admins need to view/reactivate deactivated users, this removes that capability — consider an ?include_inactive flag or a separate admin query.
• No migration or schema change involved; risk is moderate and concentrated in API contract changes.

Summary

The diff moves the cache lookup outside the lock in get_or_set, so reads no longer acquire the lock.

What's affected

• Read path performance: cache hits are now lock-free — likely the intent of the change (reduced contention under high read load).
• Write path: compute() still runs inside the lock, so expensive computations remain serialized.

Risks

1. Race / duplicate compute (thundering herd): Two threads can both miss the cache, then serialize on the lock and each call compute(). Previously the double-checked locking pattern guaranteed compute() ran at most once per key. Now it can run multiple times concurrently — a correctness/perf regression if compute() is expensive or non-idempotent (e.g., API calls, DB writes, counters).

2. Torn/stale reads without synchronization: In CPython, dict get/set are effectively atomic under the GIL, so this is mostly safe there. But on GIL-free Python (3.13+ free-threading), PyPy, or if cache is swapped/rebuilt at runtime, an unsynchronized read could observe a partially-populated state.

3. Lost semantic guarantees: If callers relied on get_or_set as a "compute exactly once" primitive (memoization of side-effectful work), that contract is silently broken.

Recommendations

• Keep double-checked locking: check inside the lock again before calling compute():

  if key in cache:
      return cache[key]
  with lock:
      if key in cache:          # re-check
          return cache[key]
      value = compute()
      cache[key] = value
  return value
  

  This preserves lock-free fast-path reads while preventing duplicate computes.
• Alternatively use functools.lru_cache / a per-key lock if duplicate computes must be fully prevented.
• Add a test covering concurrent get_or_set on the same cold key (assert compute call count == 1).

No knowledge graph is available in this environment, so here's a direct analysis of the diff.

What changed

cache.py — get_or_set(key, compute) was modified:

Before: the entire check-then-compute sequence ran inside the lock (double-checked locking, fully serialized).

After: the cache lookup (if key in cache) was moved outside the lock; only compute() and the write happen under the lock.

Affected components

• Everything that calls get_or_set — any code relying on it for deduplicated, expensive computation (DB queries, API calls, heavy transforms).
• Any consumers relying on its atomicity/consistency guarantees.

Risks

1. Race condition / duplicate compute (thundering herd): Two threads can both miss the cache, both acquire the lock sequentially, and both run compute(). The second result overwrites the first. This defeats the function's core purpose under contention.
2. Duplicate side effects: If compute() is non-idempotent (writes to DB, sends emails, charges money), it can now execute multiple times concurrently — potentially a correctness/financial bug, not just a perf one.
3. Stale overwrite: A slower thread's older computed value can overwrite a newer one written after it started.
4. Unsynchronized read: Reading cache outside the lock is only safe if cache is a dict and the GIL makes key in cache atomic; it's fragile and non-portable (e.g., alternative interpreters, or if cache becomes a non-thread-safe structure).
5. Lock no longer protects the invariant that a key's value corresponds to exactly one compute call.

Likely intent & better fix

The change was probably meant to avoid holding the lock during expensive compute() (a real problem — long lock holds serialize all callers). But the correct pattern is double-checked locking:

if key in cache:
    return cache[key]
with lock:
    if key in cache:          # re-check inside lock
        return cache[key]
    value = compute()
    cache[key] = value
return value

This still holds the lock during compute (or use a per-key lock / future-based memoization to avoid that), but guarantees single computation.

Risk level: Medium–High — safe only if compute() is pure, idempotent, cheap, and duplicate execution is acceptable; risky otherwise. Review all get_or_set call sites for side effects before merging.

Summary of Changes

auth.py — create_token now embeds an admin claim in the JWT, sourced from user.get("admin", False).

config.py — Replaces a hardcoded JWT secret with os.environ["JWT_SECRET"].

database.py — get_user now selects only id, role, admin instead of SELECT *.

Affected Components

• Token issuance (auth)
• JWT verification anywhere the payload is decoded (any code reading claims)
• User lookup queries and anything depending on the full user row
• Deployment/config (new required env var)

Risks

1. Hard startup failure: os.environ["JWT_SECRET"] raises KeyError if unset. Prefer os.environ.get("JWT_SECRET") with a fail-fast check and clear error message, and update deployment configs/secrets management.

2. Secret rotation invalidates all tokens: Switching from the old hardcoded secret means every existing JWT becomes invalid — all users are logged out at deploy time. Plan a rollout window or support dual-secret verification temporarily.

3. Privilege escalation via admin claim: The admin claim is now part of the token. Verify that downstream authorization checks read the server-side role, not just the token claim — and that the users.admin column can't be self-modified (e.g., via mass assignment or an unvalidated update endpoint). If any endpoint lets users write to admin, this change is a serious vulnerability.

4. Silent default to False: user.get("admin", False) masks schema drift. If the column is missing or named differently, admins silently lose privileges. Since database.py explicitly selects admin, a missing column will raise a DB error instead — good, but ensure the migration adding/confirming that column ships with this PR.

5. Query contract change: Callers of get_user that relied on other fields from SELECT * will now break (likely with KeyError/AttributeError). Audit all call sites. On the plus side, narrowing the query reduces accidental data leakage into tokens.

6. Algorithm unchanged (HS256): Fine, but consider confirming key length/entropy requirements for the new secret, and whether RS256/asymmetric signing is warranted if tokens are verified by multiple services.

Recommendations

• Add a migration for the admin column if it doesn't exist.
• Add tests: token contains admin claim; missing env var fails loudly; existing tokens rejected gracefully.
• Grep for all JWT decode sites to confirm how admin is consumed.
• Document the new JWT_SECRET requirement in README/deployment docs.

PR Review: JWT admin claim + secret externalization

Note: no project knowledge graph (.ua/knowledge-graph.json) was available to trace callers automatically, so this impact analysis is derived from the diff itself.

What changed

File	Change
auth.py	create_token now embeds an admin claim (user.get("admin", False)) in the JWT payload
config.py	SECRET replaced hardcoded value with os.environ["JWT_SECRET"]
database.py	get_user narrowed from SELECT * to explicit id, role, admin columns

Affected components

• Token verification / auth middleware — anything that decodes tokens must now handle (or deliberately ignore) the new admin claim. If authorization logic reads claims, it needs updating in the same PR.
• All callers of get_user — this is the biggest blast radius. Any code that used fields from SELECT * (e.g., `email