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

Added missing length check that caused an ArgumentNullException (#750) #753

Merged
merged 4 commits into from
Oct 27, 2018

Conversation

dlemstra
Copy link
Member

@dlemstra dlemstra commented Oct 26, 2018

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

This patch adds a missing null check and also an extra unit test that checks if the extension method works. This fixes #750.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

LGTM

@dlemstra
Copy link
Member Author

Looks like 2.1 returns an empty string instead.

@antonfirsov
Copy link
Member

I have a feeling that when we fix such bugs, we always have to add end-to-end regression tests with a test image, even if a unit test has been added based on the root cause.

@JimBobSquarePants @dlemstra am I overthinking?

@JimBobSquarePants
Copy link
Member

A would add the image from #750 in case we change the underlying implementation and introduce a regressive bug.

@codecov
Copy link

codecov bot commented Oct 27, 2018

Codecov Report

Merging #753 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #753      +/-   ##
=========================================
+ Coverage   88.39%   88.4%   +<.01%     
=========================================
  Files         996     997       +1     
  Lines       42314   42325      +11     
  Branches     3142    3142              
=========================================
+ Hits        37405   37416      +11     
  Misses       4225    4225              
  Partials      684     684
Impacted Files Coverage Δ
tests/ImageSharp.Tests/TestImages.cs 100% <ø> (ø) ⬆️
...Sharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs 100% <100%> (ø) ⬆️
.../ImageSharp.Tests/Common/EncoderExtensionsTests.cs 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc4da81...bf91740. Read the comment docs.

@@ -160,6 +160,8 @@ public static class Issues
public const string OrderedInterleavedProgressive723A = "Jpg/issues/Issue723-Ordered-Interleaved-Progressive-A.jpg";
public const string OrderedInterleavedProgressive723B = "Jpg/issues/Issue723-Ordered-Interleaved-Progressive-B.jpg";
public const string OrderedInterleavedProgressive723C = "Jpg/issues/Issue723-Ordered-Interleaved-Progressive-C.jpg";
public const string ExifGetString750Transform = "Jpg/issues/issue750-exif-tranform.jpg";
Copy link
Member Author

Choose a reason for hiding this comment

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

Small typo here. Thanks for adding these images.

Copy link
Member

Choose a reason for hiding this comment

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

No worries, I'll update the reference file name later on, will do for now.

@JimBobSquarePants JimBobSquarePants merged commit e508683 into master Oct 27, 2018
@JimBobSquarePants JimBobSquarePants deleted the dl/fix-750 branch October 27, 2018 13:37
antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019
Added missing length check that caused an ArgumentNullException (SixLabors#750)
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.

ImageProcessingException: An error occurred when processing the image using CropProcessor
3 participants