DEV: Ability to collect stats without exposing them via API (#23933)

This adds the ability to collect stats without exposing them 
among other stats via API.

The most important thing I wanted to achieve is to provide 
an API where stats are not exposed by default, and a developer 
has to explicitly specify that they should be 
exposed (`expose_via_api: true`). Implementing an opposite 
solution would be simpler, but that's less safe in terms of 
potential security issues. 

When working on this, I had to refactor the current solution. 
I would go even further with the refactoring, but the next steps 
seem to be going too far in changing the solution we have, 
and that would also take more time. Two things that can be 
improved in the future:
1. Data structures for holding stats can be further improved
2. Core stats are hard-coded in the About template (it's hard 
to fix it without correcting data structures first, see point 1):
    63a0700d45/app/views/about/index.html.erb (L61-L101)

The most significant refactorings are:
1. Introducing the `Stat` model
2. Aligning the way the core and the plugin stats' are registered
This commit is contained in:
Andrei Prigorshnev 2023-11-10 00:44:05 +04:00 committed by GitHub
parent bdb81b5346
commit d91456fd53
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
15 changed files with 228 additions and 119 deletions

View File

@ -110,21 +110,21 @@
<td>{{number this.model.stats.topics_last_day}}</td>
<td>{{number this.model.stats.topics_7_days}}</td>
<td>{{number this.model.stats.topics_30_days}}</td>
<td>{{number this.model.stats.topic_count}}</td>
<td>{{number this.model.stats.topics_count}}</td>
</tr>
<tr class="about-post-count">
<td>{{i18n "about.post_count"}}</td>
<td>{{number this.model.stats.posts_last_day}}</td>
<td>{{number this.model.stats.posts_7_days}}</td>
<td>{{number this.model.stats.posts_30_days}}</td>
<td>{{number this.model.stats.post_count}}</td>
<td>{{number this.model.stats.posts_count}}</td>
</tr>
<tr class="about-user-count">
<td>{{i18n "about.user_count"}}</td>
<td>{{number this.model.stats.users_last_day}}</td>
<td>{{number this.model.stats.users_7_days}}</td>
<td>{{number this.model.stats.users_30_days}}</td>
<td>{{number this.model.stats.user_count}}</td>
<td>{{number this.model.stats.users_count}}</td>
</tr>
<tr class="about-active-user-count">
<td>{{i18n "about.active_user_count"}}</td>
@ -138,7 +138,7 @@
<td>{{number this.model.stats.likes_last_day}}</td>
<td>{{number this.model.stats.likes_7_days}}</td>
<td>{{number this.model.stats.likes_30_days}}</td>
<td>{{number this.model.stats.like_count}}</td>
<td>{{number this.model.stats.likes_count}}</td>
</tr>
{{#each
this.site.displayed_about_plugin_stat_groups

View File

@ -2,10 +2,7 @@
class About
def self.displayed_plugin_stat_groups
DiscoursePluginRegistry
.about_stat_groups
.select { |stat_group| stat_group[:show_in_ui] }
.map { |stat_group| stat_group[:name] }
DiscoursePluginRegistry.stats.select { |stat| stat.show_in_ui }.map { |stat| stat.name }
end
class CategoryMods
@ -28,7 +25,7 @@ class About
end
def self.fetch_stats
About.new.stats
Stat.api_stats
end
def initialize(user = nil)
@ -64,57 +61,7 @@ class About
end
def stats
@stats ||= {
topic_count: Topic.listable_topics.count,
topics_last_day: Topic.listable_topics.where("created_at > ?", 1.days.ago).count,
topics_7_days: Topic.listable_topics.where("created_at > ?", 7.days.ago).count,
topics_30_days: Topic.listable_topics.where("created_at > ?", 30.days.ago).count,
post_count: Post.count,
posts_last_day: Post.where("created_at > ?", 1.days.ago).count,
posts_7_days: Post.where("created_at > ?", 7.days.ago).count,
posts_30_days: Post.where("created_at > ?", 30.days.ago).count,
user_count: User.real.count,
users_last_day: User.real.where("created_at > ?", 1.days.ago).count,
users_7_days: User.real.where("created_at > ?", 7.days.ago).count,
users_30_days: User.real.where("created_at > ?", 30.days.ago).count,
active_users_last_day: User.where("last_seen_at > ?", 1.days.ago).count,
active_users_7_days: User.where("last_seen_at > ?", 7.days.ago).count,
active_users_30_days: User.where("last_seen_at > ?", 30.days.ago).count,
like_count: UserAction.where(action_type: UserAction::LIKE).count,
likes_last_day:
UserAction.where(action_type: UserAction::LIKE).where("created_at > ?", 1.days.ago).count,
likes_7_days:
UserAction.where(action_type: UserAction::LIKE).where("created_at > ?", 7.days.ago).count,
likes_30_days:
UserAction.where(action_type: UserAction::LIKE).where("created_at > ?", 30.days.ago).count,
}.merge(plugin_stats)
end
def plugin_stats
final_plugin_stats = {}
DiscoursePluginRegistry.about_stat_groups.each do |stat_group|
begin
stats = stat_group[:block].call
rescue StandardError => err
Discourse.warn_exception(
err,
message: "Unexpected error when collecting #{stat_group[:name]} About stats.",
)
next
end
if !stats.key?(:last_day) || !stats.key?("7_days") || !stats.key?("30_days") ||
!stats.key?(:count)
Rails.logger.warn(
"Plugin stat group #{stat_group[:name]} for About stats does not have all required keys, skipping.",
)
else
final_plugin_stats.merge!(
stats.transform_keys { |key| "#{stat_group[:name]}_#{key}".to_sym },
)
end
end
final_plugin_stats
@stats ||= About.fetch_stats
end
def category_moderators

61
app/models/stat.rb Normal file
View File

@ -0,0 +1,61 @@
# frozen_string_literal: true
class Stat
def initialize(name, show_in_ui: false, expose_via_api: false, &block)
@name = name
@show_in_ui = show_in_ui
@expose_via_api = expose_via_api
@block = block
end
attr_reader :name, :expose_via_api, :show_in_ui
def calculate
@block.call.transform_keys { |key| build_key(key) }
rescue StandardError => err
Discourse.warn_exception(err, message: "Unexpected error when collecting #{@name} About stats.")
{}
end
def self.all_stats
calculate(_all_stats)
end
def self.api_stats
calculate(_api_stats)
end
private
def build_key(key)
"#{@name}_#{key}".to_sym
end
def self._all_stats
core_stats.concat(plugin_stats)
end
def self.calculate(stats)
stats.map { |stat| stat.calculate }.reduce(Hash.new, :merge)
end
def self.core_stats
[
Stat.new("topics", show_in_ui: true, expose_via_api: true) { Statistics.topics },
Stat.new("posts", show_in_ui: true, expose_via_api: true) { Statistics.posts },
Stat.new("users", show_in_ui: true, expose_via_api: true) { Statistics.users },
Stat.new("active_users", show_in_ui: true, expose_via_api: true) { Statistics.active_users },
Stat.new("likes", show_in_ui: true, expose_via_api: true) { Statistics.likes },
]
end
def self._api_stats
_all_stats.select { |stat| stat.expose_via_api }
end
def self.plugin_stats
DiscoursePluginRegistry.stats
end
private_class_method :_all_stats, :calculate, :core_stats, :_api_stats, :plugin_stats
end

View File

@ -30,7 +30,7 @@ class AboutSerializer < ApplicationSerializer
end
def stats
object.class.fetch_cached_stats || Jobs::AboutStats.new.execute({})
object.class.fetch_cached_stats
end
def include_contact_url?

View File

@ -113,7 +113,7 @@ class DiscoursePluginRegistry
define_filtered_register :search_groups_set_query_callbacks
define_filtered_register :about_stat_groups
define_filtered_register :stats
define_filtered_register :bookmarkables
define_filtered_register :list_suggested_for_providers

View File

@ -1110,7 +1110,7 @@ class Plugin::Instance
# but all stats will be shown on the /about.json route. For example take
# this usage:
#
# register_about_stat_group("chat_messages") do
# register_stat("chat_messages") do
# { last_day: 1, "7_days" => 10, "30_days" => 100, count: 1000, previous_30_days: 150 }
# end
#
@ -1132,18 +1132,12 @@ class Plugin::Instance
# group of stats is shown on the site About page in the Site Statistics
# table. Some stats may be needed purely for reporting purposes and thus
# do not need to be shown in the UI to admins/users.
def register_about_stat_group(plugin_stat_group_name, show_in_ui: false, &block)
def register_stat(name, show_in_ui: false, expose_via_api: false, &block)
# We do not want to register and display the same group multiple times.
if DiscoursePluginRegistry.about_stat_groups.any? { |stat_group|
stat_group[:name] == plugin_stat_group_name
}
return
end
return if DiscoursePluginRegistry.stats.any? { |stat| stat.name == name }
DiscoursePluginRegistry.register_about_stat_group(
{ name: plugin_stat_group_name, show_in_ui: show_in_ui, block: block },
self,
)
stat = Stat.new(name, show_in_ui: show_in_ui, expose_via_api: expose_via_api, &block)
DiscoursePluginRegistry.register_stat(stat, self)
end
##

50
lib/statistics.rb Normal file
View File

@ -0,0 +1,50 @@
# frozen_string_literal: true
class Statistics
def self.active_users
{
last_day: User.where("last_seen_at > ?", 1.days.ago).count,
"7_days": User.where("last_seen_at > ?", 7.days.ago).count,
"30_days": User.where("last_seen_at > ?", 30.days.ago).count,
}
end
def self.likes
{
last_day:
UserAction.where(action_type: UserAction::LIKE).where("created_at > ?", 1.days.ago).count,
"7_days":
UserAction.where(action_type: UserAction::LIKE).where("created_at > ?", 7.days.ago).count,
"30_days":
UserAction.where(action_type: UserAction::LIKE).where("created_at > ?", 30.days.ago).count,
count: UserAction.where(action_type: UserAction::LIKE).count,
}
end
def self.posts
{
last_day: Post.where("created_at > ?", 1.days.ago).count,
"7_days": Post.where("created_at > ?", 7.days.ago).count,
"30_days": Post.where("created_at > ?", 30.days.ago).count,
count: Post.count,
}
end
def self.topics
{
last_day: Topic.listable_topics.where("created_at > ?", 1.days.ago).count,
"7_days": Topic.listable_topics.where("created_at > ?", 7.days.ago).count,
"30_days": Topic.listable_topics.where("created_at > ?", 30.days.ago).count,
count: Topic.listable_topics.count,
}
end
def self.users
{
last_day: User.real.where("created_at > ?", 1.days.ago).count,
"7_days": User.real.where("created_at > ?", 7.days.ago).count,
"30_days": User.real.where("created_at > ?", 30.days.ago).count,
count: User.real.count,
}
end
end

View File

@ -4,40 +4,40 @@ module Chat
class Statistics
def self.about_messages
{
:last_day => Chat::Message.where("created_at > ?", 1.days.ago).count,
"7_days" => Chat::Message.where("created_at > ?", 7.days.ago).count,
"30_days" => Chat::Message.where("created_at > ?", 30.days.ago).count,
:previous_30_days =>
last_day: Chat::Message.where("created_at > ?", 1.days.ago).count,
"7_days": Chat::Message.where("created_at > ?", 7.days.ago).count,
"30_days": Chat::Message.where("created_at > ?", 30.days.ago).count,
previous_30_days:
Chat::Message.where("created_at BETWEEN ? AND ?", 60.days.ago, 30.days.ago).count,
:count => Chat::Message.count,
count: Chat::Message.count,
}
end
def self.about_channels
{
:last_day => Chat::Channel.where(status: :open).where("created_at > ?", 1.days.ago).count,
"7_days" => Chat::Channel.where(status: :open).where("created_at > ?", 7.days.ago).count,
"30_days" => Chat::Channel.where(status: :open).where("created_at > ?", 30.days.ago).count,
:previous_30_days =>
last_day: Chat::Channel.where(status: :open).where("created_at > ?", 1.days.ago).count,
"7_days": Chat::Channel.where(status: :open).where("created_at > ?", 7.days.ago).count,
"30_days": Chat::Channel.where(status: :open).where("created_at > ?", 30.days.ago).count,
previous_30_days:
Chat::Channel
.where(status: :open)
.where("created_at BETWEEN ? AND ?", 60.days.ago, 30.days.ago)
.count,
:count => Chat::Channel.where(status: :open).count,
count: Chat::Channel.where(status: :open).count,
}
end
def self.about_users
{
:last_day => Chat::Message.where("created_at > ?", 1.days.ago).distinct.count(:user_id),
"7_days" => Chat::Message.where("created_at > ?", 7.days.ago).distinct.count(:user_id),
"30_days" => Chat::Message.where("created_at > ?", 30.days.ago).distinct.count(:user_id),
:previous_30_days =>
last_day: Chat::Message.where("created_at > ?", 1.days.ago).distinct.count(:user_id),
"7_days": Chat::Message.where("created_at > ?", 7.days.ago).distinct.count(:user_id),
"30_days": Chat::Message.where("created_at > ?", 30.days.ago).distinct.count(:user_id),
previous_30_days:
Chat::Message
.where("created_at BETWEEN ? AND ?", 60.days.ago, 30.days.ago)
.distinct
.count(:user_id),
:count => Chat::Message.distinct.count(:user_id),
count: Chat::Message.distinct.count(:user_id),
}
end

View File

@ -480,11 +480,13 @@ after_initialize do
register_email_unsubscriber("chat_summary", EmailControllerHelper::ChatSummaryUnsubscriber)
register_about_stat_group("chat_messages", show_in_ui: true) { Chat::Statistics.about_messages }
register_stat("chat_messages", show_in_ui: true, expose_via_api: true) do
Chat::Statistics.about_messages
end
register_about_stat_group("chat_channels") { Chat::Statistics.about_channels }
register_stat("chat_channels", expose_via_api: true) { Chat::Statistics.about_channels }
register_about_stat_group("chat_users") { Chat::Statistics.about_users }
register_stat("chat_users", expose_via_api: true) { Chat::Statistics.about_users }
# Make sure to update spec/system/hashtag_autocomplete_spec.rb when changing this.
register_hashtag_data_source(Chat::ChannelHashtagDataSource)

View File

@ -79,8 +79,8 @@ describe Chat::Statistics do
it "counts non-deleted messages created in all status channels in the time period accurately" do
about_messages = described_class.about_messages
expect(about_messages[:last_day]).to eq(1)
expect(about_messages["7_days"]).to eq(3)
expect(about_messages["30_days"]).to eq(7)
expect(about_messages[:"7_days"]).to eq(3)
expect(about_messages[:"30_days"]).to eq(7)
expect(about_messages[:previous_30_days]).to eq(2)
expect(about_messages[:count]).to eq(10)
end
@ -90,8 +90,8 @@ describe Chat::Statistics do
it "counts open channels created in the time period accurately" do
about_channels = described_class.about_channels
expect(about_channels[:last_day]).to eq(1)
expect(about_channels["7_days"]).to eq(3)
expect(about_channels["30_days"]).to eq(5)
expect(about_channels[:"7_days"]).to eq(3)
expect(about_channels[:"30_days"]).to eq(5)
expect(about_channels[:previous_30_days]).to eq(1)
expect(about_channels[:count]).to eq(6)
end
@ -101,8 +101,8 @@ describe Chat::Statistics do
it "counts any users who have sent any message to a chat channel in the time periods accurately" do
about_users = described_class.about_users
expect(about_users[:last_day]).to eq(1)
expect(about_users["7_days"]).to eq(2)
expect(about_users["30_days"]).to eq(3)
expect(about_users[:"7_days"]).to eq(2)
expect(about_users[:"30_days"]).to eq(3)
expect(about_users[:previous_30_days]).to eq(2)
expect(about_users[:count]).to eq(4)
end

View File

@ -23,9 +23,9 @@ RSpec.describe DiscourseHub do
DiscourseHub.stats_fetched_at = 8.days.ago
json = JSON.parse(DiscourseHub.version_check_payload.to_json)
expect(json["topic_count"]).to be_present
expect(json["post_count"]).to be_present
expect(json["user_count"]).to be_present
expect(json["topics_count"]).to be_present
expect(json["posts_count"]).to be_present
expect(json["users_count"]).to be_present
expect(json["topics_7_days"]).to be_present
expect(json["topics_30_days"]).to be_present
expect(json["posts_7_days"]).to be_present
@ -34,7 +34,7 @@ RSpec.describe DiscourseHub do
expect(json["users_30_days"]).to be_present
expect(json["active_users_7_days"]).to be_present
expect(json["active_users_30_days"]).to be_present
expect(json["like_count"]).to be_present
expect(json["likes_count"]).to be_present
expect(json["likes_7_days"]).to be_present
expect(json["likes_30_days"]).to be_present
expect(json["installed_version"]).to be_present

View File

@ -808,15 +808,15 @@ RSpec.describe Plugin::Instance do
end
end
describe "#register_about_stat_group" do
describe "#register_stat" do
let(:plugin) { Plugin::Instance.new }
after { DiscoursePluginRegistry.reset! }
it "registers an about stat group correctly" do
stats = { :last_day => 1, "7_days" => 10, "30_days" => 100, :count => 1000 }
plugin.register_about_stat_group("some_group", show_in_ui: true) { stats }
expect(About.new.plugin_stats.with_indifferent_access).to match(
plugin.register_stat("some_group", show_in_ui: true) { stats }
expect(Stat.all_stats.with_indifferent_access).to match(
hash_including(
some_group_last_day: 1,
some_group_7_days: 10,
@ -828,15 +828,15 @@ RSpec.describe Plugin::Instance do
it "hides the stat group from the UI by default" do
stats = { :last_day => 1, "7_days" => 10, "30_days" => 100, :count => 1000 }
plugin.register_about_stat_group("some_group") { stats }
plugin.register_stat("some_group") { stats }
expect(About.displayed_plugin_stat_groups).to eq([])
end
it "does not allow duplicate named stat groups" do
stats = { :last_day => 1, "7_days" => 10, "30_days" => 100, :count => 1000 }
plugin.register_about_stat_group("some_group") { stats }
plugin.register_about_stat_group("some_group") { stats }
expect(DiscoursePluginRegistry.about_stat_groups.count).to eq(1)
plugin.register_stat("some_group") { stats }
plugin.register_stat("some_group") { stats }
expect(DiscoursePluginRegistry.stats.count).to eq(1)
end
end

View File

@ -5,9 +5,9 @@ RSpec.describe About do
include_examples "stats cacheable"
end
def register_about_stat_group(name, stats_block, show_in_ui: true)
DiscoursePluginRegistry.register_about_stat_group(
{ name: name, show_in_ui: show_in_ui, block: stats_block },
def register_stat(name, stats_block)
DiscoursePluginRegistry.register_stat(
Stat.new(name, show_in_ui: true, expose_via_api: true, &stats_block),
stub(enabled?: true),
)
end
@ -17,7 +17,7 @@ RSpec.describe About do
describe "#stats" do
it "adds plugin stats to the output" do
stats = { :last_day => 1, "7_days" => 10, "30_days" => 100, :count => 1000 }
register_about_stat_group("some_group", Proc.new { stats })
register_stat("some_group", Proc.new { stats })
expect(described_class.new.stats.with_indifferent_access).to match(
hash_including(
some_group_last_day: 1,
@ -30,7 +30,7 @@ RSpec.describe About do
it "does not add plugin stats to the output if they are missing one of the required keys" do
stats = { "7_days" => 10, "30_days" => 100, :count => 1000 }
register_about_stat_group("some_group", Proc.new { stats })
register_stat("some_group", Proc.new { stats })
expect(described_class.new.stats).not_to match(
hash_including(
some_group_last_day: 1,
@ -43,8 +43,8 @@ RSpec.describe About do
it "does not error if any of the plugin stat blocks throw an error and still adds the non-errored stats to output" do
stats = { :last_day => 1, "7_days" => 10, "30_days" => 100, :count => 1000 }
register_about_stat_group("some_group", Proc.new { stats })
register_about_stat_group("other_group", Proc.new { raise StandardError })
register_stat("some_group", Proc.new { stats })
register_stat("other_group", Proc.new { raise StandardError })
expect(described_class.new.stats.with_indifferent_access).to match(
hash_including(
some_group_last_day: 1,

View File

@ -40,9 +40,9 @@ RSpec.describe SiteController do
json = response.parsed_body
expect(response.status).to eq(200)
expect(json["topic_count"]).to be_present
expect(json["post_count"]).to be_present
expect(json["user_count"]).to be_present
expect(json["topics_count"]).to be_present
expect(json["posts_count"]).to be_present
expect(json["users_count"]).to be_present
expect(json["topics_7_days"]).to be_present
expect(json["topics_30_days"]).to be_present
expect(json["posts_7_days"]).to be_present
@ -51,7 +51,7 @@ RSpec.describe SiteController do
expect(json["users_30_days"]).to be_present
expect(json["active_users_7_days"]).to be_present
expect(json["active_users_30_days"]).to be_present
expect(json["like_count"]).to be_present
expect(json["likes_count"]).to be_present
expect(json["likes_7_days"]).to be_present
expect(json["likes_30_days"]).to be_present
end
@ -62,5 +62,32 @@ RSpec.describe SiteController do
get "/site/statistics.json"
expect(response).to redirect_to "/"
end
it "returns exposable stats only" do
Discourse.redis.del(About.stats_cache_key)
SiteSetting.login_required = true
SiteSetting.share_anonymized_statistics = true
plugin = Plugin::Instance.new
plugin.register_stat("private_stat", expose_via_api: false) do
{ :last_day => 1, "7_days" => 2, "30_days" => 3, :count => 4 }
end
plugin.register_stat("exposable_stat", expose_via_api: true) do
{ :last_day => 11, "7_days" => 12, "30_days" => 13, :count => 14 }
end
get "/site/statistics.json"
json = response.parsed_body
expect(json["exposable_stat_last_day"]).to be(11)
expect(json["exposable_stat_7_days"]).to be(12)
expect(json["exposable_stat_30_days"]).to be(13)
expect(json["exposable_stat_count"]).to be(14)
expect(json["private_stat_last_day"]).not_to be_present
expect(json["private_stat_7_days"]).not_to be_present
expect(json["private_stat_30_days"]).not_to be_present
expect(json["private_stat_count"]).not_to be_present
end
end
end

View File

@ -42,4 +42,32 @@ RSpec.describe AboutSerializer do
expect(json[:contact_email]).to eq(SiteSetting.contact_email)
end
end
describe "#stats" do
let(:plugin) { Plugin::Instance.new }
it "serialize exposable stats only" do
Discourse.redis.del(About.stats_cache_key)
plugin.register_stat("private_stat", expose_via_api: false) do
{ :last_day => 1, "7_days" => 2, "30_days" => 3, :count => 4 }
end
plugin.register_stat("exposable_stat", expose_via_api: true) do
{ :last_day => 11, "7_days" => 12, "30_days" => 13, :count => 14 }
end
serializer = AboutSerializer.new(About.new(user), scope: Guardian.new(user), root: nil)
json = serializer.as_json
stats = json[:stats]
expect(stats["exposable_stat_last_day"]).to be(11)
expect(stats["exposable_stat_7_days"]).to be(12)
expect(stats["exposable_stat_30_days"]).to be(13)
expect(stats["exposable_stat_count"]).to be(14)
expect(stats["private_stat_last_day"]).not_to be_present
expect(stats["private_stat_7_days"]).not_to be_present
expect(stats["private_stat_30_days"]).not_to be_present
expect(stats["private_stat_count"]).not_to be_present
end
end
end