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
This commit is contained in:
Eugen Rochko 2017-04-25 15:04:49 +02:00 committed by GitHub
parent 3ea5b948a4
commit 8b5179d006
7 changed files with 67 additions and 13 deletions

View File

@ -85,6 +85,10 @@ export function submitCompose() {
sensitive: getState().getIn(['compose', 'sensitive']), sensitive: getState().getIn(['compose', 'sensitive']),
spoiler_text: getState().getIn(['compose', 'spoiler_text'], ''), spoiler_text: getState().getIn(['compose', 'spoiler_text'], ''),
visibility: getState().getIn(['compose', 'privacy']) visibility: getState().getIn(['compose', 'privacy'])
}, {
headers: {
'Idempotency-Key': getState().getIn(['compose', 'idempotencyKey'])
}
}).then(function (response) { }).then(function (response) {
dispatch(submitComposeSuccess({ ...response.data })); dispatch(submitComposeSuccess({ ...response.data }));

View File

@ -26,6 +26,7 @@ import {
import { TIMELINE_DELETE } from '../actions/timelines'; import { TIMELINE_DELETE } from '../actions/timelines';
import { STORE_HYDRATE } from '../actions/store'; import { STORE_HYDRATE } from '../actions/store';
import Immutable from 'immutable'; import Immutable from 'immutable';
import uuid from '../uuid';
const initialState = Immutable.Map({ const initialState = Immutable.Map({
mounted: false, mounted: false,
@ -45,7 +46,8 @@ const initialState = Immutable.Map({
suggestions: Immutable.List(), suggestions: Immutable.List(),
me: null, me: null,
default_privacy: 'public', default_privacy: 'public',
resetFileKey: Math.floor((Math.random() * 0x10000)) resetFileKey: Math.floor((Math.random() * 0x10000)),
idempotencyKey: null
}); });
function statusToTextMentions(state, status) { function statusToTextMentions(state, status) {
@ -69,6 +71,7 @@ function clearAll(state) {
map.set('privacy', state.get('default_privacy')); map.set('privacy', state.get('default_privacy'));
map.set('sensitive', false); map.set('sensitive', false);
map.update('media_attachments', list => list.clear()); 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.set('resetFileKey', Math.floor((Math.random() * 0x10000)));
map.update('text', oldText => `${oldText.trim()} ${media.get('text_url')}`); map.update('text', oldText => `${oldText.trim()} ${media.get('text_url')}`);
map.set('focusDate', new Date()); map.set('focusDate', new Date());
map.set('idempotencyKey', uuid());
}); });
}; };
@ -89,6 +93,7 @@ function removeMedia(state, mediaId) {
return state.withMutations(map => { return state.withMutations(map => {
map.update('media_attachments', list => list.filterNot(item => item.get('id') === mediaId)); map.update('media_attachments', list => list.filterNot(item => item.get('id') === mediaId));
map.update('text', text => text.replace(media.get('text_url'), '').trim()); map.update('text', text => text.replace(media.get('text_url'), '').trim());
map.set('idempotencyKey', uuid());
if (prevSize === 1) { if (prevSize === 1) {
map.set('sensitive', false); map.set('sensitive', false);
@ -102,6 +107,7 @@ const insertSuggestion = (state, position, token, completion) => {
map.set('suggestion_token', null); map.set('suggestion_token', null);
map.update('suggestions', Immutable.List(), list => list.clear()); map.update('suggestions', Immutable.List(), list => list.clear());
map.set('focusDate', new Date()); map.set('focusDate', new Date());
map.set('idempotencyKey', uuid());
}); });
}; };
@ -111,6 +117,7 @@ const insertEmoji = (state, position, emojiData) => {
return state.withMutations(map => { return state.withMutations(map => {
map.update('text', oldText => `${oldText.slice(0, position)}${emoji} ${oldText.slice(position)}`); map.update('text', oldText => `${oldText.slice(0, position)}${emoji} ${oldText.slice(position)}`);
map.set('focusDate', new Date()); map.set('focusDate', new Date());
map.set('idempotencyKey', uuid());
}); });
}; };
@ -135,18 +142,27 @@ export default function compose(state = initialState, action) {
case COMPOSE_UNMOUNT: case COMPOSE_UNMOUNT:
return state.set('mounted', false); return state.set('mounted', false);
case COMPOSE_SENSITIVITY_CHANGE: 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: case COMPOSE_SPOILERNESS_CHANGE:
return state.withMutations(map => { return state.withMutations(map => {
map.set('spoiler_text', ''); map.set('spoiler_text', '');
map.set('spoiler', !state.get('spoiler')); map.set('spoiler', !state.get('spoiler'));
map.set('idempotencyKey', uuid());
}); });
case COMPOSE_SPOILER_TEXT_CHANGE: 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: case COMPOSE_VISIBILITY_CHANGE:
return state.set('privacy', action.value); return state
.set('privacy', action.value)
.set('idempotencyKey', uuid());
case COMPOSE_CHANGE: case COMPOSE_CHANGE:
return state.set('text', action.text); return state
.set('text', action.text)
.set('idempotencyKey', uuid());
case COMPOSE_REPLY: case COMPOSE_REPLY:
return state.withMutations(map => { return state.withMutations(map => {
map.set('in_reply_to', action.status.get('id')); 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('privacy', privacyPreference(action.status.get('visibility'), state.get('default_privacy')));
map.set('focusDate', new Date()); map.set('focusDate', new Date());
map.set('preselectDate', new Date()); map.set('preselectDate', new Date());
map.set('idempotencyKey', uuid());
if (action.status.get('spoiler_text').length > 0) { if (action.status.get('spoiler_text').length > 0) {
map.set('spoiler', true); map.set('spoiler', true);
@ -170,6 +187,7 @@ export default function compose(state = initialState, action) {
map.set('spoiler', false); map.set('spoiler', false);
map.set('spoiler_text', ''); map.set('spoiler_text', '');
map.set('privacy', state.get('default_privacy')); map.set('privacy', state.get('default_privacy'));
map.set('idempotencyKey', uuid());
}); });
case COMPOSE_SUBMIT_REQUEST: case COMPOSE_SUBMIT_REQUEST:
return state.set('is_submitting', true); return state.set('is_submitting', true);
@ -190,7 +208,10 @@ export default function compose(state = initialState, action) {
case COMPOSE_UPLOAD_PROGRESS: case COMPOSE_UPLOAD_PROGRESS:
return state.set('progress', Math.round((action.loaded / action.total) * 100)); return state.set('progress', Math.round((action.loaded / action.total) * 100));
case COMPOSE_MENTION: 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: case COMPOSE_SUGGESTIONS_CLEAR:
return state.update('suggestions', Immutable.List(), list => list.clear()).set('suggestion_token', null); return state.update('suggestions', Immutable.List(), list => list.clear()).set('suggestion_token', null);
case COMPOSE_SUGGESTIONS_READY: case COMPOSE_SUGGESTIONS_READY:

View File

@ -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);
};

View File

@ -57,11 +57,16 @@ class Api::V1::StatusesController < ApiController
end end
def create 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], @status = PostStatusService.new.call(current_user.account,
sensitive: status_params[:sensitive], status_params[:status],
spoiler_text: status_params[:spoiler_text], status_params[:in_reply_to_id].blank? ? nil : Status.find(status_params[:in_reply_to_id]),
visibility: status_params[:visibility], media_ids: status_params[:media_ids],
application: doorkeeper_token.application) 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 render :show
end end

View File

@ -11,8 +11,14 @@ class PostStatusService < BaseService
# @option [String] :spoiler_text # @option [String] :spoiler_text
# @option [Enumerable] :media_ids Optional array of media IDs to attach # @option [Enumerable] :media_ids Optional array of media IDs to attach
# @option [Doorkeeper::Application] :application # @option [Doorkeeper::Application] :application
# @option [String] :idempotency Optional idempotency key
# @return [Status] # @return [Status]
def call(account, text, in_reply_to = nil, options = {}) 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]) media = validate_media!(options[:media_ids])
status = account.statuses.create!(text: text, status = account.statuses.create!(text: text,
thread: in_reply_to, thread: in_reply_to,
@ -30,6 +36,10 @@ class PostStatusService < BaseService
DistributionWorker.perform_async(status.id) DistributionWorker.perform_async(status.id)
Pubsubhubbub::DistributionWorker.perform_async(status.stream_entry.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 status
end end
@ -63,4 +73,8 @@ class PostStatusService < BaseService
def process_hashtags_service def process_hashtags_service
@process_hashtags_service ||= ProcessHashtagsService.new @process_hashtags_service ||= ProcessHashtagsService.new
end end
def redis
Redis.current
end
end end

View File

@ -176,7 +176,14 @@ RSpec.describe PostStatusService do
) )
end 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 = {}) def create_status_with_options(options = {})
subject.call(Fabricate(:account), "test", nil, options) subject.call(Fabricate(:account), 'test', nil, options)
end end
end end