From 520ffa4c4c6c41264b0375d77c8ab9a687f1100b Mon Sep 17 00:00:00 2001 From: miteruzo Date: Sat, 7 Nov 2026 18:34:17 +0900 Subject: [PATCH] #442 --- .../controllers/post_versions_controller.rb | 3 +- backend/app/controllers/posts_controller.rb | 31 +++++++++++--- backend/app/models/history_access.rb | 13 +++--- .../requests/post_history_privacy_spec.rb | 35 ++++++++++++++++ backend/spec/requests/posts_spec.rb | 41 +++++++++++++++++++ backend/spec/services/counts_export_spec.rb | 10 ++++- 6 files changed, 117 insertions(+), 16 deletions(-) diff --git a/backend/app/controllers/post_versions_controller.rb b/backend/app/controllers/post_versions_controller.rb index 92b35c7..cf07119 100644 --- a/backend/app/controllers/post_versions_controller.rb +++ b/backend/app/controllers/post_versions_controller.rb @@ -15,7 +15,6 @@ class PostVersionsController < ApplicationController offset = (page - 1) * limit q = HistoryAccess.visible(PostVersion.all) - q = q.with_previous_version if tag_id || external_tag_id q = q.where('post_versions.post_id = ?', post_id) if post_id q = q.with_previous_version if tag_id || external_tag_id if external_tag_id || (tag_id && !(Tag.exists?(id: tag_id))) @@ -34,7 +33,7 @@ class PostVersionsController < ApplicationController .limit(limit) .offset(offset) .pluck('post_versions.id') - versions = HistoryAccess.visible(version_scope.where(id: ids)) + versions = version_scope.merge(q).where(id: ids) .order(Arel.sql('post_versions.created_at DESC, post_versions.id DESC')) render json: { versions: serialise_versions(versions, locale.language_code), count:, population_count: HistoryAccess.population_count(PostVersion) } diff --git a/backend/app/controllers/posts_controller.rb b/backend/app/controllers/posts_controller.rb index 4d96673..ba15e99 100644 --- a/backend/app/controllers/posts_controller.rb +++ b/backend/app/controllers/posts_controller.rb @@ -37,7 +37,7 @@ class PostsController < ApplicationController offset = (page - 1) * limit - updated_at_all_sql = 'posts.updated_at' + needs_history_join = order[0] == 'updated_at' || updated_between.any?(&:present?) q = filtered_posts(locale) @@ -56,12 +56,25 @@ class PostsController < ApplicationController end q = q.where('posts.created_at >= ?', created_between[0]) if created_between[0] q = q.where('posts.created_at <= ?', created_between[1]) if created_between[1] + count_scope = q + if needs_history_join + history_times = HistoryAccess.visible(PostVersion.all) + .select('post_id, MAX(created_at) AS latest_at').group(:post_id) + q = q.joins("LEFT JOIN (#{ history_times.to_sql }) history_times " + + 'ON history_times.post_id = posts.id') + updated_at_all_sql = 'GREATEST(posts.updated_at, ' + + 'COALESCE(history_times.latest_at, posts.updated_at))' + end if updated_between[0] q = q.where("#{ updated_at_all_sql } >= ?", updated_between[0]) end if updated_between[1] q = q.where("#{ updated_at_all_sql } <= ?", updated_between[1]) end + count_scope = q if updated_between.any?(&:present?) + if needs_history_join + q = q.select('posts.*', "#{ updated_at_all_sql } AS updated_at") + end sort_sql = case order[0] @@ -78,14 +91,20 @@ class PostsController < ApplicationController .limit(limit) .offset(offset) .to_a - latest_version_times = HistoryAccess.visible(PostVersion.where(post_id: posts.map(&:id))) - .group(:post_id).maximum(:created_at) + latest_version_times = + if needs_history_join + { } + else + HistoryAccess.visible(PostVersion.where(post_id: posts.map(&:id))) + .group(:post_id).maximum(:created_at) + end - q = q.except(:select, :order) + count_scope = count_scope.except(:select, :order) render json: { posts: posts.map { |post| + updated_at = [post.updated_at, latest_version_times[post.id]].compact.max PostRepr.base(post, language_code: locale.language_code) - .merge(updated_at: post.updated_at).tap do |json| + .merge('updated_at' => updated_at).tap do |json| json['thumbnail'] = if post.thumbnail.attached? rails_storage_proxy_url(post.thumbnail, only_path: false) @@ -93,7 +112,7 @@ class PostsController < ApplicationController nil end end - }, count: q.group_values.present? ? q.count.size : q.count } + }, count: count_scope.group_values.present? ? count_scope.count.size : count_scope.count } end def random diff --git a/backend/app/models/history_access.rb b/backend/app/models/history_access.rb index 5745e73..a0ef71f 100644 --- a/backend/app/models/history_access.rb +++ b/backend/app/models/history_access.rb @@ -59,20 +59,21 @@ class HistoryAccess < ActiveSupport::CurrentAttributes end def self.json_reference(table, key, document) - "NOT EXISTS (SELECT 1 FROM JSON_TABLE(COALESCE(#{ document }, JSON_ARRAY()), " + + # A scalar lookup avoids MySQL's semijoin rewrite losing the JSON row correlation. + "(SELECT 1 FROM JSON_TABLE(COALESCE(#{ document }, JSON_ARRAY()), " + "'$[*]' COLUMNS (reference_id BIGINT PATH '$' NULL ON EMPTY)) reference_entry " + "JOIN (#{ discarded_keys(table, key) }) blocked " + - "ON reference_entry.reference_id = blocked.#{ key })" + "ON reference_entry.reference_id = blocked.#{ key } LIMIT 1) IS NULL" end def self.list_reference(table, key, document) # Preserve FIND_IN_SET's exact token semantics, including empty tokens. array = "CONCAT('[', REPLACE(JSON_QUOTE(REPLACE(COALESCE(#{ document }, ''), " + "' ', ',')), ',', '\",\"'), ']')" - "NOT EXISTS (SELECT 1 FROM JSON_TABLE(#{ array }, '$[*]' " + + "(SELECT 1 FROM JSON_TABLE(#{ array }, '$[*]' " + "COLUMNS (reference_id VARCHAR(255) PATH '$')) reference_entry " + "JOIN (#{ discarded_keys(table, key) }) blocked " + - "ON reference_entry.reference_id = CAST(blocked.#{ key } AS CHAR))" + "ON reference_entry.reference_id = CAST(blocked.#{ key } AS CHAR) LIMIT 1) IS NULL" end def self.snapshot_reference(model, row, external: false) @@ -91,8 +92,8 @@ class HistoryAccess < ActiveSupport::CurrentAttributes match += ' AND reference_version.version_no = tag_reference.version_no' latest = '' end - "NOT EXISTS (SELECT 1 FROM #{ source } JOIN #{ table } reference_version ON #{ match } " + - "WHERE NOT (#{ sql(model, 'reference_version') }) #{ latest })" + "(SELECT 1 FROM #{ source } JOIN #{ table } reference_version ON #{ match } " + + "WHERE NOT (#{ sql(model, 'reference_version') }) #{ latest } LIMIT 1) IS NULL" end def self.reference_sql(table, row) diff --git a/backend/spec/requests/post_history_privacy_spec.rb b/backend/spec/requests/post_history_privacy_spec.rb index 980eecc..635fb76 100644 --- a/backend/spec/requests/post_history_privacy_spec.rb +++ b/backend/spec/requests/post_history_privacy_spec.rb @@ -37,6 +37,32 @@ RSpec.describe 'Post history privacy', type: :request do end end + it 'evaluates each reference condition against the restored snapshot alone' do + HistoryAccess.set(user: nil) do + restored = PostVersion.where(post_id: post_record.id, version_no: 3) + HistoryAccess.reference_sql('post_versions', 'post_versions').each do |condition| + expect(restored.where(Arel.sql(condition)).pluck(:version_no)).to eq([3]) + end + expect(HistoryAccess.visible(PostVersion.where(post_id: post_record.id)) + .pluck(:version_no)).to eq([3]) + end + end + + it 'keeps every version through a later discard private after another restore' do + ['discard', 'restore'].each_with_index do |event, index| + PostVersion.create!(post: post_record, version_no: index + 5, event_type: event, + title: 'later snapshot', url: post_record.url, tags: '', tags_json: [], + parent_post_ids: '', created_at: origin + (index + 5).days) + end + + get '/posts/versions', params: { post: post_record.id } + + expect(response).to have_http_status(:ok) + expect(response.parsed_body.fetch('versions').pluck('version_no')).to eq([6]) + expect(response.parsed_body.fetch('count')).to eq(1) + expect(response.parsed_body.fetch('versions').first.fetch('title').fetch('prev')).to be_nil + end + ['anonymous', 'member', 'admin'].each do |role| context "as #{ role }" do before do @@ -71,6 +97,15 @@ RSpec.describe 'Post history privacy', type: :request do end end + it 'applies authorised timestamp ordering before pagination' do + get '/posts', params: { order: 'updated_at:desc', limit: 1 } + + expect(response).to have_http_status(:ok) + expected = role == 'admin' ? post_record.id : other_post.id + expect(response.parsed_body.fetch('posts').pluck('id')).to eq([expected]) + expect(response.parsed_body.fetch('count')).to eq(2) + end + it 'counts only visible versions and masks a private previous version' do get '/posts/versions', params: { post: post_record.id } expect(response).to have_http_status(:ok) diff --git a/backend/spec/requests/posts_spec.rb b/backend/spec/requests/posts_spec.rb index f0d0ab7..aa5f7c1 100644 --- a/backend/spec/requests/posts_spec.rb +++ b/backend/spec/requests/posts_spec.rb @@ -629,6 +629,37 @@ RSpec.describe 'Posts API', type: :request do create(:post_tag, post: plain_post, tag:, created_at: t3) end + it 'displays history time after pagination without joining history to posts or count' do + queries = [] + subscriber = ->(event) { queries << event.payload.fetch(:sql) } + ActiveSupport::Notifications.subscribed(subscriber, 'sql.active_record') do + get '/posts', params: { url: '/version-time/', order: 'url:asc', limit: 1 } + end + + expect(response).to have_http_status(:ok) + expect(json.fetch('posts').pluck('id')).to eq([history_post.id]) + expect(Time.zone.parse(json.fetch('posts').first.fetch('updated_at'))).to eq(t2) + expect(json.fetch('count')).to eq(3) + post_queries = queries.select { _1.include?('FROM `posts`') } + expect(post_queries).not_to be_empty + expect(post_queries.grep(/post_versions/)).to be_empty + end + + it 'omits the history aggregate from count when only sorting by update time' do + queries = [] + subscriber = ->(event) { queries << event.payload.fetch(:sql) } + ActiveSupport::Notifications.subscribed(subscriber, 'sql.active_record') do + get '/posts', params: { url: '/version-time/', order: 'updated_at:desc', limit: 1 } + end + + expect(response).to have_http_status(:ok) + expect(json.fetch('posts').pluck('id')).to eq([newer_post.id]) + expect(json.fetch('count')).to eq(3) + count_queries = queries.select { _1.include?('FROM `posts`') && _1.match?(/COUNT\(/i) } + expect(count_queries).not_to be_empty + expect(count_queries.grep(/post_versions/)).to be_empty + end + ['asc', 'desc'].each do |direction| it "sorts by the later of post update and latest version time (#{ direction })" do get '/posts', params: { url: '/version-time/', order: "updated_at:#{ direction }" } @@ -658,6 +689,16 @@ RSpec.describe 'Posts API', type: :request do expect(json.fetch('posts').map { |item| item.fetch('id') }).to eq([history_post.id]) expect(json.fetch('count')).to eq(1) end + + it 'counts every timestamp match before pagination' do + get '/posts', params: { url: '/version-time/', order: 'updated_at:asc', + updated_from: t1.iso8601, updated_to: t3.iso8601, + limit: 1, page: 2 } + + expect(response).to have_http_status(:ok) + expect(json.fetch('posts').pluck('id')).to eq([history_post.id]) + expect(json.fetch('count')).to eq(3) + end end context 'when original_created_from/original_created_to are provided' do diff --git a/backend/spec/services/counts_export_spec.rb b/backend/spec/services/counts_export_spec.rb index de205d1..94b82c1 100644 --- a/backend/spec/services/counts_export_spec.rb +++ b/backend/spec/services/counts_export_spec.rb @@ -221,9 +221,15 @@ RSpec.describe CountsExportDownload::ResponseBody do body = described_class.new(source) { |state| states << state; true } response = ActionDispatch::Response.new response.body = body - deferred = response.stream.to_ary + + _, _, rack_body = response.to_a + expect(states).to eq([]) - expect(deferred.to_a.join).to eq(payload) + + chunks = [] + rack_body.each { |chunk| chunks << chunk } + + expect(chunks.join).to eq(payload) expect(states.count('started')).to eq(1) end end