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.

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

Check the last created subset for matching compressed report #191

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

richardhallett
Copy link
Contributor

This is because if multiple requests are made with a compressed report you will end up with multiple subsets but the last saved attachment will have the checksum of the last subset stored.

I think currently it's breaking when working with compressed reports that don't need the subset functionality.

Approach

Looking at last subset checksum.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

  • New feature (non-breaking change which adds functionality)

  • Breaking change (fix or feature that would cause existing functionality to change)

Reviewer, please remember our guidelines:

  • Be humble in the language and feedback you give, ask don't tell.
  • Consider using positive language as opposed to neutral when offering feedback. This is to avoid the negative bias that can occur with neutral language appearing negative.
  • Offer suggestions on how to improve code e.g. simplification or expanding clarity.
  • Ensure you give reasons for the changes you are proposing.

This is because if multiple requests are made with a compressed report you will end up with multiple subsets but the last saved attachment will have the checksum of the last subset stored.
@@ -185,7 +190,7 @@ def load_attachment!

# set report.compressed from the attachment.
if compressed_report? || resolution_report?
report_subset = report_subsets.order("created_at ASC").first
report_subset = report_subsets.order("created_at DESC").first
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming this is the only behavioral change, then I think this makes intuitive sense to me.

For report ab28a47e-4b66-4292-a81d-e48e8f880bb9:

Checksum in S3: de0cb0691a3d6f3019e0472ee255b013d9f70a6d0627a67bbdbbe04c3daedb19
Checksums in sashimi:

3.1.0 :001 > r = Report.where(uid:"ab28a47e-4b66-4292-a81d-e48e8f880bb9").first

 => 

#<Report:0x00007f02f5d8acd0

... 
3.1.0 :008 > r.report_subsets.each do |rs|

3.1.0 :009 >   pp rs.checksum

3.1.0 :010 > end

"de0cb0691a3d6f3019e0472ee255b013d9f70a6d0627a67bbdbbe04c3daedb19"

"e58407e2953d055e38b5611f15b55bc790698db5ccc2a0290798a819bc76c236"

"c178b302646ee2c8b77356e3dcb0b1563047bceb696fd81014383f2662c4e2ab"

"c078e954d5042ab7bd39a65d352e31e855a99bc01a0c7908c971eb4af4ee1263"

With ASC (non-matching checksum):

3.1.0 :012 > r.report_subsets.order("created_at ASC").first.checksum
 => "e58407e2953d055e38b5611f15b55bc790698db5ccc2a0290798a819bc76c236" 

With DESC (matching checksum):

3.1.0 :011 > r.report_subsets.order("created_at DESC").first.checksum
 => "de0cb0691a3d6f3019e0472ee255b013d9f70a6d0627a67bbdbbe04c3daedb19"

true
end

def compressed_report?
return nil if self.exceptions.nil? || self.exceptions.empty?
return nil if exceptions.nil? || exceptions.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

you can achieve this check by using the Rails blank? method.
return nil if exceptions.nil? || exceptions.empty?
becomes
return nil if exceptions.blank?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Auto format changed whitespace. I did not change that code.

The only thing I;ve changed is ASC to DESC

I don't want to change anything else in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

@richardhallett understood

@richardhallett richardhallett merged commit 976463a into master Oct 24, 2024
2 checks passed
@richardhallett richardhallett deleted the check_last_checksum branch October 24, 2024 12:23
Copy link

sentry-io bot commented Oct 24, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ RuntimeError: [UsageReports] Cannot find report-subset gzip field. ReportsController#show View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants