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

Handle cases where a Picture with that id doesn't exist #159

Merged
merged 1 commit into from
Jan 18, 2018
Merged

Handle cases where a Picture with that id doesn't exist #159

merged 1 commit into from
Jan 18, 2018

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Jan 18, 2018 •
edited
Loading

@bdunne bdunne force-pushed the more_pictures_edge_cases branch from b495212 to 6af81e0 Compare January 18, 2018 20:12
@bdunne
Copy link
Member Author

bdunne commented Jan 18, 2018

@miq-bot add_labels blocker bug gaprindashvili/yes

@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2018

@bdunne Cannot apply the following label because they are not recognized: blocker bug gaprindashvili/yes

2 similar comments
@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2018

@bdunne Cannot apply the following label because they are not recognized: blocker bug gaprindashvili/yes

@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2018

@bdunne Cannot apply the following label because they are not recognized: blocker bug gaprindashvili/yes

Fryguy
Fryguy reviewed Jan 18, 2018
View reviewed changes
db/migrate/20180111162442_move_pictures_blobs_to_pictures.rb Outdated
@@ -18,7 +18,7 @@ def up

say_with_time("Moving picture content from BinaryBlobs to the pictures table") do
BinaryBlob.in_my_region.includes(:binary_blob_parts).where(:resource_type => "Picture").find_each do |blob|
Picture.update(blob.resource_id, :content => blob.data, :extension => blob.data_type, :md5 => blob.md5) if blob.resource_id
Picture.find_by(:id => blob.resource_id).try(:update_attributes!, :content => blob.data, :extension => blob.data_type, :md5 => blob.md5)
Copy link
Member

@Fryguy Fryguy Jan 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to do

Picture.where(:id => blob.resource_id).update_all(:content => blob.data, :extension => blob.data_type, :md5 => blob.md5) if blob.resource_id

so you don't have to pull the Picture object and then update (2 queries + in memory object - basically an N+1)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Yep, that's what I was trying to avoid all along. For some reason I didn't think to update_all with a where.

@bdunne bdunne force-pushed the more_pictures_edge_cases branch from 6af81e0 to a7caf2f Compare January 18, 2018 21:04
@bdunne bdunne force-pushed the more_pictures_edge_cases branch from a7caf2f to 447c8a4 Compare January 18, 2018 21:10
@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2018

Checked commit bdunne@447c8a4 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

db/migrate/20180111162442_move_pictures_blobs_to_pictures.rb

@Fryguy Fryguy merged commit 40c5ced into ManageIQ:master Jan 18, 2018
@Fryguy Fryguy added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 18, 2018
simaishi pushed a commit that referenced this pull request Jan 18, 2018
@Fryguy @simaishi
Merge pull request #159 from bdunne/more_pictures_edge_cases
0e711b5 
Handle cases where a Picture with that id doesn't exist
(cherry picked from commit 40c5ced)

https://bugzilla.redhat.com/show_bug.cgi?id=1536046
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 0e711b563fe28068a34cc893c48ac81f5240b6c6
Author: Jason Frey <[email protected]>
Date:   Thu Jan 18 16:37:55 2018 -0500

    Merge pull request #159 from bdunne/more_pictures_edge_cases
    
    Handle cases where a Picture with that id doesn't exist
    (cherry picked from commit 40c5ced8ebbbdf72c86759b8d504c673076244b9)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1536046

@bdunne bdunne deleted the more_pictures_edge_cases branch January 19, 2018 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy left review comments

Assignees

@Fryguy Fryguy

Labels
blocker bug gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bdunne @miq-bot @simaishi @Fryguy @chessbyte