Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR plausible#5748

Type: Clean (correct implementation)

Original PR Title: ConsolidatedView.Cache - first pass
Original PR Description: ### Changes

This lets us have an interface to lookup regular site ids per consolidated view id efficiently. I don't think it needs more tests coverage at this point - since all the caches and their common characteristics are thoroughly tested elsewhere.

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

Original PR URL: plausible#5748


PR Type

Enhancement


Description

  • Add caching layer for consolidated views with efficient site ID lookups

  • Refactor site_ids/1 to accept both Team and consolidated view ID

  • Implement cache refresh strategies for all and recently updated views

  • Integrate ConsolidatedView.Cache into application supervision tree


Diagram Walkthrough

flowchart LR
  A["ConsolidatedView Module"] -->|refactored| B["site_ids/1 function"]
  B -->|accepts| C["Team or View ID"]
  D["New Cache Module"] -->|implements| E["Plausible.Cache"]
  E -->|provides| F["refresh_all & refresh_updated_recently"]
  D -->|integrated into| G["Application Supervision"]
  H["Tests"] -->|validate| D
Loading

File Walkthrough

Relevant files
Enhancement
consolidated_view.ex
Refactor site_ids to support view ID lookup                           

extra/lib/plausible/consolidated_view.ex

  • Modified site_ids/1 to accept both Team.t() and String.t()
    (consolidated view ID)
  • Changed return type to always return {:ok, [pos_integer()]} | {:error,
    :not_found}
  • Updated get/1 query to include inner_join and preload team association
  • Renamed variable cv to consolidated_view for clarity
+13/-7   
cache.ex
New cache layer for consolidated views                                     

extra/lib/plausible/consolidated_view/cache.ex

  • New caching module implementing Plausible.Cache behavior
  • Provides base_db_query/0 aggregating site IDs per consolidated view
  • Implements refresh_updated_recently/1 for incremental cache updates
  • Includes get_from_source/1 and unwrap_cache_keys/1 callbacks
+82/-0   
cache.ex
Make refresh_updated_recently overridable                               

lib/plausible/cache.ex

  • Split refresh_updated_recently/1 function definition from
    implementation
  • Added defoverridable refresh_updated_recently: 1 to allow module
    overrides
  • Enables submodules to customize refresh logic while maintaining base
    behavior
+5/-1     
Configuration changes
application.ex
Integrate ConsolidatedView.Cache into supervision               

lib/plausible/application.ex

  • Added ConsolidatedView.Cache to supervision tree with EE guard
  • Configured cache with read concurrency and lock partitions
  • Set up two warmers: refresh_all (20 min interval) and
    refresh_updated_recently (1 min interval)
+16/-0   
Tests
cache_test.exs
Add comprehensive cache tests                                                       

test/plausible/consolidated_view/cache_test.exs

  • New test module with four test cases covering cache functionality
  • Tests refresh_all stores correct site IDs per consolidated view
  • Tests incremental refresh adds sites to existing consolidation
  • Tests small refresh re-consolidates when view is enabled
  • Tests get_from_source/1 returns same results as cache
+103/-0 
data_case.ex
Enable EE macros in test support                                                 

test/support/data_case.ex

  • Added use Plausible to the quote block in DataCase template
  • Enables EE-specific test functionality via on_ee macro
+1/-0     

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Typo In Module: Test module name Plausible.CondolidatedView.CacheTest contains a typo that reduces clarity
and may break conventions.

Referred Code
defmodule Plausible.CondolidatedView.CacheTest do
  use Plausible.DataCase, async: true

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing Auditing: The new caching and lookup operations for consolidated views introduce critical data
access/update paths without any visible audit logging of actions, users, or outcomes in
the added code.

Referred Code
@spec refresh_updated_recently(Keyword.t()) :: :ok
def refresh_updated_recently(opts) do
  recently_updated_site_ids =
    from sc in ConsolidatedView.sites(),
      join: sr in ^Plausible.Site.regular(),
      on: sc.team_id == sr.team_id,
      where: sr.updated_at > ago(^15, "minute") or sc.updated_at > ago(^15, "minute"),
      select: sc.id

  query =
    from sc in ConsolidatedView.sites(),
      join: sr in ^Plausible.Site.regular(),
      on: sr.team_id == sc.team_id,
      where: sc.id in subquery(recently_updated_site_ids),
      group_by: sc.id,
      order_by: [desc: sc.id],
      select: %{consolidated_view_id: sc.domain, site_ids: fragment("array_agg(?)", sr.id)}

  refresh(
    :updated_recently,
    query,


 ... (clipped 3 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Narrow Handling: Functions like get_from_source/1 only handle {:ok, _} or {:error, :not_found} but do not
surface or log other potential failures from underlying calls, risking silent issues.

Referred Code
@impl true
def get_from_source(consolidated_view_id) do
  case ConsolidatedView.site_ids(consolidated_view_id) do
    {:ok, some} -> some
    {:error, :not_found} -> nil
  end

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input Assumptions: The new site_ids/1 accepts a binary consolidated view id without explicit validation or
normalization before using it in queries, relying on parameterization but lacking explicit
safeguards.

Referred Code
@spec site_ids(Team.t() | String.t()) :: {:ok, [pos_integer()]} | {:error, :not_found}
def site_ids(consolidated_view_id) when is_binary(consolidated_view_id) do
  case get(consolidated_view_id) do
    nil -> {:error, :not_found}
    view -> {:ok, Teams.owned_sites_ids(view.team)}
  end
end

def site_ids(%Team{} = team) do
  site_ids(team.identifier)
end

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix incorrect Ecto fragment usage

Correct the Ecto query fragment in refresh_updated_recently/1 to use
fragment("array_agg(?.id)", sr) for proper SQL aggregation, fixing a bug.

extra/lib/plausible/consolidated_view/cache.ex [59]

-select: %{consolidated_view_id: sc.domain, site_ids: fragment("array_agg(?)", sr.id)}
+select: %{consolidated_view_id: sc.domain, site_ids: fragment("array_agg(?.id)", sr)}
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a bug in an Ecto query fragment that would cause the query to fail or return incorrect data, making it a critical fix for the new functionality.

High
General
Supervise test cache for proper cleanup

Refactor the start_test_cache/1 helper to use start_supervised! for starting the
cache process, ensuring it is properly terminated with the test.

test/plausible/consolidated_view/cache_test.exs [91-94]

 defp start_test_cache(cache_name) do
-  %{start: {m, f, a}} = Cache.child_spec(cache_name: cache_name)
-  apply(m, f, a)
+  start_supervised!({Cache, cache_name: cache_name})
 end
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion improves test suite robustness by using start_supervised! to prevent orphaned processes, which is a best practice that enhances test isolation and reliability.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants