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

[MGPG-81] check format in constructor #11

Merged
merged 1 commit into from Feb 26, 2021
Merged

[MGPG-81] check format in constructor #11

merged 1 commit into from Feb 26, 2021

Conversation

@Syquel
Copy link
Contributor

@Syquel Syquel commented Feb 26, 2021

Provides an refactored implementation of GpgVersion to address MGPG-81.

The parsing logic is being moved from "GpgVersion#compareTo" to "GpgVersion#parse" to throw IllegalArgumentException in a semantically correct way.

Copy link
Contributor

@elharo elharo left a comment

can you split this into individual PRs for each issue? That makes it much easier to review and track in Jira and release notes.

int otherValue = Integer.parseInt( otherSegments[index] );

int compareValue = Integer.compare( thisValue, otherValue );
int compareValue = Integer.compare( thisSegments[index], otherSegments[index] );
Copy link
Contributor

@elharo elharo Feb 26, 2021

Choose a reason for hiding this comment

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

can this be a simple int comparison? e.g.

if (thisSegments[index] != otherSegments[index]) {
  return thisSegments[index] - otherSegments[index]
}

(Double check my order there)

Copy link
Contributor Author

@Syquel Syquel Feb 26, 2021

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 preferable to adhere to the contract of Integer#compareTo / Integer#compare, which is internally implemented as (x < y) ? -1 : ((x == y) ? 0 : 1), because this class is in my opinion just a container for an int array and should behave as such.

But your solution works of course, too. Your call.

Copy link
Contributor

@elharo elharo Feb 26, 2021

Choose a reason for hiding this comment

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

Yes. We should adhere to the contract. The contract is more lenient than the current implementation. It just requires negative, positive, or zero at the appropriate places, not -1, 0, or +1 specifically.

@Syquel
Copy link
Contributor Author

@Syquel Syquel commented Feb 26, 2021

can you split this into individual PRs for each issue? That makes it much easier to review and track in Jira and release notes.

I will reduce this PR to MGPG-81 by removing GpgVersion#equals and GpgVersion#hashCode first and submit another PR for MGPG-80 later on after this one is merged.

@Syquel Syquel changed the title [MGPG-80][MGPG-81] refactor GpgVersion [MGPG-81] refactor GpgVersion Feb 26, 2021
@elharo elharo changed the title [MGPG-81] refactor GpgVersion [MGPG-81] check format in constructor Feb 26, 2021
int otherValue = Integer.parseInt( otherSegments[index] );

int compareValue = Integer.compare( thisValue, otherValue );
int compareValue = Integer.compare( thisSegments[index], otherSegments[index] );
Copy link
Contributor

@elharo elharo Feb 26, 2021

Choose a reason for hiding this comment

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

Yes. We should adhere to the contract. The contract is more lenient than the current implementation. It just requires negative, positive, or zero at the appropriate places, not -1, 0, or +1 specifically.

}

@Override
public int compareTo( GpgVersion other )
public static GpgVersion parse( String rawVersion )
Copy link
Contributor

@elharo elharo Feb 26, 2021

Choose a reason for hiding this comment

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

Where does this version string come from? That is, what is it the version of? Is it ever anything that does not match this pattern? E.g. 3.45.2-beta?

Copy link
Contributor Author

@Syquel Syquel Feb 26, 2021

Choose a reason for hiding this comment

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

As far as I can see the original author of this class made the assumption that it will always be a sequence of numbers delimited by dots (although the original author wrote in the class-level comment, that this is a SemVer parser).
The raw version string is read from the output of gpg --version apparently.

Unfortunately I am in no position to evaluate whether GPG will continue using this versioning scheme, but it seems they have consistently used it up until now: https://files.gpg4win.org/
In my opinion we can make the assumption, that GPG will continue using a three segment wide versioning scheme just as the original author implemented it.

String[] thisSegments;
Matcher m = p.matcher( rawVersion );
if ( m.find() )
final Matcher versionMatcher = VERSION_PATTERN.matcher( rawVersion );
Copy link
Contributor

@elharo elharo Feb 26, 2021

Choose a reason for hiding this comment

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

The regex might be overkill. Looks like you can just catch and convert the number format exception below.

Copy link
Contributor Author

@Syquel Syquel Feb 26, 2021

Choose a reason for hiding this comment

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

The original author implemented it in a way, that the full output of gpg --version (e.g. gpg (GnuPG) 2.2.1 or gpg (GnuPG) 2.0.26 (Gpg4win 2.2.3)) is passed to this function.
They used the regex to extract the version part out of that output.

It is debatable whether that is the best way to extract the version number (personally I would have extracted the version number part already in GpgVersionParser.GpgVersionConsumer before passing it in here), but I would like to avoid introducing new bugs by breaking with the existing external contract of this class.

In the end it's your call again; I could widen the scope of the refactoring to the whole GPG version parser.
In that case we could change the contract to expect a dot-separated sequence of numbers, move the regex to GpgVersionParser.GpgVersionConsumer and simply parse it here via splitting.

// EDIT
IMO widening the scope would break the scope of the ticket. The goal was to refactor this class to use IllegalArgumentException in a proper semantic way by moving the actual parsing from compareTo to the time the object is created.

elharo
elharo approved these changes Feb 26, 2021
@elharo
Copy link
Contributor

@elharo elharo commented Feb 26, 2021

give me a minute to run this through jenkins

@elharo elharo merged commit b38c638 into apache:master Feb 26, 2021
@Syquel
Copy link
Contributor Author

@Syquel Syquel commented Feb 26, 2021

Thanks for merging.

As promised I submitted PR #12 for MGPG-80.

{
this.rawVersion = rawVersion;
}
private static final Pattern VERSION_PATTERN = Pattern.compile( "(\\d+\\.)+(\\d+)" );
Copy link
Contributor

@Tibor17 Tibor17 May 6, 2021

Choose a reason for hiding this comment

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

@Syquel
@elharo
How many times these patterns exist in the project?

Copy link
Contributor Author

@Syquel Syquel May 6, 2021

Choose a reason for hiding this comment

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

There are two patterns AFAIK: One in GpgVersionParser.GpgVersionConsumer and this one here.
This one could possibly be removed and GpgVersionParser.GpgVersionConsumer could pass the extracted version number to GpgVersion.

Copy link
Contributor Author

@Syquel Syquel May 6, 2021

Choose a reason for hiding this comment

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

It might be even better to use gpgconf --query-swdb gnupg instead of gpg --version, because gpgconf returns machine-readable output.

$ gpgconf --query-swdb gnupg
gnupg:2.2.27:-::32849:::::::

although I don't know if that would work with all distributions.

Copy link

@bmarwell bmarwell May 6, 2021

Choose a reason for hiding this comment

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

Works on mac, too:

$ gpgconf --query-swdb gnupg
gnupg:2.2.21:-::32849:::::::

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