From 11fafcf4ae58974932b0cc6c096b648addf1996b Mon Sep 17 00:00:00 2001 From: Momo Kornher Date: Wed, 12 Nov 2025 09:01:55 +0000 Subject: [PATCH] feat: validate submodule target configurations (#2415) Extends the target configuration validation from #2398 to also validate `.jsiirc.json` files used for submodule-specific target configurations. Additionally enhances validation to ensure target configurations are well-formed objects with only valid keys for each language. ## Why Submodules can have their own target configurations via `.jsiirc.json` files (for directory-based submodules) or `..jsiirc.json` files (for file-based submodules). These configurations were not being validated, allowing invalid configurations to slip through until code generation time. This PR ensures configuration errors are caught early during compilation with clear error messages, rather than failing later during package generation. ## Changes * Exports `validateTargets` function from `project-info.ts` for reuse * Calls `validateTargets` when loading `.jsiirc.json` files in `assembler.ts` * Adds comprehensive validation for target configuration structure * Adds 21 new tests covering: - File-based `.jsiirc.json` validation (e.g., `.subfile.jsiirc.json`) - Namespace conflict detection across all target languages - Target configuration structure validation - Unknown language and key detection ## Validation Rules ### Identifier Validation Target language package names must contain only valid identifier characters: * **Go**: `packageName` must be a valid identifier * **.NET**: `namespace` parts (split by `.`) must be valid identifiers * **Java**: `package` parts (split by `.`) must be valid identifiers * **Python**: `module` parts (split by `.`) must be valid identifiers An identifier is defined as matching the regex: `/^[\w_][\w\d_]*$/u` ### Structure Validation * Target language entries must be objects (not strings or primitives) * Only known target languages are allowed: `java`, `python`, `dotnet`, `go` * Only valid configuration keys are allowed for each language: - **Java**: `package`, `maven`, `versionSuffix` - **Python**: `module`, `distName`, `classifiers` - **.NET**: `namespace`, `packageId`, `iconUrl`, `versionSuffix` - **Go**: `moduleName`, `packageName`, `versionSuffix` ### Namespace Conflict Detection Warns when multiple submodules emit to the same target language namespace, which would cause conflicts during code generation. --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license](https://www.apache.org/licenses/LICENSE-2.0). --------- Signed-off-by: github-actions Co-authored-by: github-actions (cherry picked from commit 558c3eeb389d82e7a3b5809a4002d01e7f1780ec) --- src/assembler.ts | 4 +- src/project-info.ts | 54 ++++++++- test/compiler.test.ts | 2 +- test/project-info.test.ts | 2 +- test/submodules.test.ts | 242 +++++++++++++++++++++++++++++++++++++- 5 files changed, 291 insertions(+), 13 deletions(-) diff --git a/src/assembler.ts b/src/assembler.ts index 7bc9039b..63d618f1 100644 --- a/src/assembler.ts +++ b/src/assembler.ts @@ -17,7 +17,7 @@ import { normalizeConfigPath } from './helpers'; import { JsiiDiagnostic } from './jsii-diagnostic'; import * as literate from './literate'; import * as bindings from './node-bindings'; -import { AssemblyTargets, ProjectInfo } from './project-info'; +import { AssemblyTargets, ProjectInfo, validateTargets } from './project-info'; import { isReservedName } from './reserved-words'; import { Sets } from './sets'; import { DeprecatedRemover } from './transforms/deprecated-remover'; @@ -721,7 +721,7 @@ export class Assembler implements Emitter { return undefined; } const data = JSON.parse(fs.readFileSync(jsiirc, 'utf-8')); - return data.targets; + return validateTargets(data.targets as AssemblyTargets) as SubmoduleSpec['targets']; } /** diff --git a/src/project-info.ts b/src/project-info.ts index 296a393c..56cf0088 100644 --- a/src/project-info.ts +++ b/src/project-info.ts @@ -308,19 +308,53 @@ export function loadProjectInfo(projectRoot: string): ProjectInfoResult { /** * Validate the values of the `.jsii.targets` field */ -function validateTargets(targets: AssemblyTargets | undefined): AssemblyTargets | undefined { +export function validateTargets(targets: AssemblyTargets | undefined): AssemblyTargets | undefined { if (!targets) { return undefined; } + const VALID_TARGET_KEYS: Record = { + java: ['package', 'maven', 'versionSuffix'], + python: ['module', 'distName', 'classifiers'], + dotnet: ['namespace', 'packageId', 'iconUrl', 'versionSuffix'], + go: ['moduleName', 'packageName', 'versionSuffix'], + }; + + for (const [language, config] of Object.entries(targets)) { + if (!(language in VALID_TARGET_KEYS)) { + throw new JsiiError(`Unknown target language: ${language}`); + } + + if (typeof config !== 'object' || config === null) { + throw new JsiiError(`jsii.targets.${language} must be an object`); + } + + const validKeys = VALID_TARGET_KEYS[language]; + for (const key of Object.keys(config)) { + if (!validKeys.includes(key)) { + throw new JsiiError(`Unknown key in jsii.targets.${language}: ${key}`); + } + } + } + // Go package names must be valid identifiers - if (targets.go) { + if ( + targets.go && + typeof targets.go === 'object' && + 'packageName' in targets.go && + typeof targets.go.packageName === 'string' + ) { if (!isIdentifier(targets.go.packageName)) { throw new JsiiError(`jsii.targets.go.packageName contains non-identifier characters: ${targets.go.packageName}`); } } - if (targets.dotnet) { + if ( + targets.dotnet && + typeof targets.dotnet === 'object' && + 'namespace' in targets.dotnet && + typeof targets.dotnet.namespace === 'string' + ) { if (!targets.dotnet.namespace.split('.').every(isIdentifier)) { throw new JsiiError( `jsii.targets.dotnet.namespace contains non-identifier characters: ${targets.dotnet.namespace}`, @@ -328,13 +362,23 @@ function validateTargets(targets: AssemblyTargets | undefined): AssemblyTargets } } - if (targets.java) { + if ( + targets.java && + typeof targets.java === 'object' && + 'package' in targets.java && + typeof targets.java.package === 'string' + ) { if (!targets.java.package.split('.').every(isIdentifier)) { throw new JsiiError(`jsii.targets.java.package contains non-identifier characters: ${targets.java.package}`); } } - if (targets.python) { + if ( + targets.python && + typeof targets.python === 'object' && + 'module' in targets.python && + typeof targets.python.module === 'string' + ) { if (!targets.python.module.split('.').every(isIdentifier)) { throw new JsiiError(`jsii.targets.python.module contains non-identifier characters: ${targets.python.module}`); } diff --git a/test/compiler.test.ts b/test/compiler.test.ts index a92322db..5f8dc5da 100644 --- a/test/compiler.test.ts +++ b/test/compiler.test.ts @@ -116,7 +116,7 @@ describe(Compiler, () => { } finally { rmSync(sourceDir, { force: true, recursive: true }); } - }, 25_000); + }, 40_000); test('rootDir is added to assembly', () => { const outDir = 'jsii-outdir'; diff --git a/test/project-info.test.ts b/test/project-info.test.ts index ffd8f06a..1b5790f6 100644 --- a/test/project-info.test.ts +++ b/test/project-info.test.ts @@ -24,7 +24,7 @@ const BASE_PROJECT = { main: 'index.js', types: 'index.d.ts', jsii: { - targets: { foo: { bar: 'baz' } }, + targets: { python: { module: 'test_module', distName: 'test-dist' } }, }, dependencies: { 'jsii-test-dep': '^1.2.3' } as { [name: string]: string }, peerDependencies: { 'jsii-test-dep': '^1.2.3' } as { [name: string]: string }, diff --git a/test/submodules.test.ts b/test/submodules.test.ts index da6d80d2..4a453cc1 100644 --- a/test/submodules.test.ts +++ b/test/submodules.test.ts @@ -1,7 +1,9 @@ import * as spec from '@jsii/spec'; import { writeAssembly, loadAssemblyFromPath } from '@jsii/spec'; +import * as ts from 'typescript'; import { sourceToAssemblyHelper, TestWorkspace, compileJsiiForTest } from '../lib'; +import { compileJsiiForErrors } from './compiler-helpers'; test('submodules loaded from directories can have a README', () => { const assembly = sourceToAssemblyHelper({ @@ -41,7 +43,7 @@ test('submodules loaded from directories can have targets', () => { 'subdir/index.ts': 'export class Foo { }', 'subdir/.jsiirc.json': JSON.stringify({ targets: { - python: 'fun', + python: { module: 'fun', distName: 'fun-dist' }, }, }), }); @@ -49,7 +51,7 @@ test('submodules loaded from directories can have targets', () => { expect(assembly.submodules!['testpkg.submodule']).toEqual( expect.objectContaining({ targets: { - python: 'fun', + python: { module: 'fun', distName: 'fun-dist' }, }, }), ); @@ -61,7 +63,7 @@ test('submodules loaded from files can have targets', () => { 'subfile.ts': 'export class Foo { }', '.subfile.jsiirc.json': JSON.stringify({ targets: { - python: 'fun', + python: { module: 'fun', distName: 'fun-dist' }, }, }), }); @@ -69,7 +71,7 @@ test('submodules loaded from files can have targets', () => { expect(assembly.submodules!['testpkg.submodule']).toEqual( expect.objectContaining({ targets: { - python: 'fun', + python: { module: 'fun', distName: 'fun-dist' }, }, }), ); @@ -322,3 +324,235 @@ function makeDependencyWithSubmoduleAndNamespace() { 'subdir/README.md': 'This is the README', }); } + +describe('invalid .jsiirc.json target configuration', () => { + test('invalid Go packageName is rejected', () => { + const errors = compileJsiiForErrors({ + 'index.ts': 'export * as submodule from "./subdir"', + 'subdir/index.ts': 'export class Foo { }', + 'subdir/.jsiirc.json': JSON.stringify({ + targets: { + go: { + moduleName: 'asdf', + packageName: 'as-df', + }, + }, + }), + }); + expect(errors).toContainEqual( + expect.stringMatching(/jsii\.targets\.go\.packageName contains non-identifier characters/), + ); + }); + + test('invalid .NET namespace is rejected', () => { + const errors = compileJsiiForErrors({ + 'index.ts': 'export * as submodule from "./subdir"', + 'subdir/index.ts': 'export class Foo { }', + 'subdir/.jsiirc.json': JSON.stringify({ + targets: { + dotnet: { + namespace: 'a-x', + packageId: 'asdf', + }, + }, + }), + }); + expect(errors).toContainEqual( + expect.stringMatching(/jsii\.targets\.dotnet\.namespace contains non-identifier characters/), + ); + }); + + test('invalid Java package is rejected', () => { + const errors = compileJsiiForErrors({ + 'index.ts': 'export * as submodule from "./subdir"', + 'subdir/index.ts': 'export class Foo { }', + 'subdir/.jsiirc.json': JSON.stringify({ + targets: { + java: { + package: 'as-df', + maven: { + artifactId: 'asdf', + groupId: 'asdf', + }, + }, + }, + }), + }); + expect(errors).toContainEqual( + expect.stringMatching(/jsii\.targets\.java\.package contains non-identifier characters/), + ); + }); + + test('invalid Python module is rejected', () => { + const errors = compileJsiiForErrors({ + 'index.ts': 'export * as submodule from "./subdir"', + 'subdir/index.ts': 'export class Foo { }', + 'subdir/.jsiirc.json': JSON.stringify({ + targets: { + python: { + module: 'as-df', + distName: 'as-df', + }, + }, + }), + }); + expect(errors).toContainEqual( + expect.stringMatching(/jsii\.targets\.python\.module contains non-identifier characters/), + ); + }); + + test('invalid Python module in file-based config is rejected', () => { + const errors = compileJsiiForErrors({ + 'index.ts': 'export * as submodule from "./subfile"', + 'subfile.ts': 'export class Foo { }', + '.subfile.jsiirc.json': JSON.stringify({ + targets: { + python: { + module: 'invalid-name', + distName: 'dist', + }, + }, + }), + }); + expect(errors).toContainEqual( + expect.stringMatching(/jsii\.targets\.python\.module contains non-identifier characters/), + ); + }); + + test('non-object target language config is rejected', () => { + const errors = compileJsiiForErrors({ + 'index.ts': 'export * as submodule from "./subfile"', + 'subfile.ts': 'export class Foo { }', + '.subfile.jsiirc.json': JSON.stringify({ + targets: { + python: 'not-an-object', + }, + }), + }); + expect(errors).toContainEqual(expect.stringMatching(/jsii\.targets\.python must be an object/)); + }); + + test('unknown target language is rejected', () => { + const errors = compileJsiiForErrors({ + 'index.ts': 'export * as submodule from "./subfile"', + 'subfile.ts': 'export class Foo { }', + '.subfile.jsiirc.json': JSON.stringify({ + targets: { + rust: { package: 'my-crate' }, + }, + }), + }); + expect(errors).toContainEqual(expect.stringMatching(/Unknown target language: rust/)); + }); + + test('unknown key in target config is rejected', () => { + const errors = compileJsiiForErrors({ + 'index.ts': 'export * as submodule from "./subfile"', + 'subfile.ts': 'export class Foo { }', + '.subfile.jsiirc.json': JSON.stringify({ + targets: { + python: { + module: 'valid_module', + distName: 'dist', + unknownKey: 'value', + }, + }, + }), + }); + expect(errors).toContainEqual(expect.stringMatching(/Unknown key in jsii\.targets\.python: unknownKey/)); + }); +}); + +describe('submodule namespace conflicts', () => { + test('conflicting .NET namespaces produce warnings', () => { + const result = compileJsiiForTest( + { + 'index.ts': 'export * as sub1 from "./sub1"; export * as sub2 from "./sub2"', + 'sub1.ts': 'export class Foo { }', + 'sub2.ts': 'export class Bar { }', + '.sub1.jsiirc.json': JSON.stringify({ + targets: { dotnet: { namespace: 'Same.Namespace', packageId: 'pkg1' } }, + }), + '.sub2.jsiirc.json': JSON.stringify({ + targets: { dotnet: { namespace: 'Same.Namespace', packageId: 'pkg2' } }, + }), + }, + { captureDiagnostics: true }, + ); + const warnings = result.diagnostics + .filter((d) => d.category === ts.DiagnosticCategory.Warning) + .map((d) => `${d.messageText}`); + expect(warnings).toContainEqual(expect.stringMatching(/dotnet.*Same\.Namespace.*testpkg\.sub1.*testpkg\.sub2/)); + }); + + test('conflicting Java packages produce warnings', () => { + const result = compileJsiiForTest( + { + 'index.ts': 'export * as sub1 from "./sub1"; export * as sub2 from "./sub2"', + 'sub1.ts': 'export class Foo { }', + 'sub2.ts': 'export class Bar { }', + '.sub1.jsiirc.json': JSON.stringify({ + targets: { java: { package: 'same.pkg', maven: { artifactId: 'a', groupId: 'g' } } }, + }), + '.sub2.jsiirc.json': JSON.stringify({ + targets: { java: { package: 'same.pkg', maven: { artifactId: 'b', groupId: 'g' } } }, + }), + }, + { captureDiagnostics: true }, + ); + const warnings = result.diagnostics + .filter((d) => d.category === ts.DiagnosticCategory.Warning) + .map((d) => `${d.messageText}`); + expect(warnings).toContainEqual(expect.stringMatching(/java.*same\.pkg.*testpkg\.sub1.*testpkg\.sub2/)); + }); + + test('conflicting Python modules produce warnings', () => { + const result = compileJsiiForTest( + { + 'index.ts': 'export * as sub1 from "./sub1"; export * as sub2 from "./sub2"', + 'sub1.ts': 'export class Foo { }', + 'sub2.ts': 'export class Bar { }', + '.sub1.jsiirc.json': JSON.stringify({ targets: { python: { module: 'same_module', distName: 'dist1' } } }), + '.sub2.jsiirc.json': JSON.stringify({ targets: { python: { module: 'same_module', distName: 'dist2' } } }), + }, + { captureDiagnostics: true }, + ); + const warnings = result.diagnostics + .filter((d) => d.category === ts.DiagnosticCategory.Warning) + .map((d) => `${d.messageText}`); + expect(warnings).toContainEqual(expect.stringMatching(/python.*same_module.*testpkg\.sub1.*testpkg\.sub2/)); + }); + + test('conflicting Go packages produce warnings', () => { + const result = compileJsiiForTest( + { + 'index.ts': 'export * as sub1 from "./sub1"; export * as sub2 from "./sub2"', + 'sub1.ts': 'export class Foo { }', + 'sub2.ts': 'export class Bar { }', + '.sub1.jsiirc.json': JSON.stringify({ targets: { go: { moduleName: 'mod1', packageName: 'samepkg' } } }), + '.sub2.jsiirc.json': JSON.stringify({ targets: { go: { moduleName: 'mod2', packageName: 'samepkg' } } }), + }, + { captureDiagnostics: true }, + ); + const warnings = result.diagnostics + .filter((d) => d.category === ts.DiagnosticCategory.Warning) + .map((d) => `${d.messageText}`); + expect(warnings).toContainEqual(expect.stringMatching(/go.*samepkg.*testpkg\.sub1.*testpkg\.sub2/)); + }); + + test('directory and file submodules with different configs do not conflict', () => { + const assembly = sourceToAssemblyHelper({ + 'index.ts': 'export * as sub1 from "./subdir"; export * as sub2 from "./sub2"', + 'subdir/index.ts': 'export class Foo { }', + 'sub2.ts': 'export class Bar { }', + 'subdir/.jsiirc.json': JSON.stringify({ targets: { python: { module: 'dir_module', distName: 'dir-dist' } } }), + '.sub2.jsiirc.json': JSON.stringify({ targets: { python: { module: 'file_module', distName: 'file-dist' } } }), + }); + expect(assembly.submodules!['testpkg.sub1'].targets).toEqual({ + python: { module: 'dir_module', distName: 'dir-dist' }, + }); + expect(assembly.submodules!['testpkg.sub2'].targets).toEqual({ + python: { module: 'file_module', distName: 'file-dist' }, + }); + }); +});