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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
93 changes: 38 additions & 55 deletions src/main/java/org/apache/maven/plugins/gpg/GpgVersion.java
Expand Up @@ -32,54 +32,46 @@
*/
public class GpgVersion implements Comparable<GpgVersion>
{
private final String rawVersion;

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

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

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

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

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:::::::


public static GpgVersion parse( String rawVersion )
private final int[] versionSegments;

private GpgVersion( int... versionSegments )
{
return new GpgVersion( rawVersion );
this.versionSegments = versionSegments;
}

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

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

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.

{
Pattern p = Pattern.compile( "(\\d+\\.)+(\\d+)" );

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

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.

if ( !versionMatcher.find() )
{
thisSegments = m.group( 0 ).split( "\\." );
}
else
{
throw new IllegalArgumentException( "Can't parse version of " + this.rawVersion );
throw new IllegalArgumentException( "Can't parse version of " + rawVersion );
}

String[] otherSegments;
m = p.matcher( other.rawVersion );
if ( m.find() )
{
otherSegments = m.group( 0 ).split( "\\." );
}
else
final String[] rawVersionSegments = versionMatcher.group( 0 ).split( "\\." );

final int[] versionSegments = new int[rawVersionSegments.length];
for ( int index = 0; index < rawVersionSegments.length; index++ )
{
throw new IllegalArgumentException( "Can't parse version of " + other.rawVersion );
versionSegments[index] = Integer.parseInt( rawVersionSegments[index] );
}

return new GpgVersion( versionSegments );
}

@Override
public int compareTo( GpgVersion other )
{
final int[] thisSegments = versionSegments;
final int[] otherSegments = other.versionSegments;

int minSegments = Math.min( thisSegments.length, otherSegments.length );

for ( int index = 0; index < minSegments; index++ )
{
int thisValue = Integer.parseInt( thisSegments[index] );

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

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

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

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

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.


if ( compareValue != 0 )
{
Expand All @@ -101,17 +93,6 @@ public boolean isBefore( GpgVersion other )
return this.compareTo( other ) < 0;
}

/**
* Verify if this version is before some other version
*
* @param other the version to compare with
* @return {@code true} is this is less than {@code other}, otherwise {@code false}
*/
public boolean isBefore( String other )
{
return this.compareTo( parse( other ) ) < 0;
}

/**
* Verify if this version is at least some other version
*
Expand All @@ -123,21 +104,23 @@ public boolean isAtLeast( GpgVersion other )
return this.compareTo( other ) >= 0;
}

/**
* Verify if this version is at least some other version
*
* @param other the version to compare with
* @return {@code true} is this is greater than or equal to {@code other}, otherwise {@code false}
*/
public boolean isAtLeast( String other )
{
return this.compareTo( parse( other ) ) >= 0;
}

@Override
public String toString()
{
return rawVersion;
if ( versionSegments.length == 0 )
{
return "";
}

final StringBuilder versionStringBuilder = new StringBuilder();
versionStringBuilder.append( versionSegments[0] );

for ( int index = 1; index < versionSegments.length; index++ )
{
versionStringBuilder.append( '.' ).append( versionSegments[index] );
}

return versionStringBuilder.toString();
}

}
Expand Up @@ -34,7 +34,7 @@ public void test()
GpgVersionConsumer consumer = new GpgVersionConsumer();
consumer.consumeLine( "gpg (GnuPG/MacGPG2) 2.2.10" );

assertThat( consumer.getGpgVersion().toString(), is( "gpg (GnuPG/MacGPG2) 2.2.10" ) );
assertThat( consumer.getGpgVersion().toString(), is( GpgVersion.parse( "2.2.10" ).toString() ) );
}

}