Skip to content

Commit 946454b

Browse files
pditommasoclaudejorgee
authored
Fix trailing slash, response leak, NPE, and minor issues in module system (#6930)
* Fix additional code issues in module system Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com> * Use shared RegistryClient from npr-client for module commands (#6931) * Use shared RegistryClient from npr-client for module commands Replace ModuleRegistryClient with RegistryClient from the new io.seqera:npr-client library. This moves the registry HTTP client and checksum algorithm to a shared library used by both Nextflow and the plugin-registry server. Changes: - ModuleChecksum now delegates to io.seqera.npr.client.ModuleChecksum - ModuleRegistryClient removed, replaced by RegistryClientFactory that creates RegistryClient instances from RegistryConfig - All CLI commands and ModuleResolver use RegistryClient directly - Tests updated to use RegistryClient type for mocks/stubs - Exception contract changed from AbortOperationException to RegistryException for registry operations Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com> * fix import sort Signed-off-by: jorgee <jorge.ejarque@seqera.io> --------- Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com> Signed-off-by: jorgee <jorge.ejarque@seqera.io> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: jorgee <jorge.ejarque@seqera.io> --------- Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com> Signed-off-by: jorgee <jorge.ejarque@seqera.io> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Jorge Ejarque <jorgee@users.noreply.github.com> Co-authored-by: jorgee <jorge.ejarque@seqera.io>
1 parent 8aefdb2 commit 946454b

23 files changed

Lines changed: 188 additions & 208 deletions

‎modules/nextflow/build.gradle‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,8 @@ dependencies {
7171
api 'io.seqera:lib-trace:0.1.0'
7272
api 'com.fasterxml.woodstox:woodstox-core:7.1.1'
7373
api 'org.apache.commons:commons-compress:1.27.1' // For tar.gz extraction
74-
api 'io.seqera:npr-api:0.21.4-SNAPSHOT'
74+
api 'io.seqera:npr-api:0.22.0-SNAPSHOT'
75+
api 'io.seqera:npr-client:0.22.0-SNAPSHOT'
7576

7677
testImplementation 'org.subethamail:subethasmtp:3.1.7'
7778
testImplementation (project(':nf-lineage'))

‎modules/nextflow/src/main/groovy/nextflow/cli/module/CmdModuleInfo.groovy‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import com.beust.jcommander.Parameters
2323
import groovy.json.JsonOutput
2424
import groovy.transform.CompileStatic
2525
import groovy.util.logging.Slf4j
26+
import io.seqera.npr.client.RegistryClient
2627
import io.seqera.npr.api.schema.v1.ModuleChannel
2728
import io.seqera.npr.api.schema.v1.ModuleChannelItem
2829
import io.seqera.npr.api.schema.v1.ModuleMetadata
@@ -33,7 +34,7 @@ import nextflow.config.ConfigBuilder
3334
import nextflow.config.RegistryConfig
3435
import nextflow.exception.AbortOperationException
3536
import nextflow.module.ModuleReference
36-
import nextflow.module.ModuleRegistryClient
37+
import nextflow.module.RegistryClientFactory
3738
import nextflow.util.TestOnly
3839

3940
import java.nio.file.Path
@@ -77,7 +78,7 @@ class CmdModuleInfo extends CmdBase {
7778
protected Path root
7879

7980
@TestOnly
80-
protected ModuleRegistryClient client
81+
protected RegistryClient client
8182

8283
@Override
8384
String getName() {
@@ -102,7 +103,7 @@ class CmdModuleInfo extends CmdBase {
102103

103104

104105
// Fetch full metadata from registry to get input/output parameters
105-
def registryClient = this.client ?: new ModuleRegistryClient(registryConfig)
106+
def registryClient = this.client ?: RegistryClientFactory.forConfig(registryConfig)
106107
ModuleRelease release = null
107108

108109
try {
@@ -119,7 +120,7 @@ class CmdModuleInfo extends CmdBase {
119120
throw new AbortOperationException("No release information available for ${reference}")
120121
}
121122
if( !release.metadata ) {
122-
log.info("No metadata found for $reference ${release.version ? "($release.version)" : ''}")
123+
throw new AbortOperationException("No metadata found for ${reference}${release.version ? " (${release.version})" : ''}")
123124
}
124125
def moduleUrl = buildModuleUrl(registryConfig.url, reference, release.version)
125126
if( !output || output == 'text' ) {

‎modules/nextflow/src/main/groovy/nextflow/cli/module/CmdModuleInstall.groovy‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,13 @@ import com.beust.jcommander.Parameter
2020
import com.beust.jcommander.Parameters
2121
import groovy.transform.CompileStatic
2222
import groovy.util.logging.Slf4j
23+
import io.seqera.npr.client.RegistryClient
2324
import nextflow.cli.CmdBase
2425
import nextflow.config.ConfigBuilder
25-
2626
import nextflow.config.RegistryConfig
2727
import nextflow.exception.AbortOperationException
2828
import nextflow.module.ModuleReference
29-
import nextflow.module.ModuleRegistryClient
29+
import nextflow.module.RegistryClientFactory
3030
import nextflow.module.ModuleResolver
3131
import nextflow.module.ModuleSpec
3232

@@ -58,7 +58,7 @@ class CmdModuleInstall extends CmdBase {
5858
protected Path root
5959

6060
@TestOnly
61-
protected ModuleRegistryClient client
61+
protected RegistryClient client
6262

6363
@Override
6464
String getName() {
@@ -82,7 +82,7 @@ class CmdModuleInstall extends CmdBase {
8282
final registryConfig = config.navigate('registry') as RegistryConfig ?: new RegistryConfig()
8383

8484
// Create resolver and install
85-
def resolver = new ModuleResolver(baseDir, client ?: new ModuleRegistryClient(registryConfig))
85+
def resolver = new ModuleResolver(baseDir, client ?: RegistryClientFactory.forConfig(registryConfig))
8686

8787
try {
8888
def installedMainFile = resolver.installModule(reference, version, force)

‎modules/nextflow/src/main/groovy/nextflow/cli/module/CmdModulePublish.groovy‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import com.beust.jcommander.Parameter
2020
import com.beust.jcommander.Parameters
2121
import groovy.transform.CompileStatic
2222
import groovy.util.logging.Slf4j
23+
import io.seqera.npr.client.RegistryClient
2324
import nextflow.Const
2425
import nextflow.cli.CmdBase
2526
import nextflow.config.ConfigBuilder
@@ -29,7 +30,7 @@ import nextflow.module.ModuleChecksum
2930
import nextflow.module.ModuleInfo
3031
import nextflow.module.ModuleSpec
3132
import nextflow.module.ModuleReference
32-
import nextflow.module.ModuleRegistryClient
33+
import nextflow.module.RegistryClientFactory
3334
import nextflow.module.ModuleStorage
3435
import nextflow.util.TestOnly
3536

@@ -60,7 +61,7 @@ class CmdModulePublish extends CmdBase {
6061
protected Path root
6162

6263
@TestOnly
63-
protected ModuleRegistryClient client
64+
protected RegistryClient client
6465

6566
//Flag if publish is invoked from a scope/name. In this case we should create/update the .module-info with the correct checksum
6667
private boolean useModuleReference = false
@@ -138,7 +139,7 @@ class CmdModulePublish extends CmdBase {
138139
// Publish to registry
139140
final registry = registryUrl ?: registryConfig.url
140141
log.info "Publishing module to registry: ${registryUrl ?: registryConfig.url}"
141-
def registryClient = new ModuleRegistryClient(registryConfig)
142+
def registryClient = RegistryClientFactory.forConfig(registryConfig)
142143
def response = registryClient.publishModule(spec.name, request, registry)
143144

144145
if (useModuleReference) {
@@ -260,7 +261,7 @@ class CmdModulePublish extends CmdBase {
260261
final localStorage = new ModuleStorage(root ?: Paths.get('.').toAbsolutePath().normalize())
261262

262263
if (!localStorage.isInstalled(ref)){
263-
throw new AbortOperationException("No module diretory found for $module")
264+
throw new AbortOperationException("No module directory found for $module")
264265
}
265266
useModuleReference = true
266267
return localStorage.getModuleDir(ref)

‎modules/nextflow/src/main/groovy/nextflow/cli/module/CmdModuleRemove.groovy‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import groovy.transform.CompileStatic
2222
import groovy.util.logging.Slf4j
2323
import nextflow.cli.CmdBase
2424
import nextflow.exception.AbortOperationException
25-
import nextflow.module.ModuleInfo
2625
import nextflow.module.ModuleReference
2726
import nextflow.module.ModuleStorage
2827

‎modules/nextflow/src/main/groovy/nextflow/cli/module/CmdModuleRun.groovy‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,13 @@ package nextflow.cli.module
1919
import com.beust.jcommander.Parameter
2020
import com.beust.jcommander.Parameters
2121
import groovy.transform.CompileStatic
22+
import io.seqera.npr.client.RegistryClient
2223
import nextflow.cli.CmdRun
2324
import nextflow.config.ConfigBuilder
24-
2525
import nextflow.config.RegistryConfig
2626
import nextflow.exception.AbortOperationException
2727
import nextflow.module.ModuleReference
28-
import nextflow.module.ModuleRegistryClient
28+
import nextflow.module.RegistryClientFactory
2929
import nextflow.module.ModuleResolver
3030

3131
import nextflow.util.TestOnly
@@ -48,7 +48,7 @@ class CmdModuleRun extends CmdRun {
4848
protected Path root
4949

5050
@TestOnly
51-
protected ModuleRegistryClient client
51+
protected RegistryClient client
5252

5353
@Override
5454
String getName() {
@@ -78,7 +78,7 @@ class CmdModuleRun extends CmdRun {
7878

7979
def registryConfig = config.navigate('registry') as RegistryConfig ?: new RegistryConfig()
8080

81-
def resolver = new ModuleResolver(baseDir, client ?: new ModuleRegistryClient(registryConfig))
81+
def resolver = new ModuleResolver(baseDir, client ?: RegistryClientFactory.forConfig(registryConfig))
8282
Path moduleFile = resolver.installModule(reference, version)
8383
if( moduleFile ) {
8484
args[0] = moduleFile.toAbsolutePath().toString()

‎modules/nextflow/src/main/groovy/nextflow/cli/module/CmdModuleSearch.groovy‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,12 @@ import groovy.transform.CompileStatic
2525
import groovy.util.logging.Slf4j
2626
import io.seqera.npr.api.schema.v1.ModuleSearchResult
2727
import io.seqera.npr.api.schema.v1.SearchModulesResponse
28+
import io.seqera.npr.client.RegistryClient
2829
import nextflow.cli.CmdBase
2930
import nextflow.config.ConfigBuilder
3031
import nextflow.config.RegistryConfig
3132
import nextflow.exception.AbortOperationException
32-
import nextflow.module.ModuleRegistryClient
33+
import nextflow.module.RegistryClientFactory
3334
import nextflow.util.TestOnly
3435

3536
import java.nio.file.Paths
@@ -69,7 +70,7 @@ class CmdModuleSearch extends CmdBase {
6970
List<String> args
7071

7172
@TestOnly
72-
protected ModuleRegistryClient client
73+
protected RegistryClient client
7374

7475
@Override
7576
String getName() {
@@ -93,7 +94,7 @@ class CmdModuleSearch extends CmdBase {
9394
final registryConfig = config.navigate('registry') as RegistryConfig ?: new RegistryConfig()
9495

9596
// Create client to search
96-
final client = this.client ?: new ModuleRegistryClient(registryConfig)
97+
final client = this.client ?: RegistryClientFactory.forConfig(registryConfig)
9798

9899
try {
99100
println "Searching for '${query}'..."

‎modules/nextflow/src/main/groovy/nextflow/module/ModuleChecksum.groovy‎

Lines changed: 10 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -17,100 +17,45 @@
1717
package nextflow.module
1818

1919
import groovy.transform.CompileStatic
20-
import groovy.util.logging.Slf4j
20+
import io.seqera.npr.client.ModuleChecksum as SharedChecksum
2121

22-
import java.nio.file.Files
2322
import java.nio.file.Path
24-
import java.security.MessageDigest
25-
26-
import static nextflow.module.ModuleInfo.MODULE_INFO_FILE
2723

2824
/**
29-
* Utility class for computing SHA-256 checksums of module directories
25+
* Nextflow-specific checksum utility that delegates to the shared
26+
* {@link io.seqera.npr.client.ModuleChecksum} implementation and adds
27+
* convenience methods for .module-info persistence.
3028
*
3129
* @author Jorge Ejarque <jorge.ejarque@seqera.io>
3230
*/
33-
@Slf4j
3431
@CompileStatic
3532
class ModuleChecksum {
3633

37-
public static final String CHECKSUM_ALGORITHM = "SHA-256"
34+
public static final String CHECKSUM_ALGORITHM = SharedChecksum.ALGORITHM
3835

3936
/**
4037
* Compute the SHA-256 checksum of a module directory
41-
*
42-
* @param moduleDir The module directory path
43-
* @return The hex-encoded SHA-256 checksum
4438
*/
4539
static String compute(Path moduleDir) {
46-
if( !Files.exists(moduleDir) || !Files.isDirectory(moduleDir) ) {
47-
throw new IllegalArgumentException("Module directory does not exist or is not a directory: ${moduleDir}")
48-
}
49-
50-
try {
51-
def digest = MessageDigest.getInstance(CHECKSUM_ALGORITHM)
52-
53-
// Collect all files in sorted order for consistent checksums
54-
List<Path> files = []
55-
try( final walkStream = Files.walk(moduleDir) ) {
56-
walkStream
57-
.filter { Path path -> Files.isRegularFile(path) }
58-
.filter { Path path -> !path.fileName.toString().equals(MODULE_INFO_FILE) }
59-
.sorted()
60-
.each { Path path -> files.add(path) }
61-
}
62-
63-
// Compute checksum over all file contents
64-
byte[] buf = new byte[8192]
65-
for( Path file : files ) {
66-
// Include relative path in checksum for directory structure integrity
67-
def relativePath = moduleDir.relativize(file).toString()
68-
digest.update(relativePath.bytes)
69-
70-
// Include file contents via streaming to avoid loading large files into memory
71-
Files.newInputStream(file).withCloseable { is ->
72-
int n
73-
while( (n = is.read(buf)) != -1 ) {
74-
digest.update(buf, 0, n)
75-
}
76-
}
77-
}
78-
79-
def hashBytes = digest.digest()
80-
return bytesToHex(hashBytes)
81-
}
82-
catch( Exception e ) {
83-
log.error("Failed to compute checksum for module directory: ${moduleDir}", e)
84-
throw new RuntimeException("Failed to compute module checksum", e)
85-
}
40+
return SharedChecksum.computeDirectory(moduleDir)
8641
}
8742

8843
/**
89-
* Save a checksum to the .module-info file in the module directory
90-
*
91-
* @param moduleDir The module directory path
92-
* @param checksum The checksum to save
44+
* Save a checksum to the .module-info file
9345
*/
9446
static void save(Path moduleDir, String checksum) {
95-
ModuleInfo.save(moduleDir,'checksum', checksum)
47+
ModuleInfo.save(moduleDir, 'checksum', checksum)
9648
}
9749

9850
/**
99-
* Load a checksum from the .module-info file in the module directory
100-
*
101-
* @param moduleDir The module directory path
102-
* @return The checksum, or null if file doesn't exist
51+
* Load a checksum from the .module-info file
10352
*/
10453
static String load(Path moduleDir) {
10554
ModuleInfo.load(moduleDir, 'checksum')
10655
}
10756

10857
/**
10958
* Verify that a module directory matches the expected checksum
110-
*
111-
* @param moduleDir The module directory path
112-
* @param expectedChecksum The expected checksum
113-
* @return true if checksums match, false otherwise
11459
*/
11560
static boolean verify(Path moduleDir, String expectedChecksum) {
11661
def actualChecksum = compute(moduleDir)
@@ -119,46 +64,8 @@ class ModuleChecksum {
11964

12065
/**
12166
* Compute the checksum of a single file
122-
*
123-
* @param file The file path
124-
* @param type checksum algorithm (sha-256 if not provided)
125-
* @return The hex-encoded checksum
12667
*/
12768
static String computeFile(Path file, String type = CHECKSUM_ALGORITHM) {
128-
if( !Files.exists(file) || !Files.isRegularFile(file) ) {
129-
throw new IllegalArgumentException("File does not exist or is not a regular file: ${file}")
130-
}
131-
132-
try {
133-
final digest = MessageDigest.getInstance(type)
134-
final byte[] buf = new byte[8192]
135-
Files.newInputStream(file).withCloseable { is ->
136-
int n
137-
while( (n = is.read(buf)) != -1 ) {
138-
digest.update(buf, 0, n)
139-
}
140-
}
141-
return bytesToHex(digest.digest())
142-
}
143-
catch( Exception e ) {
144-
log.error("Failed to compute checksum for file: ${file}", e)
145-
throw new RuntimeException("Failed to compute file checksum", e)
146-
}
147-
}
148-
149-
/**
150-
* Convert byte array to hex string
151-
*
152-
* @param bytes The byte array
153-
* @return Hex-encoded string
154-
*/
155-
private static String bytesToHex(byte[] bytes) {
156-
def hexChars = new char[bytes.length * 2]
157-
for( int i = 0; i < bytes.length; i++ ) {
158-
int v = bytes[i] & 0xFF
159-
hexChars[i * 2] = Character.forDigit(v >>> 4, 16)
160-
hexChars[i * 2 + 1] = Character.forDigit(v & 0x0F, 16)
161-
}
162-
return new String(hexChars)
69+
return SharedChecksum.computeFile(file, type)
16370
}
16471
}

‎modules/nextflow/src/main/groovy/nextflow/module/ModuleInfo.groovy‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,13 +94,13 @@ class ModuleInfo {
9494
* Load all properties from the .module-info file in the module directory
9595
*
9696
* @param moduleDir The module directory path
97-
* @return The checksum, or null if file doesn't exist
97+
* @return Map of properties, or null if file doesn't exist
9898
*/
9999
static Map<String, String> load(Path moduleDir) {
100100
def moduleInfoFile = moduleDir.resolve(MODULE_INFO_FILE)
101101
if( !Files.exists(moduleInfoFile) ) {
102102
log.debug("Module file $moduleInfoFile not found")
103-
return [:]
103+
return null
104104
}
105105
def props = new Properties()
106106
moduleInfoFile.withInputStream { is -> props.load(is) }

‎modules/nextflow/src/main/groovy/nextflow/module/ModuleReference.groovy‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,7 @@ import java.util.regex.Pattern
3131
@EqualsAndHashCode
3232
class ModuleReference {
3333

34-
// Pattern allows: scope with letters/digits/hyphens/dots/underscores, name segments separated by slashes (no trailing slash)
35-
// Scope: starts with letter/digit, followed by letters/digits/dots/underscores/hyphens
36-
// Name: one or more segments (each starting with letter, followed by letters/digits/underscores/hyphens), separated by slashes
34+
// Must stay in sync with ModuleResolver.REMOTE_MODULE_PATTERN in nf-lang
3735
private static final Pattern MODULE_NAME_PATTERN = ~/^([a-z0-9][a-z0-9._\-]*)\/([a-z][a-z0-9._\-]*(?:\/[a-z][a-z0-9._\-]*)*)$/
3836

3937
final String scope

0 commit comments

Comments
 (0)
⚡