[Replacements] Continued progress

[Replacements] Fix exceptions when processing replacements

[Replacements] Fix handling of invalid file types

[Replacements] Fix error reporting and promote dupe checking

[Replacements] Prevent duplicate replacements from cropping up
This commit is contained in:
Kira 2020-06-11 17:55:37 -07:00
parent d1afbe6c72
commit a28466bdaf
23 changed files with 235 additions and 85 deletions

View File

@ -60,6 +60,8 @@ class ApplicationController < ActionController::Base
end
case exception
when ProcessingError
render_expected_error(400, exception)
when APIThrottled
render_expected_error(429, "Throttled: Too many requests")
when ActiveRecord::QueryCanceled

View File

@ -1,5 +1,5 @@
class PostReplacementsController < ApplicationController
respond_to :html
respond_to :html, :json
before_action :moderator_only, except: [:index, :create, :new]
before_action :member_only, only: [:create, :new]
@ -11,8 +11,11 @@ class PostReplacementsController < ApplicationController
def create
@post = Post.find(params[:post_id])
@post_replacement = @post.replacements.create(create_params.merge(creator_id: CurrentUser.id, creator_ip_addr: CurrentUser.ip_addr))
flash[:notice] = "Post replacement submitted"
if @post_replacement.errors.size == 0
flash[:notice] = "Post replacement submitted"
else
flash[:notice] = @post_replacement.errors.full_messages.join('; ')
end
respond_with(@post_replacement, location: @post)
end
@ -20,7 +23,7 @@ class PostReplacementsController < ApplicationController
@post_replacement = PostReplacement.find(params[:id])
@post_replacement.approve!
respond_with(@post_replacement)
respond_with(@post_replacement, location: post_path(@post_replacement.post))
end
def reject
@ -37,9 +40,16 @@ class PostReplacementsController < ApplicationController
respond_with(@post_replacement)
end
def promote
@post_replacement = PostReplacement.find(params[:id])
@post = @post_replacement.promote!
respond_with(@post)
end
def index
params[:search][:post_id] = params.delete(:post_id) if params.has_key?(:post_id)
@post_replacements = PostReplacement.search(search_params).paginate(params[:page], limit: params[:limit])
@post_replacements = PostReplacement.visible(CurrentUser.user).search(search_params).paginate(params[:page], limit: params[:limit])
respond_with(@post_replacements)
end

View File

@ -76,6 +76,6 @@ class UploadsController < ApplicationController
permitted_params << :locked_tags if CurrentUser.is_admin?
permitted_params << :locked_rating if CurrentUser.is_privileged?
params.require(:upload).permit(permitted_params)
params.require(:upload).permit(permitted_params).merge(uploader_id: CurrentUser.id, uploader_ip_addr: CurrentUser.ip_addr)
end
end

View File

@ -0,0 +1,6 @@
module PostReplacementHelper
def replacement_thumbnail(replacement)
return tag.a(image_tag(replacement.replacement_thumb_url), href: replacement.replacement_file_url) if replacement.file_visible_to?(CurrentUser.user)
image_tag(replacement.replacement_thumb_url)
end
end

View File

@ -58,6 +58,7 @@
@import "specific/post_flags.scss";
@import "specific/post_mode_menu.scss";
@import "specific/posts.scss";
@import "specific/post_replacements.scss";
@import "specific/post_versions.scss";
@import "specific/related_tags.scss";
@import "specific/sessions.scss";

View File

@ -0,0 +1,7 @@
div#c-post-replacements {
.replacement-pending-row {
@include themable {
background-color: darken( themed("color-danger"), 10%);
}
}
}

View File

@ -98,7 +98,7 @@ module DanbooruImageResizer
image = Vips::Image.new_from_file(filename, fail: true)
image.stats
false
rescue Vips::Error
rescue
true
end
end

View File

@ -0,0 +1,3 @@
class ProcessingError < Exception
end

View File

@ -1,8 +1,6 @@
class UploadService
class Replacer
extend Memoist
class Error < Exception;
end
attr_reader :post, :replacement
@ -16,15 +14,15 @@ class UploadService
end
def replacement_message(post, replacement)
linked_source = linked_source(replacement.replacement_url)
linked_source_was = linked_source(post.source_was)
linked_source = replacement.replacement_url
linked_source_was = post.source_was
<<-EOS.strip_heredoc
[table]
[tbody]
[tr]
[th]Old[/th]
[td]#{linked_source_was}[/td]
[td] #{linked_source_was} [/td]
[td]#{post.md5_was}[/td]
[td]#{post.file_ext_was}[/td]
[td]#{post.image_width_was} x #{post.image_height_was}[/td]
@ -32,7 +30,7 @@ class UploadService
[/tr]
[tr]
[th]New[/th]
[td]#{linked_source}[/td]
[td] #{linked_source} [/td]
[td]#{post.md5}[/td]
[td]#{post.file_ext}[/td]
[td]#{post.image_width} x #{post.image_height}[/td]
@ -43,26 +41,13 @@ class UploadService
EOS
end
def linked_source(source)
return nil if source.nil?
# truncate long sources in the middle: "www.pixiv.net...lust_id=23264933"
truncated_source = source.gsub(%r{\Ahttps?://}, "").truncate(64, omission: "...#{source.last(32)}")
if source =~ %r{\Ahttps?://}i
%("#{truncated_source}":[#{source}])
else
truncated_source
end
end
def find_replacement_url(repl, upload)
if repl.replacement_file.present?
return "file://#{repl.file_name}"
end
if !upload.source.present?
raise "No source found in upload for replacement"
raise ProcessingError "No source found in upload for replacement"
end
return upload.source
@ -75,24 +60,32 @@ class UploadService
source: post.source, reason: 'Backup of original file')
repl.replacement_file = Danbooru.config.storage_manager.open(Danbooru.config.storage_manager.file_path(post, post.file_ext, :original))
repl.save
rescue
end
def process!
# Prevent trying to replace deleted posts
raise ProcessingError, "Cannot replace post: post is deleted." if post.is_deleted?
replacement.replacement_file = Danbooru.config.storage_manager.open(Danbooru.config.storage_manager.replacement_path(replacement, replacement.file_ext, :original))
create_backup_replacement
upload = Upload.create(
uploader_id: CurrentUser.id,
uploader_ip_addr: CurrentUser.ip_addr,
rating: post.rating,
tag_string: post.tag_string,
source: replacement.source,
file: replacement.replacement_file,
replaced_post: post,
original_post_id: post.id
original_post_id: post.id,
replacement_id: replacement.id
)
begin
if upload.invalid? || upload.is_errored?
raise Error, upload.status
raise ProcessingError, upload.status
end
upload.update(status: "processing")
@ -103,7 +96,7 @@ class UploadService
upload.save!
rescue Exception => x
upload.update(status: "error: #{x.class} - #{x.message}", backtrace: x.backtrace.join("\n"))
raise Error, upload.status
raise ProcessingError, upload.status
end
md5_changed = upload.md5 != post.md5
@ -117,8 +110,9 @@ class UploadService
post.image_width = upload.image_width
post.image_height = upload.image_height
post.file_size = upload.file_size
post.source += "\n#{self.source}"
post.source += "\n#{replacement.source}"
post.tag_string = upload.tag_string
# Reset ownership information on post.
post.uploader_id = replacement.creator_id
post.uploader_ip_addr = replacement.creator_ip_addr
@ -131,7 +125,7 @@ class UploadService
end
end
replacement.update_attribute(:status, 'approved')
replacement.update_attributes({status: 'approved', approver_id: CurrentUser.id})
post.save!
if post.is_video?

View File

@ -95,6 +95,9 @@ class ModAction < ApplicationRecord
:post_undelete,
:post_unapprove,
:post_rating_lock,
:post_replacement_accept,
:post_replacement_reject,
:post_replacement_delete,
:report_reason_create,
:report_reason_delete,
:report_reason_update,

View File

@ -49,6 +49,7 @@ class Post < ApplicationRecord
belongs_to :parent, class_name: "Post", optional: true
has_one :upload, :dependent => :destroy
has_one :pixiv_ugoira_frame_data, :class_name => "PixivUgoiraFrameData", :dependent => :destroy
has_one :replacement, class_name: "PostImageHash", dependent: :destroy
has_many :flags, :class_name => "PostFlag", :dependent => :destroy
has_many :appeals, :class_name => "PostAppeal", :dependent => :destroy
has_many :votes, :class_name => "PostVote", :dependent => :destroy
@ -1580,6 +1581,11 @@ class Post < ApplicationRecord
UserStatus.for_user(uploader_id).update_all("post_deleted_count = post_deleted_count + 1")
give_favorites_to_parent(options) if options[:move_favorites]
give_post_sets_to_parent if options[:move_favorites]
reject_pending_replacements
end
def reject_pending_replacements
replacements.where(status: 'pending').update_all(status: 'rejected')
end
def undelete!(options = {})

View File

@ -1,13 +1,17 @@
class PostReplacement < ApplicationRecord
self.table_name = 'post_replacements2'
belongs_to :post
belongs_to :creator, class_name: "User"
belongs_to :approver, class_name: "User", optional: true
attr_accessor :replacement_file, :replacement_url, :final_source, :tags
validate :user_is_not_limited, on: :create
validate :post_is_valid, on: :create
validate :set_file_name, on: :create
validate :fetch_source_file, on: :create
validate :update_file_attributes, on: :create
validate :no_pending_duplicates, on: :create
validate :write_storage_file, on: :create
validate :user_is_not_limited, on: :create
before_destroy :remove_files
@ -16,6 +20,15 @@ class PostReplacement < ApplicationRecord
Addressable::URI.heuristic_parse(replacement_url) rescue nil
end
module PostMethods
def post_is_valid
if post.is_deleted?
self.errors.add(:post, "is deleted")
return false
end
end
end
module FileMethods
def is_image?
%w(jpg jpeg gif png).include?(file_ext)
@ -34,6 +47,19 @@ class PostReplacement < ApplicationRecord
end
end
def no_pending_duplicates
post = Post.where(md5: md5).first
if post
self.errors.add(:md5, "duplicate of existing post ##{post.id}")
return false
end
replacements = self.where(status: 'pending', md5: md5)
replacements.each do |replacement|
self.errors.add(:md5, "duplicate of pending replacement on post ##{replacement.post_id}")
end
replacements.size == 0
end
def user_is_not_limited
return true if status == 'original'
replaceable = creator.can_replace_post_with_reason
@ -46,6 +72,18 @@ class PostReplacement < ApplicationRecord
self.errors.add(:creator, User.upload_reason_string(uploadable))
return false
end
# Janitor bypass replacement limits
return true if creator.is_janitor?
if post.replacements.where(creator_id: creator.id).where('created_at > ?', 1.day.ago).count > 2
self.errors.add(:creator, 'has already suggested too many replacement for this post today')
return false
end
if post.replacements.where(creator_id: creator.id).count > 5
self.errors.add(:creator, 'has already suggested too many total replacements for this post')
return false
end
true
end
@ -66,6 +104,10 @@ class PostReplacement < ApplicationRecord
def update_file_attributes
self.file_ext = UploadService::Utils.file_header_to_file_ext(replacement_file)
if file_ext == "bin"
self.errors.add(:base, "Unknown or invalid file format")
throw :abort
end
self.file_size = replacement_file.size
self.md5 = Digest::MD5.file(replacement_file.path).hexdigest
@ -79,7 +121,10 @@ class PostReplacement < ApplicationRecord
if replacement_file.present?
self.file_name = replacement_file.try(:original_filename) || File.basename(replacement_file.path)
else
raise RuntimeError, "No file or source URL provided" if replacement_url_parsed.blank?
if replacement_url_parsed.blank?
self.errors.add(:base, "No file or source URL provided")
throw :abort
end
self.file_name = replacement_url_parsed.basename
end
end
@ -111,23 +156,52 @@ class PostReplacement < ApplicationRecord
module ProcessingMethods
def approve!
transaction do
ModAction.log(:post_replacement_accept, {post_id: post.id, replacement_id: self.id, old_md5: post.md5, new_md5: self.md5})
processor = UploadService::Replacer.new(post: post, replacement: self)
processor.process!
end
end
def promote!
transaction do
processor = UploadService.new(new_upload_params)
new_post = processor.start!
update_attribute(:status, 'promoted')
new_post
end
end
def reject!
ModAction.log(:post_replacement_reject, {post_id: post.id, replacement_id: self.id})
update_attribute(:status, 'rejected')
end
end
module PromotionMethods
def new_upload_params
{
uploader_id: creator_id,
uploader_ip_addr: creator_ip_addr,
file: Danbooru.config.storage_manager.open(Danbooru.config.storage_manager.replacement_path(self, file_ext, :original)),
tag_string: post.tag_string,
rating: post.rating,
source: post.source,
parent_id: post.id,
description: post.description,
locked_tags: post.locked_tags,
replacement_id: self.id
}
end
end
concerning :Search do
class_methods do
def search(params = {})
q = super
q = q.attribute_matches(:file_ext, params[:file_ext])
q = q.attribute_matches(:md5, params[:md5])
q = q.attribute_exact_matches(:file_ext, params[:file_ext])
q = q.attribute_exact_matches(:md5, params[:md5])
q = q.attribute_exact_matches(:status, params[:status])
if params[:creator_id].present?
q = q.where(creator_id: params[:creator_id].split(",").map(&:to_i))
@ -141,7 +215,8 @@ class PostReplacement < ApplicationRecord
q = q.where(post_id: params[:post_id].split(",").map(&:to_i))
end
q.apply_default_order(params)
q.order(Arel.sql("CASE status WHEN 'pending' THEN 0 ELSE 1 END ASC, id DESC"))
end
def pending
@ -159,11 +234,17 @@ class PostReplacement < ApplicationRecord
def for_user(id)
where(creator_id: id.to_i)
end
def visible(user)
return where('status != ?', 'rejected') if user.is_anonymous?
return all if user.is_janitor?
where('creator_id = ? or status != ?', user.id, 'rejected')
end
end
end
def file_visible_to?(user)
true if user.is_janitor? || creator_id == user.id && status == 'pending'
return true if user.is_janitor?
false
end
@ -171,5 +252,7 @@ class PostReplacement < ApplicationRecord
include StorageMethods
include FileMethods
include ProcessingMethods
include PromotionMethods
include PostMethods
end

View File

@ -27,6 +27,9 @@ class Upload < ApplicationRecord
end
def validate_file_size(record)
if record.file_size <= 16
record.errors[:file_size] << "is too small"
end
max_size = Danbooru.config.max_file_sizes.fetch(record.file_ext, 0)
if record.file_size > max_size
record.errors.add(:file_size, "is too large. Maximum allowed for this file type is #{max_size / (1024*1024)} MiB")
@ -42,10 +45,11 @@ class Upload < ApplicationRecord
end
replacements = PostReplacement.pending.where(md5: record.md5)
replacements = replacements.where('id != ?', record.replacement_id) if record.replacement_id
if !record.replaced_post && replacements.size > 0
replacements.each do |rep|
record.errors.add(:md5) << "duplicate of pending replacement: #{rep.post_id}"
record.errors.add(:md5) << "duplicate of pending replacement on post ##{rep.post_id}"
end
return
end
@ -97,7 +101,7 @@ class Upload < ApplicationRecord
end
attr_accessor :as_pending, :replaced_post, :file, :direct_url, :is_apng, :original_post_id, :locked_tags, :locked_rating
attr_accessor :as_pending, :replaced_post, :file, :direct_url, :is_apng, :original_post_id, :locked_tags, :locked_rating, :replacement_id
belongs_to :uploader, :class_name => "User"
belongs_to :post, optional: true
@ -113,8 +117,6 @@ class Upload < ApplicationRecord
serialize :context, JSON
def initialize_attributes
self.uploader_id = CurrentUser.id
self.uploader_ip_addr = CurrentUser.ip_addr
self.server = Danbooru.config.server_host
end

View File

@ -5,7 +5,7 @@
<%= simple_form_for(post_replacement, url: post_replacements_path(post_id: post_replacement.post_id), method: :post) do |f| %>
<%= f.input :replacement_file, label: "File", as: :file %>
<%= f.input :replacement_url, label: "Replacement URL", hint: "The source URL to download the replacement from.", as: :string, input_html: { value: post_replacement.post.normalized_source } %>
<%= f.input :replacement_url, label: "Replacement URL", hint: "The source URL to download the replacement from.", as: :string %>
<%= f.input :reason, label: "Reason", hint: "Tell us why this file should replace the original.", as: :string %>
<%= f.submit "Submit" %>
<% end %>

View File

@ -0,0 +1,7 @@
<%= simple_form_for(:search, url: post_replacements_path, method: :get, defaults: { required: false }, html: { class: "inline-form" }) do |f| %>
<%= f.input :md5, label: "MD5", input_html: { value: params[:search][:md5] } %>
<%= f.input :creator_name, label: "Creator", input_html: { value: params[:search][:creator_name] } %>
<%= f.input :post_id, label: "Post ID", input_html: { value: params[:search][:post_id] } %>
<%= f.input :status, label: "status", collection: ["pending", "rejected", "approved", "promoted"], include_blank: true, selected: params[:search][:status] %>
<%= f.submit "Search" %>
<% end %>

View File

@ -2,13 +2,9 @@
<div id="a-index">
<h1>Post Replacements</h1>
<%= render "search" %>
<%= render "posts/partials/common/inline_blacklist" %>
<%= simple_form_for(:search, url: post_replacements_path, method: :get, defaults: { required: false }, html: { class: "inline-form" }) do |f| %>
<%= f.input :creator_name, label: "Replacer", input_html: { value: params.dig(:search, :creator_name), data: { autocomplete: "user" } } %>
<%= f.submit "Search" %>
<% end %>
<table width="100%" class="striped autofit">
<thead>
<tr>
@ -24,9 +20,9 @@
</thead>
<tbody>
<% @post_replacements.each do |post_replacement| %>
<tr>
<tr class="<%= 'replacement-pending-row' if post_replacement.status == 'pending' %>">
<td><%= PostPresenter.preview(post_replacement.post, show_deleted: true) %></td>
<td><%= tag.a(image_tag(post_replacement.replacement_thumb_url), href: post_replacement.replacement_file_url) %></td>
<td><%= replacement_thumbnail(post_replacement) %></td>
<td>
<dl>
<dt>Replacement Source</dt>
@ -37,6 +33,8 @@
<em>file</em>
<% end %>
</dd>
<dt>Filename</dt>
<dd><%= post_replacement.file_name %></dd>
</dl>
</td>
@ -62,7 +60,16 @@
</td>
<td>
<%= post_replacement.status %>
<dl>
<dt>Status</dt>
<dd><%= post_replacement.status %></dd>
<dt>Reason</dt>
<dd><%= post_replacement.reason %></dd>
<% if post_replacement.status == 'approved' %>
<dt>Approver</dt>
<dd><%= link_to_user post_replacement.approver %></dd>
<% end %>
</dl>
</td>
<td>
@ -72,9 +79,14 @@
</td>
<td>
<%= link_to "Approve", approve_post_replacement_path(post_replacement), method: :PUT %><br>
<%= link_to "Reject", reject_post_replacement_path(post_replacement), method: :PUT %><br>
<%= link_to "Destroy", post_replacement_path(post_replacement), method: :DELETE %>
<% if CurrentUser.is_janitor? %>
<%= link_to "Approve", approve_post_replacement_path(post_replacement), method: :PUT %><br>
<%= link_to "Reject", reject_post_replacement_path(post_replacement), method: :PUT %><br>
<%= link_to "Promote", promote_post_replacement_path(post_replacement), method: :PUT %><br>
<% end %>
<% if CurrentUser.is_moderator? %>
<%= link_to "Destroy", post_replacement_path(post_replacement), method: :DELETE, 'data-confirm': 'Are you sure you want to destroy this replacement?' %>
<% end %>
</td>
</tr>
<% end %>

View File

@ -47,6 +47,12 @@
</div>
<% end %>
<% if CurrentUser.is_janitor? && post.replacements.pending.any? %>
<div class="notice notice-flagged">
<p>This post has <%= fast_link_to "pending replacements.", post_replacements_path(:search => {:post_id => @post.id}) %></p>
</div>
<% end %>
<% if CurrentUser.is_janitor? && (post.is_flagged? || post.is_deleted?) && post.appeals.any? %>
<div class="notice notice-appealed">
<p>This post was appealed:</p>

View File

@ -63,6 +63,12 @@
</div>
<% end %>
<% if !CurrentUser.is_janitor? && post.replacements.pending.any? %>
<div class="notice notice-flagged">
<p>This post has <%= fast_link_to "pending replacements.", post_replacements_path(:search => {:post_id => @post.id}) %></p>
</div>
<% end %>
<% if !CurrentUser.is_janitor? && (post.is_flagged? || post.is_deleted?) && post.appeals.any? %>
<div class="notice notice-appealed">
<p>This post was appealed:</p>

View File

@ -51,10 +51,9 @@
<li><%= link_to "Update IQDB", update_iqdb_post_path(@post) %></li>
<li><%= tag.a "Destroy", href: '#', id: 'destroy-post-link', 'data-pid': post.id %></li>
<% end %>
<% if CurrentUser.is_moderator? %>
<li><%= link_to "Replace image", new_post_replacement_path(post_id: post.id), id: "replace-image" %></li>
<% end %>
<% end %>
<% if CurrentUser.is_member? %>
<li><%= link_to "Replace image", new_post_replacement_path(post_id: post.id), id: "replace-image" %></li>
<% end %>
<% end %>
<% end %>

View File

@ -247,9 +247,13 @@ Rails.application.routes.draw do
end
end
resources :post_replacements, :only => [:index, :new, :create, :destroy] do
collection do
get :queue
end
member do
put :approve
put :reject
put :promote
end
end
resources :deleted_posts, only: [:index]

View File

@ -1,18 +0,0 @@
class NewPostReplacementSystem < ActiveRecord::Migration[6.0]
def change
remove_column :post_replacements, :original_url, :text
remove_column :post_replacements, :replacement_url, :text
remove_column :post_replacements, :file_ext_was, :string
remove_column :post_replacements, :file_size_was, :integer
remove_column :post_replacements, :md5_was, :string
remove_column :post_replacements, :image_width_was, :integer
remove_column :post_replacements, :image_height_was, :integer
add_column :post_replacements, :creator_ip_addr, :inet, null: false
add_column :post_replacements, :source, :string
add_column :post_replacements, :file_name, :string
add_column :post_replacements, :storage_id, :string, null: false
add_column :post_replacements, :status, :string, null: false, default: 'pending'
add_column :post_replacements, :reason, :string, null: false
add_column :post_replacements, :protected, :boolean, default: false
end
end

View File

@ -0,0 +1,25 @@
class CreateNewReplacementsTable < ActiveRecord::Migration[6.0]
def change
create_table :post_replacements2 do |t|
t.timestamps
t.integer :post_id, null: false
t.integer :creator_id, null: false
t.inet :creator_ip_addr, null: false
t.integer :approver_id
t.string :file_ext, length: 8, null: false
t.integer :file_size, null: false
t.integer :image_height, null: false
t.integer :image_width, null: false
t.string :md5, null: false
t.string :source
t.string :file_name, length: 512
t.string :storage_id, null: false
t.string :status, null: false, default: 'pending'
t.string :reason, null: false, length: 500
t.boolean :protected, null: false, default: false
end
add_index :post_replacements2, :creator_id
add_index :post_replacements2, :post_id
end
end

View File

@ -1595,13 +1595,6 @@ CREATE TABLE public.post_replacements (
id integer NOT NULL,
post_id integer NOT NULL,
creator_id integer NOT NULL,
original_url text NOT NULL,
replacement_url text NOT NULL,
created_at timestamp without time zone NOT NULL,
updated_at timestamp without time zone NOT NULL,
file_ext_was character varying,
file_size_was integer,
image_width_was integer,
created_at timestamp without time zone NOT NULL,
updated_at timestamp without time zone NOT NULL,
file_ext character varying,
@ -5093,4 +5086,3 @@ INSERT INTO "schema_migrations" (version) VALUES
('20201220190335'),
('20210117173030');