From 08b7cb9685d33f958e458aa9d568eec20a4b4a1e Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 25 Apr 2017 02:39:46 +0200 Subject: [PATCH] Fix #2402 - Add Idempotency-Key header to PostStatusService that prevents 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 | 4 +-- spec/services/post_status_service_spec.rb | 9 ++++- 7 files changed, 68 insertions(+), 14 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 de75ddabe8e5c..d7ff6ea63f859 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 a411feedd1fa0..c873847804061 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 0000000000000..be18993057b73 --- /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 5a2e18cc0570d..77bdaa49436d4 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -57,11 +57,16 @@ def favourited_by 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 6ce434a13bd12..958cc289008d7 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 @@ def call(account, text, in_reply_to = nil, options = {}) 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 @@ def process_mentions_service 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 b0af0575af13d..d026a3b25fa55 100644 --- a/config/locales/simple_form.fa.yml +++ b/config/locales/simple_form.fa.yml @@ -1,5 +1,5 @@ --- -en: +fa: simple_form: hints: defaults: @@ -35,7 +35,7 @@ en: 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 c9d80257ff6e6..57876dcc2bf78 100644 --- a/spec/services/post_status_service_spec.rb +++ b/spec/services/post_status_service_spec.rb @@ -176,7 +176,14 @@ ) 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