Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Post status idempotency - prevent duplicates from network failures #2419

Merged
merged 2 commits into from Apr 25, 2017
Merged

Post status idempotency - prevent duplicates from network failures #2419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08b7cb9
Fix #2402 - Add Idempotency-Key header to PostStatusService that prev…
Gargron Apr 25, 2017
8737a7e
Merge branch 'master' into feature-idempotency
ineffyble Apr 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/assets/javascripts/components/actions/compose.jsx
View file
Open in desktop
Expand Up @@ -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 }));

Expand Down
33 changes: 27 additions & 6 deletions app/assets/javascripts/components/reducers/compose.jsx
View file
Open in desktop
Expand Up @@ -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,
Expand All @@ -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) {
Expand All @@ -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());
});
};

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

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

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

Expand All @@ -135,25 +142,35 @@ 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'));
map.set('text', statusToTextMentions(state, action.status));
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);
Expand All @@ -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);
Expand All @@ -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:
Expand Down
3 changes: 3 additions & 0 deletions app/assets/javascripts/components/uuid.jsx
View file
Open in desktop
@@ -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);
};
15 changes: 10 additions & 5 deletions app/controllers/api/v1/statuses_controller.rb
View file
Open in desktop
Expand Up @@ -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

Expand Down
14 changes: 14 additions & 0 deletions app/services/post_status_service.rb
View file
Open in desktop
Expand Up @@ -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,
Expand All @@ -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

Expand Down Expand Up @@ -63,4 +73,8 @@ def process_mentions_service
def process_hashtags_service
@process_hashtags_service ||= ProcessHashtagsService.new
end

def redis
Redis.current
end
end
2 changes: 1 addition & 1 deletion config/locales/simple_form.fa.yml
View file
Open in desktop
Expand Up @@ -35,7 +35,7 @@ fa:
type: نوع درون‌ریزی
username: نام کاربری
interactions:
must_be_follower: مسدودکردن اعلان‌های همه به جز پیگیران
must_be_follower: مسدودکردن اعلان‌های همه به جز پیگیران
must_be_following: مسدودکردن اعلان‌های کسانی که شما پی نمی‌گیرید
notification_emails:
digest: خلاصه‌کردن چند اعلان در یک ایمیل
Expand Down
9 changes: 8 additions & 1 deletion spec/services/post_status_service_spec.rb
View file
Open in desktop
Expand Up @@ -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