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.

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

Jump to bottom

TRUNK-5480: Make ModuleUtil support more version specification format #4611

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

TRUNK-5480: Make ModuleUtil support more version specification format #4611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 23 additions & 11 deletions api/src/main/java/org/openmrs/module/ModuleUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,11 +242,15 @@ public static boolean isOpenmrsVersionInVersions(String ...versions) {
* <li>1.2.*</li>
* <li>1.2.2 - 1.2.3</li>
* <li>1.2.* - 1.3.*</li>
* <li>1.4+ - 1.8.3</li>
* <li>1.4+ - 1.8.3+</li>
* <li>1.4.2 - 1.8.3+</li>
* </ul>
* <p>
* Again the possible require version number formats with their interpretation:
* <ul>
* <li>1.2.3 means 1.2.3 and above</li>
* <li>1.4+ means 1.4.0 and above. Supported only for versions 2.7+</li>
* <li>1.2.* means any version of the 1.2.x branch. That is 1.2.0, 1.2.1, 1.2.2,... but not 1.3.0, 1.4.0</li>
* <li>1.2.2 - 1.2.3 means 1.2.2 and 1.2.3 (inclusive)</li>
* <li>1.2.* - 1.3.* means any version of the 1.2.x and 1.3.x branch</li>
Expand Down Expand Up @@ -287,8 +291,8 @@ public static boolean matchRequiredVersions(String version, String versionRange)
for (String range : ranges) {
// need to externalize this string
String separator = "-";
if (range.indexOf("*") > 0 || range.indexOf(separator) > 0 && (!isVersionWithQualifier(range))) {
// if it contains "*" or "-" then we must separate those two
if (range.indexOf("*") > 0 || range.indexOf("+") > 0 || range.indexOf(separator) > 0 && (!isVersionWithQualifier(range))) {
// if it contains "*" or "-" or "+" then we must separate those two
// assume it's always going to be two part
// assign the upper and lower bound
// if there's no "-" to split lower and upper bound
Expand All @@ -309,17 +313,22 @@ public static boolean matchRequiredVersions(String version, String versionRange)
// only preserve part of the string that match the following format:
// - xx.yy.*
// - xx.yy.zz*
lowerBound = StringUtils.remove(lowerBound, lowerBound.replaceAll("^\\s?\\d+[\\.\\d+\\*?|\\.\\*]+", ""));
upperBound = StringUtils.remove(upperBound, upperBound.replaceAll("^\\s?\\d+[\\.\\d+\\*?|\\.\\*]+", ""));
// - aa.bb+
// - aa.b+
lowerBound = StringUtils.remove(lowerBound, lowerBound.replaceAll("^\\s?\\d+[\\.\\d+\\*?|\\.\\*|\\.\\d+\\+?|\\.\\+]+", ""));
upperBound = StringUtils.remove(upperBound, upperBound.replaceAll("^\\s?\\d+[\\.\\d+\\*?|\\.\\*|\\.\\d+\\+?|\\.\\+]+", ""));

// if the lower contains "*" then change it to zero
// if the lower contains "*" or "+" then change it to zero
if (lowerBound.indexOf("*") > 0) {
lowerBound = lowerBound.replaceAll("\\*", "0");
}else if (lowerBound.indexOf("+") > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}else if (lowerBound.indexOf("+") > 0) {
} else if (lowerBound.indexOf("+") > 0) {

lowerBound = lowerBound.replaceAll("\\+", ".0");
}

// if the upper contains "*" then change it to maxRevisionNumber
// if the upper contains "*" or "+" then change it to maxRevisionNumber
String maxRevisionNumber = Integer.toString(Integer.MAX_VALUE);
if (upperBound.indexOf("*") > 0) {
upperBound = upperBound.replaceAll("\\*", Integer.toString(Integer.MAX_VALUE));
upperBound = upperBound.replaceAll("\\*", maxRevisionNumber);
}

int lowerReturn = compareVersion(version, lowerBound);
Expand Down Expand Up @@ -597,15 +606,18 @@ public static void expandJar(File fileToExpand, File tmpModuleDir, String name,
* @return File the file created by the expansion.
* @throws IOException if an error occurred while copying
*/
private static File expand(InputStream input, String fileDir, String name) throws IOException {
private static void expand(InputStream input, String fileDir, String name) throws IOException {
log.debug("expanding: {}", name);

File file = new File(fileDir, name);

if (!file.toPath().normalize().startsWith(fileDir)) {
throw new UnsupportedOperationException("Attempted to write file '" + name + "' rejected as it attempts to write outside the chosen directory. This may be the result of a zip-slip style attack.");
}

try (FileOutputStream outStream = new FileOutputStream(file)) {
OpenmrsUtil.copyFile(input, outStream);
}

return file;
}

/**
Expand Down
13 changes: 13 additions & 0 deletions api/src/test/java/org/openmrs/module/ModuleUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,4 +773,17 @@ protected File getEmptyJarDestinationFolder() throws IOException {
}
return destinationFolder;
}

@Test
public void matchRequiredVersions_shouldAllowRangedUsingPlusSignVersion() {
Copy link
Member

@ibacher ibacher Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Couple of thoughts here:

  1. We want to test the simple cases, i.e. 1.4+, etc..
  2. Did you see my comment on the ticket here, because I'm not sure that 2.2.5+ - 3.19.2+ is clear.
  3. I think the easiest way to get the requested behavior without a lot of changes is to only support the forms: 1.4+, 1+, and 1.4.2+, i.e., don't support the + without the separator (-). (This would make it similar to, e.g., Node and Rust's .X suffix).
  4. The comments on the + format should mention that it's only available on platform 2.7+.
  5. It would be good to have some checks on the boundaries, i.e., 1.4+ should be strictly equivalent to 1.4.0 - 1.4.*, 1.4.2+ should be equivalent to 1.4.2 - 1.4.*, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the last recommendation, I hope it doesn't require the complexity that we are avoiding to check all this scenarios.
Therefore, I agree with the recommendations and explanations!
I will apply the changes and reach out to you for any concerns.

assertTrue(ModuleUtil.matchRequiredVersions("1.4.3","1+ - 1.8.3"));
assertTrue(ModuleUtil.matchRequiredVersions("1.4.3","1.4+ - 1.8.3"));
assertTrue(ModuleUtil.matchRequiredVersions("1.3.3","1.3.2+ - 1.5.2"));
}

@Test
public void matchRequiredVersions_shouldRefuseRangedOutOfBoundUsingPlusSignVersion() {
assertFalse(ModuleUtil.matchRequiredVersions("2.0.4","2.2.5+ - 3.19.*"));
assertFalse(ModuleUtil.matchRequiredVersions("5.0.0","2.2.5+ - 3.19.2"));
}
}