Skip to content

Commit 5a7bee7

Browse files
Fixes for projects that use cds-indexer (#378)
* Fix cds-indexer flow for plugin-rich projects cds-indexer support, as introduced, called projectInstallDependencies AFTER running the indexer and used 'npm install --ignore-scripts' for that install. For projects that depend on cds plugins like @sap/cds-shim, @sap/cds-mtxs, @cap-js/hana, file-based local plugins, or anything whose cds-plugin.js entry is wired by a postinstall/prepare hook, this produced an under-populated model walk: - The indexer ran first against an empty project node_modules, so plugins never registered their hooks. The generated index.cds files therefore failed to transitively include everything that annotation files in the project reference. - The full install that followed ran with --ignore-scripts, leaving any plugin that depends on its own lifecycle hooks (e.g. the project's own husky 'prepare', or packages whose cds-plugin.js gets emitted by postinstall) only half-wired even after install completed. Subsequent 'cds compile db srv app --to json' then failed with dozens of 'Artifact has not been found' errors against service-level projections and _texts companion entities the plugins were supposed to synthesize. Two changes: 1. indexer.ts: install full project dependencies BEFORE running the indexer, so the indexer walks a fully-populated model. Record the install on project.retryStatus.fullDependenciesInstalled so the compilation retry path (needsFullDependencyInstallation) doesn't repeat the same install. If install fails, log a warning and still attempt the indexer — the cache binary may still produce useful index files. 2. projectInstaller.ts: prefer 'npm ci' when a package-lock.json exists, matching what the project locked exactly. Drop --ignore-scripts so cds-plugin lifecycle hooks can run. Falls back to 'npm install' when there is no lockfile. Verified on the SAP ctsm project (~843 cds files, ~1700 deps, mix of @sap/cds-shim, @sap/cds-mtxs, @cap-js/hana, @cap-js/ord, and a file-based local plugin): all 'Artifact not found' errors disappear, leaving only the project's pre-existing modeling warnings — same output as the working manual flow ('npm ci' + 'npx @sap/cds-indexer' + 'cds compile'). Note: this enables npm lifecycle script execution for projects under analysis. Threat model is comparable to running 'cds compile' itself, which already loads and executes plugin code from node_modules; some existing CI tests on Linux may need adjustment for the new install behavior. * Update tests to match new cds-indexer install order Adjusts the two unit tests that were encoding the old behavior: - projectInstaller.test.ts: the install args no longer include '--ignore-scripts'. Adds a new test for the 'npm ci' branch taken when a package-lock.json exists. Mocks fs/existsSync so the lockfile-detection check is deterministic. - indexer.test.ts: the install now happens BEFORE the indexer, so an indexer failure no longer gates the install. The 'should not install ... when cds-indexer fails' test is replaced with 'should still install ... even when cds-indexer subsequently fails' to assert the new contract. Renames the post-install-failure test to reflect that the install runs first.
1 parent c887110 commit 5a7bee7

6 files changed

Lines changed: 137 additions & 68 deletions

File tree

extractors/cds/tools/dist/cds-extractor.bundle.js

Lines changed: 39 additions & 30 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

extractors/cds/tools/dist/cds-extractor.bundle.js.map

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

extractors/cds/tools/src/cds/indexer.ts

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -198,24 +198,53 @@ export function orchestrateCdsIndexer(
198198

199199
summary.projectsRequiringIndexer++;
200200
const cacheDir = projectCacheDirMap.get(projectDir);
201+
202+
// Install the project's full dependencies BEFORE running cds-indexer.
203+
//
204+
// The indexer loads the project's cds runtime to walk the model and decide
205+
// which `.cds` files to include in the generated `index.cds` files. If the
206+
// project's plugins (e.g. `@sap/cds-shim`, `@sap/cds-mtxs`, `@cap-js/hana`,
207+
// file-based local plugins) are not yet installed, those plugins never
208+
// register their hooks, and the indexer sees a stripped-down model. The
209+
// resulting `index.cds` files then fail to transitively include every file
210+
// the project's annotations reference, and later `cds compile` fails with
211+
// "Artifact not found" errors.
212+
//
213+
// The cache directory only contains `@sap/cds`, `@sap/cds-dk`, and
214+
// `@sap/cds-indexer`, which is not enough for projects that reference
215+
// additional CDS packages — hence the project-local install.
216+
const installResult = projectInstallDependencies(project, sourceRoot);
217+
218+
// Record the install on the project's retry status so the retry path
219+
// (see needsFullDependencyInstallation) doesn't run a second redundant
220+
// `npm install` after compilation. Mirrors the bookkeeping done in
221+
// retry.ts when the retry path itself drives the install.
222+
project.retryStatus ??= {
223+
fullDependenciesInstalled: false,
224+
tasksRequiringRetry: 0,
225+
tasksRetried: 0,
226+
installationErrors: [],
227+
};
228+
229+
if (installResult.success) {
230+
project.retryStatus.fullDependenciesInstalled = true;
231+
dependencyGraph.retryStatus.projectsWithFullDependencies.add(projectDir);
232+
} else {
233+
project.retryStatus.installationErrors = [
234+
...(project.retryStatus.installationErrors ?? []),
235+
installResult.error ?? 'Unknown installation error',
236+
];
237+
cdsExtractorLog(
238+
'warn',
239+
`Full dependency installation failed for project '${projectDir}' before cds-indexer run: ${installResult.error ?? 'unknown error'}. Continuing with indexer attempt — it may still produce useful output.`,
240+
);
241+
}
242+
201243
const result = runCdsIndexer(project, sourceRoot, cacheDir);
202244
summary.results.push(result);
203245

204246
if (result.success) {
205247
summary.successfulRuns++;
206-
207-
// Install the project's full dependencies so the CDS compiler can
208-
// resolve all CDS model imports (e.g. `@sap/cds-shim`) during
209-
// compilation. The cache directory only contains `@sap/cds`,
210-
// `@sap/cds-dk`, and `@sap/cds-indexer`, which is not enough for
211-
// projects that reference additional CDS packages.
212-
const installResult = projectInstallDependencies(project, sourceRoot);
213-
if (!installResult.success) {
214-
cdsExtractorLog(
215-
'warn',
216-
`Full dependency installation failed for project '${projectDir}' after successful cds-indexer run: ${installResult.error ?? 'unknown error'}`,
217-
);
218-
}
219248
} else {
220249
summary.failedRuns++;
221250

extractors/cds/tools/src/packageManager/projectInstaller.ts

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/** Full dependency installation utilities for retry scenarios. */
22

33
import { execFileSync } from 'child_process';
4+
import { existsSync } from 'fs';
45
import { join } from 'path';
56

67
import { npmExecutable, getPlatformInfo } from '../environment';
@@ -65,10 +66,24 @@ export function projectInstallDependencies(
6566
return result;
6667
}
6768

68-
// Install dependencies using npm in the project's directory
69+
// Install dependencies using npm in the project's directory.
70+
//
71+
// Prefer `npm ci` when a lockfile exists: it installs exactly the versions
72+
// the project pinned, matching what `npm ci` would do in CI. Fall back to
73+
// `npm install` only when there is no lockfile.
74+
//
75+
// We deliberately do NOT pass `--ignore-scripts`. Many cds plugins
76+
// (e.g. file-based local plugins, and packages whose `cds-plugin.js`
77+
// entries get wired via `postinstall`/`prepare`) need their lifecycle
78+
// scripts to run for the cds runtime to discover them. Without those
79+
// scripts, the indexer walks an under-populated model and the resulting
80+
// `index.cds` files omit artifacts that annotation files reference,
81+
// causing later `cds compile` to fail with "Artifact not found".
82+
const hasLockfile = existsSync(join(projectPath, 'package-lock.json'));
83+
const installSubcommand = hasLockfile ? 'ci' : 'install';
6984
cdsExtractorLog(
7085
'info',
71-
`Installing full dependencies for project ${project.projectDir} in project's node_modules`,
86+
`Installing full dependencies for project ${project.projectDir} via 'npm ${installSubcommand}' in project's node_modules`,
7287
);
7388

7489
try {
@@ -78,14 +93,7 @@ export function projectInstallDependencies(
7893
// (e.g. engines.node ^18) which would otherwise abort the install on newer Node.
7994
// npm's default is non-strict; we make that explicit so the project's .npmrc
8095
// can't flip it on and break the retry path.
81-
[
82-
'install',
83-
'--engine-strict=false',
84-
'--ignore-scripts',
85-
'--quiet',
86-
'--no-audit',
87-
'--no-fund',
88-
],
96+
[installSubcommand, '--engine-strict=false', '--quiet', '--no-audit', '--no-fund'],
8997
{
9098
cwd: projectPath,
9199
stdio: 'inherit',

extractors/cds/tools/test/src/cds/indexer.test.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,7 @@ describe('cds/indexer', () => {
494494
);
495495
});
496496

497-
it('should install full project dependencies after successful cds-indexer run', () => {
497+
it('should install full project dependencies before running cds-indexer', () => {
498498
(childProcess.spawnSync as jest.Mock).mockReturnValue({
499499
status: 0,
500500
stdout: Buffer.from(''),
@@ -516,7 +516,10 @@ describe('cds/indexer', () => {
516516
expect(projectInstaller.projectInstallDependencies).toHaveBeenCalledWith(project, '/source');
517517
});
518518

519-
it('should not install full project dependencies when cds-indexer fails', () => {
519+
it('should still install full project dependencies even when cds-indexer subsequently fails', () => {
520+
// Install runs BEFORE the indexer now, so an indexer failure can no longer
521+
// gate the install. We still want the install to have happened so that
522+
// downstream compilation has a fully-populated node_modules to work with.
520523
(childProcess.spawnSync as jest.Mock).mockReturnValue({
521524
status: 1,
522525
stdout: Buffer.from(''),
@@ -535,7 +538,7 @@ describe('cds/indexer', () => {
535538

536539
orchestrateCdsIndexer(graph, '/source', new Map());
537540

538-
expect(projectInstaller.projectInstallDependencies).not.toHaveBeenCalled();
541+
expect(projectInstaller.projectInstallDependencies).toHaveBeenCalledWith(project, '/source');
539542
});
540543

541544
it('should not install full project dependencies for projects without cds-indexer', () => {
@@ -550,7 +553,7 @@ describe('cds/indexer', () => {
550553
expect(projectInstaller.projectInstallDependencies).not.toHaveBeenCalled();
551554
});
552555

553-
it('should continue even when full dependency installation fails after indexer success', () => {
556+
it('should continue even when full dependency installation fails before the indexer runs', () => {
554557
(childProcess.spawnSync as jest.Mock).mockReturnValue({
555558
status: 0,
556559
stdout: Buffer.from(''),

extractors/cds/tools/test/src/packageManager/projectInstaller.test.ts

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/** Tests for CDS compiler installer functionality */
22

33
import { execFileSync } from 'child_process';
4+
import { existsSync } from 'fs';
45

56
import type { CdsProject } from '../../../src/cds/parser';
67
import { getPlatformInfo } from '../../../src/environment';
@@ -11,14 +12,19 @@ import {
1112

1213
// Mock all external dependencies
1314
jest.mock('child_process');
15+
jest.mock('fs');
1416

1517
const mockExecFileSync = execFileSync as jest.MockedFunction<typeof execFileSync>;
18+
const mockExistsSync = existsSync as jest.MockedFunction<typeof existsSync>;
1619

1720
describe('CDS Compiler Installer', () => {
1821
const mockSourceRoot = '/test/source';
1922

2023
beforeEach(() => {
2124
jest.clearAllMocks();
25+
// Default: no package-lock.json present, so projectInstallDependencies
26+
// falls through to 'npm install'. Tests that need 'npm ci' override this.
27+
mockExistsSync.mockReturnValue(false);
2228
// Mock Date.now to return a consistent timestamp for testing
2329
jest.spyOn(Date, 'now').mockReturnValue(1640995200000);
2430
// Mock process.hrtime.bigint for performance timing
@@ -65,14 +71,28 @@ describe('CDS Compiler Installer', () => {
6571

6672
expect(mockExecFileSync).toHaveBeenCalledWith(
6773
'npm',
68-
[
69-
'install',
70-
'--engine-strict=false',
71-
'--ignore-scripts',
72-
'--quiet',
73-
'--no-audit',
74-
'--no-fund',
75-
],
74+
['install', '--engine-strict=false', '--quiet', '--no-audit', '--no-fund'],
75+
{
76+
cwd: '/test/source/test-project',
77+
shell: getPlatformInfo().isWindows,
78+
stdio: 'inherit',
79+
timeout: 120000,
80+
},
81+
);
82+
});
83+
84+
it("should use 'npm ci' when a package-lock.json exists in the project", () => {
85+
const project = createMockProject();
86+
87+
mockExistsSync.mockReturnValue(true);
88+
mockExecFileSync.mockReturnValue(Buffer.from(''));
89+
90+
const result = projectInstallDependencies(project, mockSourceRoot);
91+
92+
expect(result.success).toBe(true);
93+
expect(mockExecFileSync).toHaveBeenCalledWith(
94+
'npm',
95+
['ci', '--engine-strict=false', '--quiet', '--no-audit', '--no-fund'],
7696
{
7797
cwd: '/test/source/test-project',
7898
shell: getPlatformInfo().isWindows,

0 commit comments

Comments
 (0)