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

[james-bjss]

Replace int.Parse() with int.TryParse() This is to prevent the tool failing with an overflowing value.

Description

If a tag has a suffix which is a numeric value larger than a 32bit int then gitversion fails with an Overflow error.
Whilst the tag can be ignore by using the tag-prefix config It would be preferable if it didn't fall over when it encounters such a tag.

Related Issue

#2390

Motivation and Context

We have some tags in our repository which are suffixed with a timestamp value (created by another build process).
We have used the tag-prefix config to ignore these, but without it gitversion falls over with an Int32 Overflow exception.

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

• [X ] My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes. ( Will add if this change is deemed acceptable)
• [X ] All new and existing tests passed.

[asbjornu]

Great stuff, I just have some design feedback: Please add the method SemanticVersion.TryParse() instead to deal with this and then use Int32.TryParse() within it to avoid the exceptions.

[james-bjss]

Great stuff, I just have some design feedback: Please add the method SemanticVersion.TryParse() instead to deal with this and then use Int32.TryParse() within it to avoid the exceptions.

Sure, will update this evening and add tests. Thanks!

Should we swallow the errors silently or would you like it to print to STDERR?

[asbjornu]

Sure, will update this evening and add tests. Thanks!

👍🏼

Should we swallow the errors silently or would you like it to print to STDERR?

If you're able to inject an instance of ILog somewhere natural, you can always do a log.Error() statement when parsing fails. For more verbosity you can add log.Warning()´ or log.Debug()` statements as well, perhaps on the individual field version part level.

[james-bjss]

Updated to use TryParse(), the code is already within a method called SemanticVersion.TryParse() unless you meant to create a private method for this?

Defaulted the out param to null, the code returns if major version is not matched (there was no default 0 value set for this previously) or if the int.TryParse()s fail.

I looked into the logging but could't see a decent way to provide a logger. I'm wondering how useful it is now to check each field if we can't provide details of the failure to the user?

I noticed the SemanticVersion.Parse() method wraps SemanticVersion.TryParse() and this throws a WarningException. Perhaps it could bubble-up/throw an exception with information about the parsing failure?

   public static SemanticVersion Parse(string version, string tagPrefixRegex)
        {
            if (!TryParse(version, tagPrefixRegex, out var semanticVersion))
                throw new WarningException($"Failed to parse {version} into a Semantic Version");

            return semanticVersion;
        }

[asbjornu]

Updated to use TryParse()

👍🏼

the code is already within a method called SemanticVersion.TryParse()

Sorry, I did not explore the surrounding code and context fully.

unless you meant to create a private method for this?

No, this is perfect.

Defaulted the out param to null, the code returns if major version is not matched (there was no default 0 value set for this previously) or if the int.TryParse()s fail.

Sounds reasonable to me.

I looked into the logging but could't see a decent way to provide a logger. I'm wondering how useful it is now to check each field if we can't provide details of the failure to the user?

If it's difficult it's not worth it. If possible, we can log on the outside (when SemanticVersion.TryParse returns false) instead.

I noticed the SemanticVersion.Parse() method wraps SemanticVersion.TryParse() and this throws a WarningException. Perhaps it could bubble-up/throw an exception with information about the parsing failure?

I'm not sure why we would need the Parse method. Our own codebase should be confidently and consistently using TryParse everywhere, imho.

   public static SemanticVersion Parse(string version, string tagPrefixRegex)
        {
            if (!TryParse(version, tagPrefixRegex, out var semanticVersion))
                throw new WarningException($"Failed to parse {version} into a Semantic Version");

            return semanticVersion;
        }

This works if we actually want exceptions, but if this again is just caught and written to ILog, it would be better to just use TryParse and log if it returns false.

[arturcic]

@james-bjss any updates?

[james-bjss]

@james-bjss any updates?

Sorry, have been away on leave and busy with day job. Will try and take a look at this again this week.

[james-bjss]

@asbjornu @arturcic - Have updated the PR with the requested changes.

The WriteVersionInfoTaskShouldLogOutputVariablesToBuildOutputInAzurePipeline is failing though, I noticed these build pipeline tests seem intermittent and seem to fail on other PRs. Not 100% sure this is failing due to my change as previous builds this passed and the GitHub Actions test failed.

Is this a flaky test or do I need to dig deeper?

[arturcic]

@asbjornu @arturcic - Have updated the PR with the requested changes.

The WriteVersionInfoTaskShouldLogOutputVariablesToBuildOutputInAzurePipeline is failing though, I noticed these build pipeline tests seem intermittent and seem to fail on other PRs. Not 100% sure this is failing due to my change as previous builds this passed and the GitHub Actions test failed.

Is this a flaky test or do I need to dig deeper?

It's a flaky test unfortunately.

[asbjornu]

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)

[james-bjss]

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
            };`

[asbjornu]

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

[stale]

This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions.

[asbjornu]

@james-bjss, could you please go over my feedback and rebase this onto the HEAD of main? 🙏🏼

[asbjornu]

While I think I've solved this by changing int to long in #3028, I like the changes made in this PR and would love to see them rebased and merged.

[james-bjss]

While I think I've solved this by changing int to long in #3028, I like the changes made in this PR and would love to see them rebased and merged.

Hi, Sorry for the late reply. I have had quite a lot on work-wise. Will try to grab some time to revisit this and put this PR to bed.

Just to confirm the outstanding change is to replace all usages with TryParse()?

I suppose that then means adding logic in all the calling code to check the repose and throw an error?

[asbjornu]

I believe a bool TryParse() pattern is better, but I'm not 100% certain it's needed anymore after changing from int to long in #3028. Please have a look at the code and let me know whether you think a TryParse pattern is going to provide an improvement or not still. If it is, I'd love to see this PR rebased and completed.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions.

[asbjornu]

@james-bjss, can you please complete this PR one way or another? 🙏🏼

[arturcic]

This one is not needed anymore due to recent changes in the main branch