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.

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

Make changes to a copy of the options parameter #2454

Merged
merged 2 commits into from Dec 5, 2013

Conversation

Projects
None yet
3 participants
@joeyates
Contributor

joeyates commented Dec 5, 2013

If the method alters the options hash, clients calling this method in a loop
will unwittingly be sending a different hash on the second and subsequent calls.

Alteration of the options hash is causing failure in the backup gem, in cases where there are more than 1000 objects to delete. See backup/backup#510

About the test: as this is behaviour which is tested in the Mock version of the method, I have duplicated code between the Real and Mock versions.

Make changes to a copy of the options parameter
If the method alters the options hash, clients calling this method in a loop
will unwittingly be sending a different hash on the second and subsequent calls.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 5, 2013

Coverage Status

Coverage remained the same when pulling 3b97a23 on joeyates:dont_alter_options_parameter into 1251e89 on fog:master.

coveralls commented Dec 5, 2013

Coverage Status

Coverage remained the same when pulling 3b97a23 on joeyates:dont_alter_options_parameter into 1251e89 on fog:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 5, 2013

Coverage Status

Coverage remained the same when pulling 19ef48a on joeyates:dont_alter_options_parameter into 1251e89 on fog:master.

coveralls commented Dec 5, 2013

Coverage Status

Coverage remained the same when pulling 19ef48a on joeyates:dont_alter_options_parameter into 1251e89 on fog:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 5, 2013

Coverage Status

Coverage remained the same when pulling 19ef48a on joeyates:dont_alter_options_parameter into 1251e89 on fog:master.

coveralls commented Dec 5, 2013

Coverage Status

Coverage remained the same when pulling 19ef48a on joeyates:dont_alter_options_parameter into 1251e89 on fog:master.

@geemus

This comment has been minimized.

Show comment
Hide comment
@geemus

geemus Dec 5, 2013

Member

Looks good, thanks!

Member

geemus commented Dec 5, 2013

Looks good, thanks!

geemus added a commit that referenced this pull request Dec 5, 2013

Merge pull request #2454 from joeyates/dont_alter_options_parameter
Make changes to a copy of the options parameter

@geemus geemus merged commit fb53607 into fog:master Dec 5, 2013

1 check failed

default The Travis CI build failed
Details

@joeyates joeyates deleted the joeyates:dont_alter_options_parameter branch Dec 5, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment