Skip to content

Replace int.Parse() with int.TryParse() to avoid exceptions caused by overflowing values in tags #2703

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

Closed
wants to merge 7 commits into from
Closed

Replace int.Parse() with int.TryParse() to avoid exceptions caused by overflowing values in tags #2703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
329e663
Handle failure to parse integers
james-bjss May 22, 2021
e8c0bf8
fix formatting
james-bjss May 24, 2021
6e4d1b6
use TryParse
james-bjss May 25, 2021
e7628d9
remove stray whitespace
james-bjss May 26, 2021
99ae520
fix formatting
james-bjss Jun 21, 2021
69e2000
rename Parse method to TryParse
james-bjss Jun 21, 2021
2159745
replace Parse with TryParse
james-bjss Jun 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions src/GitVersion.Core/VersionCalculation/SemanticVersioning/SemanticVersion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,10 @@ public static SemanticVersion Parse(string version, string tagPrefixRegex)
public static bool TryParse(string version, string tagPrefixRegex, out SemanticVersion semanticVersion)
{
var match = Regex.Match(version, $"^({tagPrefixRegex})?(?<version>.*)$");
semanticVersion = null;

if (!match.Success)
{
semanticVersion = null;
return false;
}

Expand All @@ -159,23 +159,39 @@ public static bool TryParse(string version, string tagPrefixRegex, out SemanticV

if (!parsed.Success)
{
semanticVersion = null;
return false;
}

var semanticVersionBuildMetaData = SemanticVersionBuildMetaData.Parse(parsed.Groups["BuildMetaData"].Value);
var fourthPart = parsed.Groups["FourthPart"];
if (fourthPart.Success && semanticVersionBuildMetaData.CommitsSinceTag == null)
{
semanticVersionBuildMetaData.CommitsSinceTag = int.Parse(fourthPart.Value);
semanticVersionBuildMetaData.CommitsSinceTag = Int32.Parse(fourthPart.Value);
}

int major = 0, minor = 0, patch = 0;

if (!parsed.Groups["Major"].Success || !Int32.TryParse(parsed.Groups["Major"].Value, out major))
{
return false;
}

if (parsed.Groups["Minor"].Success && !Int32.TryParse(parsed.Groups["Minor"].Value, out minor))
{
return false;
}

if (parsed.Groups["Patch"].Success && !Int32.TryParse(parsed.Groups["Patch"].Value, out patch))
{
return false;
}

semanticVersion = new SemanticVersion
{
Major = int.Parse(parsed.Groups["Major"].Value),
Minor = parsed.Groups["Minor"].Success ? int.Parse(parsed.Groups["Minor"].Value) : 0,
Patch = parsed.Groups["Patch"].Success ? int.Parse(parsed.Groups["Patch"].Value) : 0,
PreReleaseTag = SemanticVersionPreReleaseTag.Parse(parsed.Groups["Tag"].Value),
Major = major,
Minor = minor,
Patch = patch,
PreReleaseTag = SemanticVersionPreReleaseTag.TryParse(parsed.Groups["Tag"].Value),
BuildMetaData = semanticVersionBuildMetaData
};

Expand Down
7 changes: 4 additions & 3 deletions src/GitVersion.Core/VersionCalculation/SemanticVersioning/SemanticVersionPreReleaseTag.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,10 @@ public static implicit operator string(SemanticVersionPreReleaseTag preReleaseTa

public static implicit operator SemanticVersionPreReleaseTag(string preReleaseTag)
{
return Parse(preReleaseTag);
return TryParse(preReleaseTag);
}

public static SemanticVersionPreReleaseTag Parse(string preReleaseTag)
public static SemanticVersionPreReleaseTag TryParse(string preReleaseTag)
Copy link
Member

Choose a reason for hiding this comment

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

Since this method does not have the bool TryParse(string, out T) signature, I'm not sure it should be named TryParse. The method signature indicates Parse while its behaviour indicates TryParse. How much breakage would it cause to change the method signature to bool TryParse(string, out SemanticVersionPreReleaseTag)?

Suggested change
public static SemanticVersionPreReleaseTag TryParse(string preReleaseTag)
public static bool TryParse(string preReleaseTag, out SemanticVersionPreReleaseTag)

Copy link
Author

@james-bjss james-bjss Jun 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The Int.TryParse() line in the SemanticVersionPreReleaseTag defaults the value to null if it's not parseable.

Will let you be the guide here as to whether you would prefer to rename the method it back to Parse OR return false on failure and modify the signature as you have suggested above OR take the same approach as in SemanticVersion.cs

In terms of breakage the only place it seems to be directly called is within SemanticVersion.cs when it constructs and returns a new SemanticVersion and assigns a pre-release tag.

The tests do not seem to directly reference this method directly either.

From there there we can decide if we want to do anything about it eg. returning false from SemanticVersion.TryParse() which would result in the calling method Parse() throwing a new Warning exception.

NB. All of the code calls SemanticVersion.Parse() nothing seems to call the TryParse() directly. I could follow the same pattern in SemanticVersionPreReleaseTag and wrap the method?

Calling Code in TryParse()

            semanticVersion = new SemanticVersion
            {
                Major = major,
                Minor = minor,
                Patch = patch,
                PreReleaseTag = SemanticVersionPreReleaseTag.TryParse(parsed.Groups["Tag"].Value),
                BuildMetaData = semanticVersionBuildMetaData
            };`

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer consistent use of TryParse returning bool everywhere, if possible.

{
if (string.IsNullOrEmpty(preReleaseTag))
{
Expand All @@ -103,7 +103,8 @@ public static SemanticVersionPreReleaseTag Parse(string preReleaseTag)
}

var value = match.Groups["name"].Value;
var number = match.Groups["number"].Success ? int.Parse(match.Groups["number"].Value) : (int?)null;
var number = (match.Groups["number"].Success && int.TryParse(match.Groups["number"].Value, out int parsedNumber)) ? (int?)parsedNumber : null;

if (value.EndsWith("-"))
return new SemanticVersionPreReleaseTag(preReleaseTag, null);

Expand Down