diff --git a/scripts/check-baseline-totals.mjs b/scripts/check-baseline-totals.mjs index fa06f5d5..944c547d 100644 --- a/scripts/check-baseline-totals.mjs +++ b/scripts/check-baseline-totals.mjs @@ -8,8 +8,10 @@ * reaches goes up, and the per-module checks read the baseline in the tree. * So a move could carry a rise inside it unseen. This compares each * baseline's total with the base branch's: a move keeps the totals, typing - * lowers them, nothing may raise them. lint-baseline.json's total and the - * dead-code total are compared too. + * lowers them, nothing may raise them. A module the base already counts may + * not rise either (moves create their modules), so a rise cannot hide behind + * a fall elsewhere. lint-baseline.json's total and the dead-code total are + * compared too. * * node scripts/check-baseline-totals.mjs [base] # base defaults to origin/main */ @@ -63,6 +65,22 @@ for (const [name, file, total] of BASELINES) { if (before !== undefined && now > before) rose.push(`${name} rose: ${now}, base ${before}`); } +// A module the base already counts may not rise, unless it is one the split +// creates to receive moved code (spec 014: the shared modules, the families, +// the registry and the server), which a later move may add to. Code is moved +// out of service.ts into those, never into the services' other modules, so +// a rise in any other is new code, whatever fell elsewhere to keep the +// total. The split's modules are held by the total. +const RECEIVES_MOVES = + /^tools\/content-node\/(java-text|body|spring|answers|objects|reference-entities|search-params|release|registry|server|[\w-]+-endpoints)\.ts$/; +for (const [name, file] of BASELINES.slice(0, 2)) { + const now = JSON.parse(readFileSync(file, 'utf8')); + const was = atBase(file) ?? {}; + for (const [module, count] of Object.entries(now)) + if (module in was && count > was[module] && !RECEIVES_MOVES.test(module)) + rose.push(`${name} in ${module} rose: ${count}, base ${was[module]}`); +} + // Lint's counts are per rule and global, so no move changes them; a rise in // one rule must not hide behind a fall in another (check-lint.mjs's own // reason for counting per rule). diff --git a/scripts/check-lint.mjs b/scripts/check-lint.mjs index 6bb4d1b9..f57f3522 100644 --- a/scripts/check-lint.mjs +++ b/scripts/check-lint.mjs @@ -90,6 +90,13 @@ export function countLoose(report, root = ROOT) { return modules; } +/** Modules whose loose count went down, not yet locked in. */ +export const looseDecreases = (counts, baseline) => + Object.entries(baseline) + .filter(([module, n]) => (counts[module] ?? 0) < n) + .map(([module, n]) => `${module}: ${n} -> ${counts[module] ?? 0}`) + .sort(); + /** Modules whose loose count went up; one the baseline does not list is held to none. */ export const looseIncreases = (counts, baseline) => Object.entries(counts) @@ -191,6 +198,14 @@ async function main() { console.error('\nType them: `npx eslint ` lists them (spec 013).'); process.exit(1); } + // A fall not locked in is room a later change could fill unseen. + const tighter = looseDecreases(loose, looseBaseline); + if (tighter.length > 0) { + console.error('Loosely typed values in the services went down; lock it in:\n'); + for (const line of tighter) console.error(` ${line}`); + console.error('\nnpm run check:lint -- --update'); + process.exit(1); + } const { increased, decreased, cleared } = compare(counts, baseline); if (increased.length > 0) { diff --git a/scripts/check-move.mjs b/scripts/check-move.mjs index c5aa8e5e..8448082f 100644 --- a/scripts/check-move.mjs +++ b/scripts/check-move.mjs @@ -16,7 +16,10 @@ * none of the base branch's modules -- a new `javaTrim` beside a moved * caller would quietly replace the one it called; * - rebound: a name a moved declaration imports from a module whose - * declaration of it is not the one the base branch's module had. + * declaration of it is not the one the base branch's module had; or any + * name a moved declaration uses that is bound to something else where it + * is now -- a receiving module's own declaration of that name, or a + * global where the original had a declaration or an import. * * node scripts/check-move.mjs [base] # base defaults to origin/main * @@ -128,10 +131,14 @@ function pieces(file, text) { const effects = []; // specifiers imported for their side effects const reexports = new Map(); // exported name -> [specifier, name there] const stars = []; // specifiers of `export * from` + const uses = new Map(); // declaration or endpoint key -> the names it uses const add = (map, key, node) => { const list = map.get(key) ?? []; list.push(canonical(sf, node)); map.set(key, list); + const used = uses.get(key) ?? new Set(); + for (const name of namesUsed(node)) used.add(name); + uses.set(key, used); }; const endpointsIn = (list) => { for (const element of list.elements) { @@ -206,7 +213,74 @@ function pieces(file, text) { ); } } - return { declarations, endpoints, imports, effects, reexports, stars }; + return { declarations, endpoints, imports, effects, reexports, stars, uses }; +} + +/** + * The names a node refers to from outside itself: every identifier but a + * property's name after a dot, a key in an object or class, and any name + * the node declares within it (a local, a parameter: those bind the same + * wherever the node is). The node's own name is left to the caller. + */ +function namesUsed(node) { + const names = new Set(); + const declared = new Set(); + const visit = (n) => { + if (ts.isIdentifier(n)) { + const p = n.parent; + const isNameOf = + (ts.isPropertyAccessExpression(p) && p.name === n) || + (ts.isQualifiedName(p) && p.right === n) || + ((ts.isPropertyAssignment(p) || + ts.isMethodDeclaration(p) || + ts.isPropertyDeclaration(p) || + ts.isPropertySignature(p) || + ts.isMethodSignature(p) || + ts.isGetAccessorDeclaration(p) || + ts.isSetAccessorDeclaration(p) || + ts.isEnumMember(p)) && + p.name === n) || + (ts.isBindingElement(p) && p.propertyName === n) || + ((ts.isVariableDeclaration(p) || + ts.isParameter(p) || + ts.isFunctionDeclaration(p) || + ts.isClassDeclaration(p) || + ts.isInterfaceDeclaration(p) || + ts.isTypeAliasDeclaration(p) || + ts.isBindingElement(p)) && + p.name === n); + if (!isNameOf) names.add(n.text); + else if ( + n !== node.name && + (ts.isVariableDeclaration(p) || + ts.isParameter(p) || + ts.isFunctionDeclaration(p) || + ts.isClassDeclaration(p) || + ts.isBindingElement(p)) + ) + declared.add(n.text); + } + ts.forEachChild(n, visit); + }; + visit(node); + // A top-level `const x = …` declares its own name, which is no local. + if (ts.isVariableStatement(node)) + for (const d of node.declarationList.declarations) + if (ts.isIdentifier(d.name)) declared.delete(d.name.text); + return [...names].filter((name) => !declared.has(name)); +} + +/** + * What a name is bound to at the top of a module: its declaration there, + * what it imports (followed to a declaration), or a global. + */ +function bindingOf(modules, file, name) { + const module = modules.get(file); + if (module?.declarations.has(name)) + return `declared: ${module.declarations.get(name).join('\n')}`; + if (module?.imports.has(name)) + return `imported: ${identity(modules, ...module.imports.get(name))}`; + return 'global'; } /** @@ -339,6 +413,36 @@ for (const file of receivers) { ); } +// Every name a declaration or endpoint uses must be bound now as it was: +// a moved one's, as in the module it came from -- a receiving module's own +// `javaTrim` would quietly replace the one a moved caller called, and a +// helper left behind unimported would fall through to a global of the same +// name (`close`) -- and one that stayed, as where it is, so its caller is +// not pointed at another module's `javaTrim` in the same change. +const stayed = []; +for (const [file, was] of before) + for (const kind of ['declarations', 'endpoints']) + for (const key of was[kind].keys()) + if (after.get(file)?.[kind].has(key)) stayed.push([`${file}#${key}`, file]); +for (const [where, from] of [...origins, ...stayed]) { + const at = where.indexOf('#'); + const to = where.slice(0, at); + const key = where.slice(at + 1); + const kind = before.get(from).declarations.has(key) ? 'declarations' : 'endpoints'; + for (const name of after.get(to).uses.get(key) ?? []) { + const was = bindingOf(before, from, name); + const is = bindingOf(after, to, name); + if (was === is) continue; + // What it imported may have moved too: the same declaration, now here. + if (was.startsWith('imported: ') && is === `declared: ${was.slice(10)}`) continue; + if (was.startsWith('declared: ') && is === `imported: ${was.slice(10)}`) continue; + if (kind === 'declarations' && name === key) continue; // its own name + problems.push( + `rebound: ${name}, used by ${key} in ${to}, is ${is.split(':')[0]} there; in ${from} at ${base} it was ${was.split(':')[0]}${was.split(':')[0] === is.split(':')[0] ? ' as something else' : ''}` + ); + } +} + const count = (prefix) => problems.filter((p) => p.startsWith(prefix)).length; console.log( `moved ${moved.declarations} declarations and ${moved.endpoints} endpoints, ` + diff --git a/scripts/check-types.mjs b/scripts/check-types.mjs index f9d702b5..2219568b 100644 --- a/scripts/check-types.mjs +++ b/scripts/check-types.mjs @@ -111,6 +111,11 @@ if (up.length) { } const total = Object.values(counts).reduce((sum, n) => sum + n, 0); console.log(` tools/ with noImplicitAny: ${total} left, none new`); -if (down.length) +// A fall not locked in is room a later change could fill unseen: the +// baseline would not move, and nothing would say the count rose again. +if (down.length) { console.log(` ${down.length} module(s) went down; lock it in: npm run check:types -- --update`); + for (const [file, n] of down) console.log(` ${file}: ${n} -> ${counts[file] ?? 0}`); + process.exit(1); +} process.exit(0); diff --git a/specs/014-content-service-modules/contracts/boundaries.md b/specs/014-content-service-modules/contracts/boundaries.md index 512e2a29..ebc95dbf 100644 --- a/specs/014-content-service-modules/contracts/boundaries.md +++ b/specs/014-content-service-modules/contracts/boundaries.md @@ -4,18 +4,20 @@ The service's HTTP answers are not a contract this feature changes. Every one st The contract here is between the modules: -| Module | May import | -| -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------- | -| `java-text` | the services' existing leaves (`java-hashmap`, `schema`, …) | -| `body` | `java-text` | -| `spring` | `java-text`, `body` | -| `answers` | `java-text`, `spring` | -| `objects` | the above, `graph`, `domain` | -| `reference-entities`, `search-params`, `release` | the above, `solr` | -| a family (`-endpoints`: data-query, pathways-events, content, person, entities, mapping-species, schema, interactors, exporters, citation, search, search-diagram) | any shared module; **never another family, never the registry** | -| `registry` | the families | -| `server` | the registry, the shared modules | +| Module | May import | +| -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------- | +| `java-text` | the services' existing leaves (`java-hashmap`, `schema`, …) | +| `body` | `java-text` | +| `spring` | `java-text`, `body` | +| `answers` | `java-text`, `spring` | +| `objects` | the above, `graph`, `domain` | +| `reference-entities`, `search-params`, `release` | the above, `solr` | +| a family (`-endpoints`: data-query, pathways-events, content, person, entities, mapping-species, schema, interactors, exporters, citation, search, search-diagram) | any shared module, any leaf; **never another family, never the registry** | +| `registry` | the families | +| `server` | the registry, the shared modules | A test reads each module's imports and fails on any import this table does not allow, naming the module and the import. It is shown failing first: make one family import another. +A **leaf** is any module of the service not in the table -- `graph`, `solr`, `domain`, `schema`, `settings`, `endpoint` (the `Endpoint` types) and the like, and any module added without a row -- and imports only leaves. Every shared module may import leaves besides the shared modules its row names: `spring` reads `domain`'s views, for one. So a family, or `answers`, declares `Endpoint[]` or returns a `Handler` from `endpoint.ts`, never from the registry. The two tools that read the endpoint list, `diff` and `invalid-inputs`, may import anything, and nothing in the service imports them. + Every family module is named `-endpoints.ts`, because several family names are taken already: `schema.ts` (the model's class table) and `citation.ts` (the citation formatter) stay as they are. diff --git a/specs/014-content-service-modules/contracts/checks.md b/specs/014-content-service-modules/contracts/checks.md index a6bc1d91..e357c5df 100644 --- a/specs/014-content-service-modules/contracts/checks.md +++ b/specs/014-content-service-modules/contracts/checks.md @@ -16,9 +16,12 @@ It fails with a line for each difference: changed: searchEntry (now in search-endpoints.ts) differs from service.ts at missing: facetMapping (in service.ts at , nowhere now) added: javaTrim (in body-endpoints.ts: not there at , nor moved in) -rebound: javaTrim in body.ts is jsoup-text.ts's, not the one service.ts had at +rebound: javaTrim in body.ts is not the javaTrim service.ts had at +rebound: javaJson, used by pageText in paging.ts, is global there; in service.ts at it was declared ``` +A `rebound` line covers both an import that resolves to something else and any name a moved declaration uses that is bound differently where it now is: the receiving module's own declaration of that name, or a global where the original module had a declaration or an import. Names the moved code declares itself (locals, parameters) are not compared. The same holds for code that stays where it was: a caller left in `service.ts` that now reaches another module's helper of the same name is `rebound` too. + ## Baseline totals (`node scripts/check-baseline-totals.mjs [base]`) It passes with: @@ -30,9 +33,14 @@ loose (base ) · implicit any (base ) · lint (base ) It fails with: ```text -loose rose: , base — a move keeps totals; typing lowers them +loose rose: , base -- a move keeps totals; typing lowers them +loose in tools/content-node/a.ts rose: 6, base 5 -- a move keeps totals; typing lowers them ``` +A module the base branch already counts may not rise, even if another falls, unless it is one the split creates to receive moved code (the shared modules, the families, the registry, the server), which a later move may add to. Code never moves into the services' other modules, so a rise in one of those is new code. The split's modules are held by the total. + +The per-module checks (`check:types` for implicit `any`, `check:lint` for loose values) also fail on a fall that is not locked in with `--update`, so room left by typing cannot be spent later without the baseline moving. + ## Route order (a spec over the registry) It fails when the list differs from the recorded one, or when one of the 15 named pairs is out of order: diff --git a/specs/014-content-service-modules/plan.md b/specs/014-content-service-modules/plan.md index bbb1eef4..6a9f97e4 100644 --- a/specs/014-content-service-modules/plan.md +++ b/specs/014-content-service-modules/plan.md @@ -77,7 +77,8 @@ specs/014-content-service-modules/ tools/content-node/ ├── service.ts # shrinks to nothing as families move; then removed (the image's CMD runs server.ts) ├── server.ts # start-up, routing, path rules, download guard, failure answers, /health -├── registry.ts # Endpoint type; the families' lists, in today's order +├── endpoint.ts # the Endpoint, Handler, Answer and Request types (a leaf) +├── registry.ts # the families' lists, in today's order ├── release.ts # the release, answered once (R2) ├── java-text.ts body.ts spring.ts answers.ts objects.ts # shared, layered (R1) ├── reference-entities.ts search-params.ts # shared by two families @@ -108,7 +109,7 @@ deploy/content-node/Dockerfile # CMD → server.ts, once service.ts is gone Each is seen to fail. 2. **The release without the list**: the one logic edit; `release.ts` itself comes with layer 6. Verified on beta like a move. -3. **The `Endpoint` type and `registry.ts`**, in the same pull request as step 2, with `service.ts`'s list typed as `Endpoint[]` in place. The casts in the tools go. +3. **The `Endpoint` type** (`endpoint.ts`, a leaf, so families and `answers` can use it without importing the registry), in the same pull request as step 2, with `service.ts`'s list typed as `Endpoint[]` in place. The casts in the tools go. 4. **Shared modules**, bottom layer first: `java-text`, `body`, `spring`, `answers`, `objects`, then the pair-shared two. Each is a move pull request. 5. **Families**, least coupled first, one pull request each: 1. citation diff --git a/specs/014-content-service-modules/research.md b/specs/014-content-service-modules/research.md index 093d625c..09b4c04c 100644 --- a/specs/014-content-service-modules/research.md +++ b/specs/014-content-service-modules/research.md @@ -124,7 +124,7 @@ Each runs `endpoints.find(... '/data/database/version').handler(...)`. Once move Eight handlers carry `warms`. The parameter record (`parameters.ts` `RECORD`) is keyed by path and stays separate. It is shared with the cache labels and the invalid-input corpus, and folding it into the endpoint would tie three modules to one shape. -**Decision.** One `Endpoint` type in `registry.ts` with exactly these fields (see data-model.md). Each family declares its list as `Endpoint[]`. The casts in `diff.ts`, `invalid-inputs.ts`/`.spec.ts` and `parameters.spec.ts` go. +**Decision.** One `Endpoint` type in `endpoint.ts` (types only, a leaf: families and `answers` use it, and none may import the registry) with exactly these fields (see data-model.md). Each family declares its list as `Endpoint[]`. The casts in `diff.ts`, `invalid-inputs.ts`/`.spec.ts` and `parameters.spec.ts` go. ## R5. Ratchets across a move diff --git a/specs/014-content-service-modules/tasks.md b/specs/014-content-service-modules/tasks.md index 478b533e..8049d124 100644 --- a/specs/014-content-service-modules/tasks.md +++ b/specs/014-content-service-modules/tasks.md @@ -67,7 +67,7 @@ It must not mix in logic edits. **Independent Test**: a misspelled or mistyped endpoint field fails `check:types`, and `git grep` finds no cast on the endpoint list in the tools or specs. -- [x] T006 [US2] Add `tools/content-node/registry.ts` with the `Endpoint`, `Handler`, `Answer` and `Request` types (data-model.md, the 11 fields plus `warms` on the handler). `service.ts` types its list `Endpoint[]` in place, and `cached` returns a `Handler`. Show `sampel: '…'` on one endpoint failing the type check. +- [x] T006 [US2] Add the `Endpoint`, `Handler`, `Answer` and `Request` types (first as `tools/content-node/registry.ts`; renamed `endpoint.ts`, a types-only leaf, when the review of 20 Oct found families and `answers` need them and may not import the registry) (data-model.md, the 11 fields plus `warms` on the handler). `service.ts` types its list `Endpoint[]` in place, and `cached` returns a `Handler`. Show `sampel: '…'` on one endpoint failing the type check. Done with T005 in one PR: naming the version handler changed what TypeScript inferred for the untyped list, so the list had to be typed at the same time. The registry does not re-export the list yet, because nothing would read it there. `endpointFor` and `getEndpointFor` stay in `service.ts`, beside the server code that calls them; they move to the registry in T026, when it assembles the families and `service.ts` goes. `deploy/nginx/routes.spec.ts` and the route-order test move to the registry then too. — #570 diff --git a/tools/content-node/boundaries.spec.ts b/tools/content-node/boundaries.spec.ts index 77f72129..0e079747 100644 --- a/tools/content-node/boundaries.spec.ts +++ b/tools/content-node/boundaries.spec.ts @@ -10,17 +10,40 @@ import path from 'node:path'; import ts from 'typescript'; import { describe, expect, it } from 'vitest'; -/** The shared modules, by layer: a module imports only from earlier layers. */ -const LAYERS = [ - ['java-text'], - ['body'], - ['spring'], - ['answers'], - ['objects'], - ['reference-entities', 'search-params', 'release'], -]; -const layerOf = (name: string) => LAYERS.findIndex((layer) => layer.includes(name)); +/** + * The shared modules and what each may import besides the leaves + * (contracts/boundaries.md): each only those beneath it, as the table names + * them. + */ +const SHARED: Record = { + 'java-text': [], + body: ['java-text'], + spring: ['java-text', 'body'], + answers: ['java-text', 'spring'], + objects: ['java-text', 'body', 'spring', 'answers'], + 'reference-entities': ['java-text', 'body', 'spring', 'answers', 'objects'], + 'search-params': ['java-text', 'body', 'spring', 'answers', 'objects'], + release: ['java-text', 'body', 'spring', 'answers', 'objects'], +}; +/** + * The tools beside the service that read its endpoint list -- the parity + * harness and the invalid-input corpus: they may import anything, and + * nothing in the service may import them. + */ +const TOOLS = new Set(['diff', 'invalid-inputs']); const isFamily = (name: string) => name.endsWith('-endpoints'); +const isShared = (name: string) => name in SHARED; +/** + * Everything else is a leaf -- graph, solr, domain, schema, settings and the + * like, and any module added without a place here -- and imports only + * leaves: a leaf that reached the registry, a family or the service would + * close a cycle through whatever imports it. + */ +const isLeaf = (name: string) => + !isFamily(name) && + !isShared(name) && + !TOOLS.has(name) && + !['registry', 'server', 'service'].includes(name); /** * The local modules a source imports -- static, type-only, side-effect, @@ -47,19 +70,22 @@ function violations(modules: Record): string[] { for (const [name, source] of Object.entries(modules)) { for (const other of importsOf(source)) { const say = (why: string) => found.push(`${name} imports ${other}: ${why}`); - if (isFamily(name)) { + if (TOOLS.has(name)) continue; + if (TOOLS.has(other)) say('nothing in the service imports a tool'); + else if (isFamily(name)) { if (isFamily(other)) say('a family imports no other family'); - else if (['registry', 'server', 'service'].includes(other)) - say('a family imports only shared modules'); - } else if (layerOf(name) >= 0) { - const theirs = layerOf(other); - if (isFamily(other) || ['registry', 'server', 'service'].includes(other)) - say('a shared module imports only shared modules beneath it'); - else if (theirs >= layerOf(name)) - say(`layer ${layerOf(name) + 1} imports layer ${theirs + 1}`); + else if (!isShared(other) && !isLeaf(other)) say('a family imports only shared modules'); + } else if (isShared(name)) { + if (!isLeaf(other) && !(SHARED[name] ?? []).includes(other)) + say(`${name} imports only ${(SHARED[name] ?? []).join(', ') || 'leaves'} beneath it`); } else if (name === 'registry') { - if (other === 'server') say('the registry is read by the server, not the other way'); - } + if (!isFamily(other)) say('the registry imports only the families'); + } else if (name === 'server') { + if (isFamily(other) || other === 'service') + say('the server imports the registry, never a family'); + } else if (name === 'service') { + // The list's last home until T026 removes it: anything but a tool. + } else if (!isLeaf(other)) say('a leaf imports only leaves'); } } return found; @@ -108,9 +134,67 @@ describe('the content service boundaries', () => { ]); }); - it('refuse a shared module importing one above it', () => { - expect(violations({ spring: "import { javaJson } from './answers.ts';" })).toEqual([ - 'spring imports answers: layer 3 imports layer 4', + it('refuse a shared module importing one its row does not name', () => { + expect( + violations({ + spring: "import { javaJson } from './answers.ts';", + // body is beneath answers, but answers' row does not name it. + answers: "import { plainText } from './body.ts';", + objects: "import { read } from './graph.ts';\nimport { only } from './answers.ts';", + }) + ).toEqual([ + 'spring imports answers: spring imports only java-text, body beneath it', + 'answers imports body: answers imports only java-text, spring beneath it', + ]); + }); + + it('refuse a leaf reaching the registry, a family, the service or a tool', () => { + expect( + violations({ + graph: "import { endpoints } from './registry.ts';", + solr: "import { x } from './search-endpoints.ts';\nimport './service.ts';", + settings: "import { diffAll } from './diff.ts';", + // A module added without a place in the table is a leaf too. + 'new-helper': + "import { y } from './citation-endpoints.ts';\nimport { z } from './domain.ts';", + }) + ).toEqual([ + 'graph imports registry: a leaf imports only leaves', + 'solr imports search-endpoints: a leaf imports only leaves', + 'solr imports service: a leaf imports only leaves', + 'settings imports diff: nothing in the service imports a tool', + 'new-helper imports citation-endpoints: a leaf imports only leaves', + ]); + }); + + it('refuse the server importing a family, and the registry anything but families', () => { + expect( + violations({ + server: + "import { endpoints } from './registry.ts';\nimport { x } from './citation-endpoints.ts';\nimport { read } from './graph.ts';", + registry: + "import { a } from './citation-endpoints.ts';\nimport { read } from './graph.ts';", + }) + ).toEqual([ + 'server imports citation-endpoints: the server imports the registry, never a family', + 'registry imports graph: the registry imports only the families', ]); }); + + it('let families and shared modules take the Endpoint types from their leaf', () => { + expect( + violations({ + 'citation-endpoints': "import type { Endpoint } from './endpoint.ts';", + answers: "import type { Answer, Handler, Request } from './endpoint.ts';", + }) + ).toEqual([]); + }); + + it('let the tools read anything, the endpoint list included', () => { + expect( + violations({ + diff: "import { endpoints } from './service.ts';\nimport { x } from './registry.ts';", + }) + ).toEqual([]); + }); }); diff --git a/tools/content-node/registry.ts b/tools/content-node/endpoint.ts similarity index 84% rename from tools/content-node/registry.ts rename to tools/content-node/endpoint.ts index 07b4b149..06d98815 100644 --- a/tools/content-node/registry.ts +++ b/tools/content-node/endpoint.ts @@ -1,9 +1,10 @@ /** - * The endpoints the content service answers, and the shape each is declared - * in (spec 014, data-model.md). Until the families move out of service.ts - * the list is service.ts's, typed there as `Endpoint[]`; once they do, the - * registry assembles it here, and routing, endpointFor, the parity harness, - * the invalid-input corpus and the specs read it from here. + * The shape the content service's endpoints are declared in (spec 014, + * data-model.md): types only, a leaf every module may import. Families + * declare their lists as `Endpoint[]`, and `cached` (answers) returns a + * `Handler`, without importing the registry that assembles the families -- + * which would be a cycle. Until the families move out of service.ts the + * list is service.ts's, typed there in place. */ import type { Request as ExpressRequest } from 'express'; diff --git a/tools/content-node/invalid-inputs.spec.ts b/tools/content-node/invalid-inputs.spec.ts index 3e668a2c..b94cc6b2 100644 --- a/tools/content-node/invalid-inputs.spec.ts +++ b/tools/content-node/invalid-inputs.spec.ts @@ -17,17 +17,23 @@ const corpus = invalidInputs(GETS); describe('invalidInputs', () => { it('tries every endpoint that declares a parameter of its own, unless it is never stored', () => { - const tried = new Set(corpus.map((url) => url.split('?')[0] ?? '')); + // Each URL is the endpoint's that answers it: the first in the list whose + // template it fits, as routing takes it. Not any template it fits -- + // /search/diagram/summary fits /search/diagram/{diagram} too, and would + // have counted that endpoint tried when it never was. + const pattern = (path: string) => new RegExp(`^${path.replace(/\{[^}]+\}/g, '[^/]+')}$`); + const tried = new Set( + corpus.map((url) => { + const path = url.split('?')[0] ?? ''; + return GETS.find((endpoint) => pattern(endpoint.path).test(path))?.path; + }) + ); for (const endpoint of GETS) { const own = declaredQuery(endpoint.path).filter( (p) => !['view', 'includeRef'].includes(p.name) ); if (!own.length || RECORD[endpoint.path]?.cache) continue; - const pattern = new RegExp(`^${endpoint.path.replace(/\{[^}]+\}/g, '[^/]+')}$`); - expect([endpoint.path, [...tried].some((path) => pattern.test(path))]).toEqual([ - endpoint.path, - true, - ]); + expect([endpoint.path, tried.has(endpoint.path)]).toEqual([endpoint.path, true]); } }); diff --git a/tools/content-node/registry.spec.ts b/tools/content-node/registry.spec.ts index 7185ac00..ad0277a1 100644 --- a/tools/content-node/registry.spec.ts +++ b/tools/content-node/registry.spec.ts @@ -7,7 +7,8 @@ // comes first decides which answers. Splitting the service into modules must // not change it: the list is held to today's, recorded, and the overlapping // pairs to the fifteen measured. -import { readFileSync } from 'node:fs'; +import { readFileSync, readdirSync } from 'node:fs'; +import ts from 'typescript'; import { describe, expect, it } from 'vitest'; const { endpoints } = (await import('./service.ts')) as { @@ -98,27 +99,93 @@ const PAIRS_ANY_METHOD = [ * Reads of the endpoint list that pick one endpoint out by its path: a helper * reaching into the list for another endpoint's answer, as the release lookup * once did (research R2). A family module doing that would import the - * registry, which assembles the families: a cycle. A backstop for service.ts - * while it holds the list; boundaries.spec.ts holds the rule itself. + * registry, which assembles the families: a cycle. boundaries.spec.ts holds + * the rule on imports; this catches the lookup itself, in any module, however + * it is written: a list method whose callback reads `path` beside a path, + * endpointFor or getEndpointFor given a path, or a loop over the list that + * compares with one. */ function lookupsByPath(source: string): string[] { - return [ - ...source.matchAll( - /endpoints\s*\.\s*(?:find|filter|some|findIndex)\(\s*\(?\s*(\w+)[^)\n]*\)\s*=>\s*[^;\n]*\1\.path\s*(?:\.\s*(?:endsWith|startsWith|includes)\(|===|!==|==)/g - ), - ].map((match) => match[0]); + const sf = ts.createSourceFile('m.ts', source, ts.ScriptTarget.Latest, true); + const found: string[] = []; + // A path: any text with a '/' in it, a template's parts included, or a + // constant named for one (VERSION_PATH). + const isPath = (n: ts.Node) => + ((ts.isStringLiteral(n) || ts.isNoSubstitutionTemplateLiteral(n)) && n.text.includes('/')) || + ((ts.isTemplateHead(n) || ts.isTemplateMiddle(n) || ts.isTemplateTail(n)) && + n.text.includes('/')) || + (ts.isIdentifier(n) && /_?PATH$/.test(n.text)); + // The list: `endpoints`, a family's own (`dataQueryEndpoints`), or either + // reached through a module (`registry.endpoints`). + const isList = (n: ts.Node): boolean => + (ts.isIdentifier(n) && /endpoints$/i.test(n.text)) || + (ts.isPropertyAccessExpression(n) && /endpoints$/i.test(n.name.text)); + const has = (n: ts.Node, test: (m: ts.Node) => boolean): boolean => + test(n) || (ts.forEachChild(n, (child) => (has(child, test) ? true : undefined)) ?? false); + const readsPath = (n: ts.Node) => ts.isIdentifier(n) && n.text === 'path'; + const visit = (n: ts.Node) => { + if (ts.isCallExpression(n)) { + const callee = n.expression; + if ( + ts.isPropertyAccessExpression(callee) && + isList(callee.expression) && + n.arguments.some((argument) => has(argument, readsPath) && has(argument, isPath)) + ) + found.push(n.getText(sf)); + if ( + ts.isIdentifier(callee) && + ['endpointFor', 'getEndpointFor'].includes(callee.text) && + n.arguments[0] !== undefined && + isPath(n.arguments[0]) + ) + found.push(n.getText(sf)); + } + if ( + ts.isForOfStatement(n) && + isList(n.expression) && + has(n.statement, readsPath) && + has(n.statement, isPath) + ) + found.push(n.getText(sf).split('\n')[0] ?? ''); + ts.forEachChild(n, visit); + }; + visit(sf); + return found; } +/** The service's modules, the tools that read the list (diff, invalid-inputs) aside. */ +const MODULES = readdirSync(new URL('./', import.meta.url)) + .filter((file) => file.endsWith('.ts') && !/\.(spec|d)\.ts$/.test(file)) + .filter((file) => !['diff.ts', 'invalid-inputs.ts'].includes(file)); + describe('the endpoint list', () => { - it('is not read by a helper looking an endpoint up by its path', () => { - const source = readFileSync(new URL('./service.ts', import.meta.url), 'utf8'); - expect(lookupsByPath(source)).toEqual([]); - // What it is there to catch, as the release lookup read before R2. - expect( - lookupsByPath( - "const version = endpoints.find((e) => e.path.endsWith('/data/database/version'));" + it('is not read by a helper looking an endpoint up by its path, in any module', () => { + const found = MODULES.flatMap((file) => + lookupsByPath(readFileSync(new URL(file, import.meta.url), 'utf8')).map( + (lookup) => `${file}: ${lookup}` ) - ).toHaveLength(1); + ); + expect(found).toEqual([]); + }); + + it('sees a lookup however it is written', () => { + for (const lookup of [ + "const version = endpoints.find((e) => e.path.endsWith('/data/database/version'));", + "const v = endpoints.find((e) =>\n e.path.endsWith('/data/database/version'));", + "const v = endpoints.find(({ path }) => path === '/ContentService/data/database/version');", + "const v = endpoints.findLast(function (e) { return /version$/.test(e.path) || e.path === '/x'; });", + "const v = endpointFor('/ContentService/data/database/version');", + "for (const e of endpoints) if (e.path === '/ContentService/data/database/version') v = e;", + "const v = endpoints.find((e) => e.path.includes('database/version'));", + 'const v = endpoints.find((e) => e.path === VERSION_PATH);', + 'const v = endpoints.find((e) => e.path.endsWith(`${BASE}/data/database/version`));', + "const v = registry.endpoints.find((e) => e.path.endsWith('/version'));", + "const v = dataQueryEndpoints.find((e) => e.path.endsWith('/version'));", + ]) + expect([lookup, lookupsByPath(lookup)]).toEqual([lookup, [expect.any(String)]]); + // Reading the list for every endpoint is not a lookup. + expect(lookupsByPath('for (const e of endpoints) server.get(e.path, handle(e));')).toEqual([]); + expect(lookupsByPath('const n = endpoints.filter((e) => !e.method).length;')).toEqual([]); }); it('is matched in the recorded order', () => { diff --git a/tools/content-node/service.ts b/tools/content-node/service.ts index b1b66dd4..5369b6ab 100644 --- a/tools/content-node/service.ts +++ b/tools/content-node/service.ts @@ -27,7 +27,7 @@ import { read, configured, close } from './graph.ts'; import { settings } from './settings.ts'; import type { GraphNode, GraphRelationship, Properties, Row } from './graph.ts'; import type { MappedRow } from './domain.ts'; -import type { Answer, Endpoint, Handler, Request } from './registry.ts'; +import type { Answer, Endpoint, Handler, Request } from './endpoint.ts'; import type { Contributor, DoiPathway, diff --git a/tools/content-node/snapshot-files.spec.ts b/tools/content-node/snapshot-files.spec.ts index 02496560..7999b31c 100644 --- a/tools/content-node/snapshot-files.spec.ts +++ b/tools/content-node/snapshot-files.spec.ts @@ -56,8 +56,11 @@ const kinds = (tuple: unknown[]) => describe('the snapshot files, against their types', () => { it("domain-snapshot.json's tuples have their types' lengths and kinds", () => { - for (const property of files.domain.properties) + for (const property of files.domain.properties) { expect(kinds(property).join()).toMatch(/^string,string,number(,string)?$/); + // The flag, when there, is the one value the type allows. + if (property.length > 3) expect([property, property[3]]).toEqual([property, 'D']); + } for (const field of files.domain.fields) expect(kinds(field).join()).toMatch( /^string,string$|^string,string,string,string,string,(string|null)$/ diff --git a/tools/loose-baseline.json b/tools/loose-baseline.json index 93d7f7e7..b2105abe 100644 --- a/tools/loose-baseline.json +++ b/tools/loose-baseline.json @@ -5,15 +5,13 @@ "tools/content-node/domain.spec.ts": 77, "tools/content-node/domain.ts": 358, "tools/content-node/graph.ts": 1, - "tools/content-node/invalid-inputs.spec.ts": 1, - "tools/content-node/invalid-inputs.ts": 4, "tools/content-node/java-hashmap.ts": 138, "tools/content-node/jsoup-text.ts": 4, - "tools/content-node/parameters.spec.ts": 8, + "tools/content-node/parameters.spec.ts": 2, "tools/content-node/parameters.ts": 48, "tools/content-node/psicquic.spec.ts": 1, "tools/content-node/report.ts": 3, - "tools/content-node/service.ts": 1677, + "tools/content-node/service.ts": 1529, "tools/content-node/snapshot-openapi.spec.ts": 5, "tools/content-node/snapshot-rfd.ts": 4, "tools/content-node/snapshot-schema.ts": 11,