From 553d6a1ea61e9327fd68f4e74d4fe4bdfac6e5d3 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 25 Apr 2017 15:04:49 +0200 Subject: [PATCH] Fix #2402 - Add Idempotency-Key header to PostStatusService that prevents (#2419) duplicates. Web UI regenerates UUID for that header every time the compose form is changed or successfully submitted Also, fix Farsi i18n overwriting the English one --- .../components/actions/compose.jsx | 4 +++ .../components/reducers/compose.jsx | 33 +++++++++++++++---- app/assets/javascripts/components/uuid.jsx | 3 ++ app/controllers/api/v1/statuses_controller.rb | 15 ++++++--- app/services/post_status_service.rb | 14 ++++++++ config/locales/simple_form.fa.yml | 2 +- spec/services/post_status_service_spec.rb | 9 ++++- 7 files changed, 67 insertions(+), 13 deletions(-) create mode 100644 app/assets/javascripts/components/uuid.jsx diff --git a/app/assets/javascripts/components/actions/compose.jsx b/app/assets/javascripts/components/actions/compose.jsx index de75ddabe8..d7ff6ea63f 100644 --- a/app/assets/javascripts/components/actions/compose.jsx +++ b/app/assets/javascripts/components/actions/compose.jsx @@ -85,6 +85,10 @@ export function submitCompose() { sensitive: getState().getIn(['compose', 'sensitive']), spoiler_text: getState().getIn(['compose', 'spoiler_text'], ''), visibility: getState().getIn(['compose', 'privacy']) + }, { + headers: { + 'Idempotency-Key': getState().getIn(['compose', 'idempotencyKey']) + } }).then(function (response) { dispatch(submitComposeSuccess({ ...response.data })); diff --git a/app/assets/javascripts/components/reducers/compose.jsx b/app/assets/javascripts/components/reducers/compose.jsx index a411feedd1..c873847804 100644 --- a/app/assets/javascripts/components/reducers/compose.jsx +++ b/app/assets/javascripts/components/reducers/compose.jsx @@ -26,6 +26,7 @@ import { import { TIMELINE_DELETE } from '../actions/timelines'; import { STORE_HYDRATE } from '../actions/store'; import Immutable from 'immutable'; +import uuid from '../uuid'; const initialState = Immutable.Map({ mounted: false, @@ -45,7 +46,8 @@ const initialState = Immutable.Map({ suggestions: Immutable.List(), me: null, default_privacy: 'public', - resetFileKey: Math.floor((Math.random() * 0x10000)) + resetFileKey: Math.floor((Math.random() * 0x10000)), + idempotencyKey: null }); function statusToTextMentions(state, status) { @@ -69,6 +71,7 @@ function clearAll(state) { map.set('privacy', state.get('default_privacy')); map.set('sensitive', false); map.update('media_attachments', list => list.clear()); + map.set('idempotencyKey', uuid()); }); }; @@ -79,6 +82,7 @@ function appendMedia(state, media) { map.set('resetFileKey', Math.floor((Math.random() * 0x10000))); map.update('text', oldText => `${oldText.trim()} ${media.get('text_url')}`); map.set('focusDate', new Date()); + map.set('idempotencyKey', uuid()); }); }; @@ -89,6 +93,7 @@ function removeMedia(state, mediaId) { return state.withMutations(map => { map.update('media_attachments', list => list.filterNot(item => item.get('id') === mediaId)); map.update('text', text => text.replace(media.get('text_url'), '').trim()); + map.set('idempotencyKey', uuid()); if (prevSize === 1) { map.set('sensitive', false); @@ -102,6 +107,7 @@ const insertSuggestion = (state, position, token, completion) => { map.set('suggestion_token', null); map.update('suggestions', Immutable.List(), list => list.clear()); map.set('focusDate', new Date()); + map.set('idempotencyKey', uuid()); }); }; @@ -111,6 +117,7 @@ const insertEmoji = (state, position, emojiData) => { return state.withMutations(map => { map.update('text', oldText => `${oldText.slice(0, position)}${emoji} ${oldText.slice(position)}`); map.set('focusDate', new Date()); + map.set('idempotencyKey', uuid()); }); }; @@ -135,18 +142,27 @@ export default function compose(state = initialState, action) { case COMPOSE_UNMOUNT: return state.set('mounted', false); case COMPOSE_SENSITIVITY_CHANGE: - return state.set('sensitive', !state.get('sensitive')); + return state + .set('sensitive', !state.get('sensitive')) + .set('idempotencyKey', uuid()); case COMPOSE_SPOILERNESS_CHANGE: return state.withMutations(map => { map.set('spoiler_text', ''); map.set('spoiler', !state.get('spoiler')); + map.set('idempotencyKey', uuid()); }); case COMPOSE_SPOILER_TEXT_CHANGE: - return state.set('spoiler_text', action.text); + return state + .set('spoiler_text', action.text) + .set('idempotencyKey', uuid()); case COMPOSE_VISIBILITY_CHANGE: - return state.set('privacy', action.value); + return state + .set('privacy', action.value) + .set('idempotencyKey', uuid()); case COMPOSE_CHANGE: - return state.set('text', action.text); + return state + .set('text', action.text) + .set('idempotencyKey', uuid()); case COMPOSE_REPLY: return state.withMutations(map => { map.set('in_reply_to', action.status.get('id')); @@ -154,6 +170,7 @@ export default function compose(state = initialState, action) { map.set('privacy', privacyPreference(action.status.get('visibility'), state.get('default_privacy'))); map.set('focusDate', new Date()); map.set('preselectDate', new Date()); + map.set('idempotencyKey', uuid()); if (action.status.get('spoiler_text').length > 0) { map.set('spoiler', true); @@ -170,6 +187,7 @@ export default function compose(state = initialState, action) { map.set('spoiler', false); map.set('spoiler_text', ''); map.set('privacy', state.get('default_privacy')); + map.set('idempotencyKey', uuid()); }); case COMPOSE_SUBMIT_REQUEST: return state.set('is_submitting', true); @@ -190,7 +208,10 @@ export default function compose(state = initialState, action) { case COMPOSE_UPLOAD_PROGRESS: return state.set('progress', Math.round((action.loaded / action.total) * 100)); case COMPOSE_MENTION: - return state.update('text', text => `${text}@${action.account.get('acct')} `).set('focusDate', new Date()); + return state + .update('text', text => `${text}@${action.account.get('acct')} `) + .set('focusDate', new Date()) + .set('idempotencyKey', uuid()); case COMPOSE_SUGGESTIONS_CLEAR: return state.update('suggestions', Immutable.List(), list => list.clear()).set('suggestion_token', null); case COMPOSE_SUGGESTIONS_READY: diff --git a/app/assets/javascripts/components/uuid.jsx b/app/assets/javascripts/components/uuid.jsx new file mode 100644 index 0000000000..be18993057 --- /dev/null +++ b/app/assets/javascripts/components/uuid.jsx @@ -0,0 +1,3 @@ +export default function uuid(a) { + return a ? (a^Math.random() * 16 >> a / 4).toString(16) : ([1e7]+-1e3+-4e3+-8e3+-1e11).replace(/[018]/g, uuid); +}; diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 5a2e18cc05..77bdaa4943 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -57,11 +57,16 @@ class Api::V1::StatusesController < ApiController end def create - @status = PostStatusService.new.call(current_user.account, status_params[:status], status_params[:in_reply_to_id].blank? ? nil : Status.find(status_params[:in_reply_to_id]), media_ids: status_params[:media_ids], - sensitive: status_params[:sensitive], - spoiler_text: status_params[:spoiler_text], - visibility: status_params[:visibility], - application: doorkeeper_token.application) + @status = PostStatusService.new.call(current_user.account, + status_params[:status], + status_params[:in_reply_to_id].blank? ? nil : Status.find(status_params[:in_reply_to_id]), + media_ids: status_params[:media_ids], + sensitive: status_params[:sensitive], + spoiler_text: status_params[:spoiler_text], + visibility: status_params[:visibility], + application: doorkeeper_token.application, + idempotency: request.headers['Idempotency-Key']) + render :show end diff --git a/app/services/post_status_service.rb b/app/services/post_status_service.rb index 6ce434a13b..958cc28900 100644 --- a/app/services/post_status_service.rb +++ b/app/services/post_status_service.rb @@ -11,8 +11,14 @@ class PostStatusService < BaseService # @option [String] :spoiler_text # @option [Enumerable] :media_ids Optional array of media IDs to attach # @option [Doorkeeper::Application] :application + # @option [String] :idempotency Optional idempotency key # @return [Status] def call(account, text, in_reply_to = nil, options = {}) + if options[:idempotency].present? + existing_id = redis.get("idempotency:status:#{account.id}:#{options[:idempotency]}") + return Status.find(existing_id) if existing_id + end + media = validate_media!(options[:media_ids]) status = account.statuses.create!(text: text, thread: in_reply_to, @@ -30,6 +36,10 @@ class PostStatusService < BaseService DistributionWorker.perform_async(status.id) Pubsubhubbub::DistributionWorker.perform_async(status.stream_entry.id) + if options[:idempotency].present? + redis.setex("idempotency:status:#{account.id}:#{options[:idempotency]}", 3_600, status.id) + end + status end @@ -63,4 +73,8 @@ class PostStatusService < BaseService def process_hashtags_service @process_hashtags_service ||= ProcessHashtagsService.new end + + def redis + Redis.current + end end diff --git a/config/locales/simple_form.fa.yml b/config/locales/simple_form.fa.yml index 9ff40a120b..d026a3b25f 100644 --- a/config/locales/simple_form.fa.yml +++ b/config/locales/simple_form.fa.yml @@ -35,7 +35,7 @@ fa: type: نوع درون‌ریزی username: نام کاربری interactions: - must_be_follower: مسدودکردن اعلان‌های همه به جز پیگیران + must_be_follower: مسدودکردن اعلان‌های همه به جز پیگیران must_be_following: مسدودکردن اعلان‌های کسانی که شما پی نمی‌گیرید notification_emails: digest: خلاصه‌کردن چند اعلان در یک ایمیل diff --git a/spec/services/post_status_service_spec.rb b/spec/services/post_status_service_spec.rb index c9d80257ff..57876dcc2b 100644 --- a/spec/services/post_status_service_spec.rb +++ b/spec/services/post_status_service_spec.rb @@ -176,7 +176,14 @@ RSpec.describe PostStatusService do ) end + it 'returns existing status when used twice with idempotency key' do + account = Fabricate(:account) + status1 = subject.call(account, 'test', nil, idempotency: 'meepmeep') + status2 = subject.call(account, 'test', nil, idempotency: 'meepmeep') + expect(status2.id).to eq status1.id + end + def create_status_with_options(options = {}) - subject.call(Fabricate(:account), "test", nil, options) + subject.call(Fabricate(:account), 'test', nil, options) end end