diff --git a/.claude/lint-rules/missing_documentation.star b/.claude/lint-rules/missing_documentation.star index da8697dcb..745cf2da0 100644 --- a/.claude/lint-rules/missing_documentation.star +++ b/.claude/lint-rules/missing_documentation.star @@ -1,66 +1,249 @@ # Starlark Lint Rule: Missing Documentation # -# This rule checks that entities and microflows have documentation. -# Good documentation helps with maintainability and onboarding new developers. +# Undocumented model elements are invisible to `mxcli check` and to the build — +# nothing fails, so nothing reminds you. This rule is the reminder, and it +# covers every document type a user authors, not just the domain model. # -# Checks: -# - Entities should have a description explaining their purpose -# - Microflows should have a description explaining what they do +# Documents swept generically (one option each, all default True): +# Module, Entity, Page, Snippet, BuildingBlock, Layout, Enumeration, +# JavaScriptAction, ImageCollection, DataTransformer, Workflow, +# BusinessEventService, RestClient, PublishedRestService, Constant, +# JsonStructure, ImportMapping, ExportMapping # -# Entity properties: -# .description - Documentation text +# Handled separately, because they carry exemptions or children a uniform +# sweep cannot express: +# Microflow .description (nanoflows and trivial flows exempt) +# JavaAction .documentation +# JavaAction params .description <- the one Studio Pro shows a CALLER # -# Microflow properties: -# .description - Documentation text +# Members, off by default purely because of volume: +# Attribute .description +# Association .description +# +# Why Java action parameters default ON while attributes default OFF: an action +# has a handful of parameters and Studio Pro renders each description in the +# dialog where someone wires up the call — an undocumented parameter is a blank +# field next to a name like `pInput` at exactly the moment a caller has to +# decide what to pass. A domain model has hundreds of attributes and +# associations, so the same check there is a wall of text rather than a signal. +# +# Every kind is individually switchable; see the table in _DOC_KINDS and the +# options listed under docs-site/src/tools/starlark-rules.md. RULE_ID = "QUAL002" RULE_NAME = "Missing Documentation" -DESCRIPTION = "Entities and microflows should have documentation describing their purpose" +DESCRIPTION = "Model elements should have documentation describing their purpose" CATEGORY = "quality" SEVERITY = "info" +# kind (as emitted by documentable_elements) -> (option, noun, suggestion) +# +# A new Mendix document type is covered by adding a row in Go's +# documentableSources and a row here — not by writing another loop. +_DOC_KINDS = { + "Module": ( + "check_modules", + "Module", + "Document what the module is for: it is the first thing a newcomer opens.", + ), + "Entity": ( + "check_entities", + "Entity", + "Add a description explaining the entity's purpose and what data it represents.", + ), + "Page": ( + "check_pages", + "Page", + "Describe what the page shows and who reaches it.", + ), + "Snippet": ( + "check_snippets", + "Snippet", + "Describe what the snippet renders and what context it expects, since it is reused across pages.", + ), + "BuildingBlock": ( + "check_building_blocks", + "Building block", + "Describe what the building block is for: it exists to be dropped in by someone who did not write it.", + ), + "Layout": ( + "check_layouts", + "Layout", + "Describe the layout's intended use and its placeholders.", + ), + "Enumeration": ( + "check_enumerations", + "Enumeration", + "Describe what the enumeration models, especially where the values map to something external.", + ), + "JavaScriptAction": ( + "check_javascript_actions", + "JavaScript action", + "Document what the action does and what it returns. Like a Java action, its body is code the model cannot show a reader.", + ), + "ImageCollection": ( + "check_image_collections", + "Image collection", + "Describe what the collection is for and where its images are used.", + ), + "DataTransformer": ( + "check_data_transformers", + "Data transformer", + "Describe the transformation applied and the shape it expects.", + ), + "Workflow": ( + "check_workflows", + "Workflow", + "Describe the process the workflow models and who its user tasks are for.", + ), + "BusinessEventService": ( + "check_business_event_services", + "Business event service", + "Document the events published or consumed, since other applications depend on them.", + ), + "RestClient": ( + "check_rest_clients", + "REST client", + "Document which external service is consumed and what it is used for.", + ), + "PublishedRestService": ( + "check_published_rest_services", + "Published REST service", + "Document the contract: this is the description external consumers read.", + ), + "Constant": ( + "check_constants", + "Constant", + "Describe what the constant configures and what a valid value looks like — it is set per environment by someone who cannot see the code.", + ), + "JsonStructure": ( + "check_json_structures", + "JSON structure", + "Note which payload the structure was captured from.", + ), + "ImportMapping": ( + "check_import_mappings", + "Import mapping", + "Describe the source payload and what it maps onto.", + ), + "ExportMapping": ( + "check_export_mappings", + "Export mapping", + "Describe the target payload and what it is produced for.", + ), + "Association": ( + # Off by default with attributes: a real domain model has as many + # associations as entities, and none of them are documented. + "check_associations", + "Association", + "Add a description, or switch this off with `check_associations: false` if the names are self-describing here.", + ), +} + +# Kinds whose option defaults to False. Everything else defaults to True. +_OFF_BY_DEFAULT = {"check_associations": True} + +def _blank(text): + """True when a documentation field is absent or whitespace-only.""" + return not text or text.strip() == "" + +def _flag(violations, module, doc_type, doc_name, message, suggestion): + violations.append(violation( + message = message, + location = location( + module = module, + document_type = doc_type, + document_name = doc_name, + ), + suggestion = suggestion, + )) + def check(): - """ - Check that entities and microflows have documentation. - """ violations = [] - # Check entities - for entity in entities(): - if not entity.description or entity.description.strip() == "": - loc = location( - module=entity.module_name, - document_type="Entity", - document_name=entity.qualified_name - ) - v = violation( - message="Entity '{}' has no documentation.".format(entity.name), - location=loc, - suggestion="Add a description explaining the entity's purpose and what data it represents." + # ---- every document type, one sweep ------------------------------------- + for el in documentable_elements(): + entry = _DOC_KINDS.get(el.kind) + if entry == None: + # A kind Go knows about but this table does not. Staying silent is + # right: a rule inventing a message for an element it cannot + # describe is worse than not reporting it. + continue + option, noun, suggestion = entry + if not get_option(option, not _OFF_BY_DEFAULT.get(option, False)): + continue + if _blank(el.description): + _flag( + violations, + el.module_name, + el.kind, + el.qualified_name, + "{} '{}' has no documentation.".format(noun, el.name), + suggestion, ) - violations.append(v) - # Check microflows (skip nanoflows as they're often simple) - for mf in microflows(): - # Only check microflows, not nanoflows - if mf.microflow_type != "MICROFLOW": - continue + # ---- microflows: exempt nanoflows and trivial flows --------------------- + if get_option("check_microflows", True): + # Nanoflows are excluded: they are usually a couple of client-side steps + # whose name says everything a description would. + min_activities = get_option("min_activities", 3) + for mf in microflows(): + if mf.microflow_type != "MICROFLOW": + continue + if mf.activity_count < min_activities: + continue + if _blank(mf.description): + _flag( + violations, + mf.module_name, + "Microflow", + mf.qualified_name, + "Microflow '{}' has no documentation.".format(mf.name), + "Add a description explaining what this microflow does and when it should be called.", + ) - # Skip very simple microflows (1-2 activities) - if mf.activity_count <= 2: - continue + # ---- Java actions and their parameters ---------------------------------- + check_actions = get_option("check_java_actions", True) + check_params = get_option("check_java_action_params", True) + if check_actions or check_params: + for ja in java_actions(): + if check_actions and _blank(ja.documentation): + _flag( + violations, + ja.module_name, + "JavaAction", + ja.qualified_name, + "Java action '{}' has no documentation.".format(ja.name), + "Add documentation explaining what the action does, and what it returns. " + + "Unlike a microflow, its body is Java that the model cannot show a reader.", + ) + if not check_params: + continue + for p in ja.parameters: + if _blank(p.description): + _flag( + violations, + ja.module_name, + "JavaAction", + ja.qualified_name, + "Java action parameter '{}.{}' has no description.".format(ja.name, p.name), + "Add a description: Studio Pro shows it to whoever wires up the call, " + + "where the parameter name is all they otherwise have to go on.", + ) - if not mf.description or mf.description.strip() == "": - loc = location( - module=mf.module_name, - document_type="Microflow", - document_name=mf.qualified_name - ) - v = violation( - message="Microflow '{}' has no documentation.".format(mf.name), - location=loc, - suggestion="Add a description explaining what this microflow does and when it should be called." - ) - violations.append(v) + # ---- entity attributes (off by default: high volume) -------------------- + if get_option("check_attributes", False): + for entity in entities(): + for attr in attributes_for(entity.qualified_name): + if _blank(attr.description): + _flag( + violations, + entity.module_name, + "Entity", + entity.qualified_name, + "Attribute '{}.{}' has no documentation.".format(entity.name, attr.name), + "Add a description, or switch this off with `check_attributes: false` if " + + "attribute names are self-describing in this project.", + ) return violations diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 0e44661ed..1910e45ae 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -443,3 +443,9 @@ extracting `OffsetExpression`/`LimitExpression`. | MDL cannot declare an OData action. `publish microflow …`, `publish action …` and every variant is a parse error at `missing ENTITY`, so a service's `$metadata` has an `EntityContainer` with `EntitySet` elements and no `ActionImport` — and every parameterised resource has to be modelled as an entity set that echoes its own arguments back as columns | `createODataServiceStatement` admitted `publishEntityBlock*` and nothing else. Unlike the custom-auth gap the write path was missing too: neither `sdk/mpr/writer_odata.go` nor `mdl/backend/modelsdk/odata_write.go` had a single reference to `PublishedMicroflow`, though `modelsdk/gen/odatapublish` has `NewPublishedMicroflow()`/`NewPublishedMicroflowParameter()` with every setter wired | `mdl/grammar/domains/MDLService.g4` (`publishMicroflowBlock`, reusing `exposeClause`), `mdl/ast/ast_odata.go`, `mdl/visitor/visitor_odata.go`, `model/types.go` (`PublishedMicroflow`/`…Parameter`), `mdl/executor/cmd_odata_actions.go` (resolve the microflow, derive types), `sdk/mpr/writer_odata.go` + `parser_odata.go`, `mdl/backend/modelsdk/odata_write.go` + `odata_read_detail.go` + `integration_read.go`, `mdl/executor/cmd_odata.go` (`printPublishedMicroflowMDL`) | **With no Studio Pro reference available, derive every shape from something already proven rather than stopping.** Property names/kinds came from the generated metamodel; the `Module.Microflow.Param` form of the `MicroflowParameter` by-name ref from `published_rest_write.go`, which already ships it; the `DataTypes$ObjectType`/`ListType` `Entity` key and `EnumerationType` `Enumeration` key from `serializeMicroflowDataType`. **Do not restate what the model already declares** — parameter types and the return type are read off the microflow, so they cannot drift. Store a data type as **kind + ref, never one string**: `Module.X` cannot say whether X is an entity or an enumeration. **`mx check` reporting 0 errors is near-worthless for a NEW element type** (mxbuild ignores unknown properties); the test that settles it is to delete the referent — dropping the microflow makes mxbuild answer `CE1613 … at Published microflow parameter DriverId from published microflow RecordPrediction`, naming the elements back in its own vocabulary. **A test that calls the emitter helper directly does not prove the emitter is wired**: reverting the loop in `outputPublishedODataServiceMDL` left every `printPublishedMicroflowMDL` test green, so the DESCRIBE test goes through the real entry point. Also note the replay control does NOT reproduce a missing emitter, because `create or replace` preserves microflows when the statement supplies none — the defect is an incomplete description, not in-place loss. Tests `mdl/visitor/visitor_odata_action_test.go`, `mdl/executor/odata_auth_microflow_test.go`; example `mdl-examples/doctype-tests/10-odata-examples.mdl`. mxcli-formula1 §47.1 | | A service that correctly declares `TopSupported: No` cannot be consumed: `sql generate connector` / contract import produces an app whose external entity says `Supports $top = True`, and the **consumer** build fails **CE6630** "'Seasons' is marked supports $top=False in the OData service, but True in the app". The publisher is right, the contract is right, and only the generated consumer is wrong | Two layers, and the first hid the second. `applyCapabilityAnnotations` began with `if ann.Record == nil { continue }` — but `TopSupported`/`SkipSupported` are **standalone boolean terms** (``), not the record shape that `InsertRestrictions` and friends use, so the parser discarded them and `EdmEntitySet` had no field to hold them. `applyExternalEntityFields` then hardcoded `ent.SkipSupported = true; ent.TopSupported = true` — directly beside `Countable`, which *was* derived, and under a comment explaining the CE6630 mechanism the hardcode went on to trigger | `mdl/types/edmx.go` (`EdmEntitySet.TopSupported`/`SkipSupported`, `xmlCapabilitiesAnnotation.Bool`, standalone branch in `applyCapabilityAnnotations`), `mdl/executor/cmd_contract.go` (`applyExternalEntityFields`) | **The OData capabilities vocabulary has two annotation shapes, and a parser written against one silently drops the other.** Record terms (`InsertRestrictions`, `CountRestrictions`) wrap a ``; scalar terms (`TopSupported`, `SkipSupported`) carry `Bool` on the annotation element itself. The `Record == nil` guard read as "malformed, skip" when it actually meant "different, valid shape" — grep any capability parser for that guard before adding a term. **Absent must stay `true`, not `false`**: OData's own default is supported, so a tri-state `*bool` resolved as `p == nil || *p` is load-bearing — defaulting to false would invert CE6630 for every unannotated service, i.e. every service that worked before the fix. **This is the prerequisite for MDL-ODATA03's advice being followable**: the rule tells an author to declare `TopSupported: No` on a read-microflow resource, which was previously a trap that broke consumers. A fixture for this is worth capturing from a live `$metadata` rather than hand-written — the standalone shape is what made the bug, so a hand-built record-shaped fixture would have tested the wrong thing. Tests `mdl/types/edmx_test.go` (`TestParseEdmx_StandaloneTopAndSkipSupported`, plus a control proving record terms still parse), `mdl/executor/cmd_contract_capabilities_test.go`. mxcli-formula1 §42 | | `MDL-ODATA03` ("advertises TopSupported … so no paging is applied") is silenced by giving the read microflow a `System.HttpRequest` parameter — even when the parameter was added for the **KEY** and no paging was implemented. The half-fixed resource, which is the one still shipping unpaged 200s, is exactly the one the rule stops warning about | MDL-ODATA02 (KEY) and MDL-ODATA03 (paging) shared a single test — `takesHTTPRequest(decl)` — and answering either concern satisfied both. The rule could see whether the microflow *could* read the request, not whether it *did* | `mdl/executor/validate_odata_read_contract.go` (`capabilityViolations` now takes the declaration; `pagingEvidence`/`walkForPaging` reflect-walk the reachable body), `mdl-examples/doctype-tests/10-odata-examples.mdl` | **When one input satisfies two rules, the rules will silence each other.** The tell is a shared precondition that means something different to each check — grep for other validators keyed on the same `takesX()` helper before adding one. **The wire spelling is the evidence.** An OData query option is literally named `$top`, so a microflow implementing it must contain that text; presence of the text is weak evidence of handling, absence is strong evidence against — which is the only direction a rule that reports absences needs. **Reflect-walk, do not type-switch per statement**: MDL gains activity types regularly and a walker that misses one converts a false negative into a false *positive* ("nothing reads $top" about a body it never looked at). Reflection also picks up new nesting — loop bodies, if-branches, error handlers — for free. **Exclude `Annotations`/`Documentation`/`Comment` fields from the walk**: an `@annotation '$top is not applied here'` is prose admitting the defect, and counting it as implementation silences the rule on the one resource that documents the bug. **Keep an escape hatch and make it outrank the evidence**: a call into a Java action, a JavaScript action, a nanoflow, or a microflow the script does not define ends the analysis with silence — same stance the rule already took toward out-of-script read microflows. Follow calls into helpers the script *does* define, since real code factors paging out. **Report per option, not per resource** — flagging `SkipSupported` while `$top` is genuinely handled avoids advising an author to withdraw working support. **Shipped examples are part of the blast radius**: the doctype example's variant (a) was written to the old rationale ("taking `$Request` is enough to earn its silence") and started warning the moment the rule tightened — a changed rule needs `mxcli check` over `mdl-examples/` and then the integration gate, not just its own unit tests. Each guard has its own revert control (restore benefit-of-the-doubt / drop the annotation skip / drop `opaque` / stop following helpers), and all four reproduce. Tests `mdl/executor/validate_odata_read_contract_test.go`. mxcli-formula1 §42 | +| `mxcli run --local` refuses to boot — *"port 6643 is already in use — a previous 'mxcli run --local' … is likely still serving on it"* — and names a process the user already killed. The guard is right to refuse; the diagnosis is a guess, and the recovery it suggests is a three-command `pgrep` hunt whose own pattern (`pgrep -f 'mxcli run'`) matches the shell it is typed into | Two different situations printed one message. mxcli **does** reap its children on Ctrl-C/SIGTERM (`procgroup_unix.go` puts mxbuild's JVM, the runtime and the bundler each in their own process group and signals the group), so a held port after a graceful stop is a *foreign* listener. But `kill -9`, a crash, or a reaped container runs no handler at all, and then it really is the previous run's orphan. The guard could not tell them apart because it never asked who held the port | `cmd/mxcli/docker/portowner_linux.go` (`listenerOnPort`, `parseListeningInodes`, `pidForSocketInode`), `portowner_other.go` (non-Linux stub), `portguard.go` (`portCulpritAdvice`), wired into `checkTargetPortsFree` (`runlocal.go`) and `checkLocalAppPortsFree` (`localapp.go`) | **Resolve the port owner from `/proc`, never by shelling out to `lsof`/`ss`** — both are routinely absent from slim containers, and needing a subprocess to explain why a boot failed is its own failure mode. `/proc/net/tcp{,6}` gives the socket inode; scanning `/proc//fd` for `socket:[]` gives the pid. **Read `tcp6` as well as `tcp`**: a JVM binding "localhost" lands on `[::1]` or dual-stack `[::]` as often as `127.0.0.1`, and its local_address is a 128-bit hex blob, not the 32-bit one. **Filter on state `0A` (TCP_LISTEN)** — an ESTABLISHED socket on that port is a *client*, and naming it would tell the user to kill their own browser. **Report the two cases differently**: a leftover of a previous run gets `kill `, a foreign listener gets "pick another port" and no kill suggestion, because offering to kill a stranger's process is worse advice than the vague message it replaced. Keep the guard **detection-only** — reaping someone else's process stays the user's call. **Three test traps hit in a row here.** (1) The live-IPv6 test only ever *skipped* (no IPv6 in the container), so the tcp6 parser was effectively untested — assert against captured kernel output instead. (2) A parser test calling `parseListeningInodes` directly does not prove `listeningInodes` *opens* both files: deleting `/proc/net/tcp6` from the list left every test green. (3) Making the list a `var` so a test can point it at fixtures then means the test no longer covers the **shipped** default either — assert the default's contents before overriding it. Only the third version of that control failed. The best test in the file is the cheapest: bind a port in the test process and demand `listenerOnPort` return `os.Getpid()` — it cannot pass by accident. Tests `cmd/mxcli/docker/portowner_linux_test.go`. mxcli-formula1 suggested issue 8 | +| `create external entities from` a service where an entity set is wholly unfilterable/unsortable generates an app the contract contradicts: **28 × CE6630** on one service — "'message' is marked Sortable=False in the OData service, but True in the app". The publisher is right, the `$metadata` is right, only the generated consumer is wrong | `FilterRestrictions`/`SortRestrictions` have **two shapes**, and only one was read. `applyCapabilityAnnotations` pulled `NonFilterableProperties` out of the record and ignored the record's own `Bool` property value, so `Filterable: !nonFilterable[p.Name]` evaluated `true` for every property of a set that had declared nothing filterable. This is the §42 Top/Skip bug one layer along — same vocabulary, same two-shapes cause, different term | `mdl/types/edmx.go` (`EdmEntitySet.Filterable`/`Sortable`, the `Filterable`/`Sortable` cases in `applyCapabilityAnnotations`, and the `AttrFilterable`/`AttrSortable` accessors), `mdl/executor/cmd_contract.go` (`createExternalEntities` calls the accessors) | **Mendix picks the annotation shape by arithmetic, not preference.** It lists `NonFilterableProperties` when SOME attributes are filterable and emits a bare `` when NONE are, because then there is no list to write — so both shapes appear in **one document** on different entity sets, and an entity exposing only a KEY reliably produces the whole-set form. A fixture with just the list shape tests the half that already worked. **After fixing one term in a vocabulary, sweep the siblings**: §42 fixed Top/Skip by teaching the parser about standalone booleans and stopped there; Filterable/Sortable had the same defect and were found only because a consumer app failed to build. Grep the whole `applyCapabilityAnnotations` switch for terms whose Bool is never read. **Put both shapes behind one accessor** (`AttrFilterable`) rather than two fields the caller must remember to AND together — the caller then cannot consult one and forget the other, which is exactly how this arose, and the nil-safe receiver deletes the `entitySet != nil` dance at the call site. Absent still means `true`: OData's default is allowed, and defaulting to false would invert CE6630 for every unannotated service. Tests `mdl/types/edmx_test.go` (`TestParseEdmx_WholeSetFilterAndSortRestrictions`, plus a control proving the list shape still parses); both controls reproduce (ignore the record Bool → the whole-set test fails with the CE6630 message; drop the check inside `AttrFilterable` → same). mxcli-formula1 §48 | +| A project is generated with no documentation anywhere — Java actions, Java action parameters, entities, microflows — and nothing says so. `mxcli check` and the build both pass, because documentation is never load-bearing | `QUAL002` existed but reached only entities and microflows. Java actions were not reachable from Starlark **at all** (no `java_actions()` builtin), and a Java action's parameters were not reachable from anywhere: the catalog stored a parameter *count* and discarded each parameter's `Description` | `mdl/catalog/tables.go` (`java_action_parameters_data` + view, `CatalogSchemaVersion` bump), `mdl/catalog/catalog.go` (`Tables()` registry), `mdl/catalog/builder_modules.go` (insert parameters), `mdl/linter/context.go` (`JavaActions()` with parameters attached), `mdl/linter/starlark.go` (`java_actions()` builtin, `javaActionToStarlark`), `.claude/lint-rules/missing_documentation.star` | **Adding a catalog table has three obligations beyond the `CREATE TABLE`.** (1) Bump `CatalogSchemaVersion` — otherwise every existing `.mxcli/catalog.db` keeps its old schema, the new query fails, the error is swallowed, and the rule silently reports *zero* of the new findings. Silent under-reporting from a stale cache is indistinguishable from a clean project. (2) Register the view in `Tables()` — `catalog_test.go` enforces this and will fail with "add CATALOG.X so SHOW CATALOG TABLES includes it"; let that test tell you rather than discovering it from a user. (3) Exclude Marketplace/System modules in the iterator's `WHERE COALESCE(m.Source,'') = ''`, as every sibling iterator does — reporting undocumented Community Commons actions buries the findings the user can act on. **Attach children to their parent in the projection, not as a second iterator**: a rule reporting an undocumented parameter must name the action it belongs to, and pairing them in `JavaActions()` saves every rule from re-joining. **Load the shipped `.star` from disk in tests** — an inlined copy proves the builtin works and says nothing about whether the rule uses it. **Make each target switchable via `get_option`** before widening a rule: attribute-level checks are hundreds of findings on a real domain model, so `check_attributes` defaults off while the rest default on. Watch the whitespace when writing revert controls for Starlark: a `perl` pattern with `\t` silently matches nothing in a space-indented `.star`, so the control passes and proves nothing — it was the second attempt that failed correctly. Tests `mdl/linter/starlark_javaactions_test.go`; five controls reproduce. mxcli-formula1 (missing documentation) | +| QUAL002 reports ~40 platform elements (`FileDocument`, `HttpRequest`, `System`) as undocumented, burying the handful of real findings; or a whole document type (pages, workflows, constants, REST services, mappings) is never reported at all no matter how undocumented it is | Two separate causes. (1) **Coverage**: QUAL002 only ever looked at entities and microflows — every other document type was unreachable from Starlark. (2) **The System leak**: `modules.Source` is `"Marketplace …"` for downloaded modules and **empty for System — exactly as it is for the user's own modules**, so the near-universal `WHERE COALESCE(m.Source,'') = ''` filter excludes Marketplace and lets all of System through | `mdl/linter/context.go` (`documentableSources`, `DocumentableElements()`, `notPlatformModule()`, `systemModuleID`), `mdl/linter/starlark.go` (`documentable_elements()` builtin), `.claude/lint-rules/missing_documentation.star` (`_DOC_KINDS`) | **Source alone does not exclude System** — only the sentinel Id `00000000-0000-0000-0000-000000000001` does (`modelsdk/meta.SystemModuleID`). Use `notPlatformModule(alias)` — **every** `LintContext` iterator now routes through it (Entities, Microflows, Pages, Enumerations, Constants, Snippets, Widgets, DatabaseConnections, JavaActions, DocumentableElements and all three `FindUnused` kinds), and `TestIterators_ExcludePlatformModules` fails if a new one is added without it. The leak was not confined to QUAL002: on a blank 9.24 project it inflated the whole run from 8 findings to 60 — CONV001 renaming System booleans, SEC001 demanding access rules on 38 System entities, DESIGN001 splitting `QueuedTask`, MPR003 splitting the System module itself. Adding the predicate can only ever narrow a result set, so the change cannot invent a finding; prove that by diffing full lint output before/after and asserting the ADDED set is empty. **One projection beats N builtins**: a rule that wants "every document" should get one `documentable_elements()` sweep driven by a table, so a new Mendix document type is two rows (one in `documentableSources`, one in `_DOC_KINDS`) rather than another builtin nobody remembers to call. **The documentation column is NOT uniform** — Mendix says `Documentation` for Java actions / REST / mappings / JSON structures and `Description` for everything else; assuming one spelling silently reports the other half as undocumented (a revert control confirmed 5 kinds go dark). **Test the catalog BUILDER, not just the query**: unit tests that INSERT rows directly cannot see that a builder never populates a column, which would flood the user with false positives — an end-to-end `exec` + `lint` on a real `.mpr` is what proved the constant/entity paths actually work. Three tables (`json_structures`, `import_mappings`, `export_mappings`) use `Id INTEGER PRIMARY KEY AUTOINCREMENT` while the rest use `Id TEXT PRIMARY KEY`, so a synthetic string id is a "datatype mismatch" on exactly those. Beware: MDL `COMMENT` and `DOCUMENTATION` are **different fields** on an entity — `CREATE ENTITY … COMMENT 'x'` does not set documentation, `ALTER ENTITY … SET DOCUMENTATION 'x'` does; a test using the wrong one looks exactly like a write-path bug. Tests `mdl/linter/starlark_javaactions_test.go`; controls reproduce for the sweep, each kind list, the doc-column split, Marketplace (both join shapes) and System | +| A `LintContext` iterator suddenly yields nothing and the test says "expected ModA entities to be yielded" — no SQL error, no log line, just an empty result | The iterator's query failed (typically `no such column`) and the iterator swallows it: `rows, err := ctx.db.Query(...); if err != nil { return }`. A hand-rolled test double whose schema has drifted from the real catalog view produces exactly this. Hit when platform filtering started reading `modules.Id` and `context_test.go`'s minimal `modules` table had only `(Name, Source)` | `mdl/linter/context.go` (the `if err != nil { return }` in every iterator), `mdl/linter/context_test.go` (`setupModuleFilterDB`) | Read the failure as **"the query broke"**, not "the filter is too strict" — the two are indistinguishable from the assertion text, and the second reading sends you rewriting correct logic. Confirm by running the query by hand against the fixture DB. Test doubles that hand-roll a `CREATE TABLE` instead of using `catalog.NewFromFile` drift silently from the real schema; prefer the real schema builder for anything that joins. The swallowed error is the root problem — an iterator that cannot run its query is not the same as one with no results, and today nothing distinguishes them | +| A lint run reports fewer findings than expected, or a rule that clearly should fire reports nothing — and there is no error, no warning, no log line | An iterator's catalog query failed and the failure was swallowed: `if err != nil { return }` inside an `iter.Seq[T]`, which has no error channel. The run looks successful because a linter's whole output is "here is what I found", and "found nothing" is what both a clean project and a dead query produce | `mdl/linter/context.go` (`QueryError`, `recordQueryError`, `QueryErrors`), `mdl/linter/linter.go` (`Linter.QueryErrors`), `cmd/mxcli/cmd_lint.go` (report + exit 1) | Iterators still degrade to "no rows" — one broken query must not take down the run — but they now **record** the failure, and `mxcli lint` prints it and exits 1, because silently passing CI on a run that could not read the model is the worst available outcome. All 34 sites are covered: `Query` + `return`, `Query` + `continue`, bare and inline `rows.Scan` forms, `return unused` (the non-bare return that a naive regex sweep misses — `TestQueryError_AllIteratorsReport` is what caught it), and the reader-backed `ListScheduledEvents`. **Dedupe on iterator+cause**: several rules iterate the same accessor, so one broken view is otherwise reported once per rule. Keying on the iterator alone instead hides a second, distinct failure in the same accessor. When adding an error message with a remedy, **run the remedy** — the first draft here suggested `mxcli lint --refresh`, a flag that does not exist; the shipped message names `.mxcli/catalog.db` and deleting it was verified to clear the error. Tests `mdl/linter/context_queryerror_test.go`; controls: no-op recorder (every iterator goes silent) and a single reverted iterator (only that one) | diff --git a/.claude/skills/mendix/odata-data-sharing.md b/.claude/skills/mendix/odata-data-sharing.md index 3b605b6f8..627322a13 100644 --- a/.claude/skills/mendix/odata-data-sharing.md +++ b/.claude/skills/mendix/odata-data-sharing.md @@ -542,12 +542,25 @@ publish entity Api.Row as 'Rows' ( ``` Declaring `No` is the read path's substitute for the `400` it cannot send. It is -also safe to declare: the contract carries `TopSupported`/`SkipSupported` as -standalone boolean annotations, and `sql generate connector` reads them, -so a consuming app generated from this service says `No` too. (Until mxcli -learned to read them it always generated `Yes`, and the consumer failed to build -with **CE6630** "'Rows' is marked supports $top=False in the OData service, but -True in the app" — if you hit that against an older mxcli, that is the cause.) +also safe to declare: mxcli reads these capabilities off the contract, so a +consuming app generated from this service says `No` too. (Until it learned to, +the generator always said `Yes`, and the consumer failed to build with **CE6630** +"'Rows' is marked supports $top=False in the OData service, but True in the app" +— if you hit that against an older mxcli, that is the cause.) + +**The capabilities vocabulary has two annotation shapes**, and it is worth knowing +which you are looking at when reading a `$metadata` by hand: + +| Capability | Shape | +|---|---| +| `TopSupported`, `SkipSupported` | standalone: `` | +| `Insertable`, `Updatable`, `Deletable`, `Countable` | a `` with a `Bool` property value | +| `Filterable`, `Sortable` | **either** — a record listing `NonFilterableProperties` when *some* attributes are filterable, or a bare `Bool="false"` on the record when *none* are | + +That last row is the trap. Mendix picks the shape by arithmetic (there is no list +to write when nothing is filterable), so both appear in one document on different +entity sets, and a service where an entity exposes only a KEY emits the whole-set +form. `mxcli check` enforces this as **MDL-ODATA03**, and it looks for the option *names* in the microflow body, not just for a `System.HttpRequest` parameter — diff --git a/.claude/skills/mendix/run-local.md b/.claude/skills/mendix/run-local.md index 626f3a132..c9513672a 100644 --- a/.claude/skills/mendix/run-local.md +++ b/.claude/skills/mendix/run-local.md @@ -107,13 +107,26 @@ background-color: rgba($cf-over, 0.1); because a leftover `run --local` / `mxbuild --serve` / runtime would otherwise be silently adopted and keep serving old output (it looks like a cache but is a stale **process**). If a background `run --local` died while its serve+runtime kept serving, -recover with: +**the refusal names the pid** — on Linux it resolves the listener through `/proc`, so +recovery is one command: -```bash -pgrep -af 'mxbuild --serve|runtimelauncher|mxcli run' # find them -kill # stop each -curl -s -o /dev/null -w '%{http_code}\n' http://127.0.0.1:8080 # want 000 ``` +port 8080 (app) is already in use. + Held by pid 11893: /root/.mxcli/mxbuild/11.13.0/modeler/mxbuild --serve … + That is a leftover from an earlier run that did not shut down cleanly + (a kill -9 or a reaped container skips mxcli's own teardown). + kill 11893 +``` + +It also distinguishes the two cases, which need opposite remedies: a leftover of a +previous run is safe to kill, while a **foreign** listener (someone else's server on +8080) is not — for that it says so and points at `--app-port`. + +Note that a *graceful* stop already reaps everything: `run --local` puts each child +(mxbuild's JVM, the runtime, the rollup bundler) in its own process group and kills the +group on Ctrl-C/SIGTERM. Reaching this error means the previous run was killed with +`kill -9`, crashed, or had its container reaped — none of which run any handler. Do +**not** `pkill -f 'mxcli run'`: that pattern also matches the shell you type it in. Launch `run --local` as the **sole** command in its invocation (don't chain a trailing `sleep`/`curl` whose non-zero exit can kill the backgrounded run); poll separately. diff --git a/cmd/mxcli/cmd_lint.go b/cmd/mxcli/cmd_lint.go index b984463e1..56c137e99 100644 --- a/cmd/mxcli/cmd_lint.go +++ b/cmd/mxcli/cmd_lint.go @@ -47,7 +47,10 @@ Bundled Starlark rules (in .claude/lint-rules/): - Entity business key (ARCH003) - persistent entities need a unique key Quality: - McCabe complexity (QUAL001) - microflow cyclomatic complexity threshold - - Missing documentation (QUAL002) - entities/microflows need documentation + - Missing documentation (QUAL002) - every document type (modules, entities, + pages, microflows, workflows, Java/JavaScript actions and their parameters, + REST services, mappings, constants, ...). Attributes and associations are + off by default; see the check_* options. - Long microflows (QUAL003) - microflows with too many activities - Orphaned elements (QUAL004) - unreferenced elements in the project Design: @@ -247,6 +250,24 @@ Examples: os.Exit(1) } + // A failed catalog query means some iterator yielded nothing and the + // findings above are incomplete. Reported on stderr so it survives + // `| jq` on the results, and treated as a failure: silently passing CI + // on a lint run that could not read the model is the worst outcome. + queryErrs := lint.QueryErrors() + for _, qe := range queryErrs { + fmt.Fprintf(os.Stderr, + "Error: lint could not read the catalog for %s: %v\n", qe.Iterator, qe.Err) + } + if len(queryErrs) > 0 { + fmt.Fprintf(os.Stderr, + "Results are INCOMPLETE (%d failed %s). The cached catalog is usually the cause; "+ + "delete %s and re-run.\n", + len(queryErrs), map[bool]string{true: "query", false: "queries"}[len(queryErrs) == 1], + filepath.Join(projectDir, ".mxcli", "catalog.db")) + os.Exit(1) + } + // Exit with error if there are errors summary := linter.Summarize(violations) if summary.Errors > 0 { diff --git a/cmd/mxcli/docker/localapp.go b/cmd/mxcli/docker/localapp.go index 8842a9c4b..2c4e3e029 100644 --- a/cmd/mxcli/docker/localapp.go +++ b/cmd/mxcli/docker/localapp.go @@ -271,7 +271,8 @@ func checkLocalAppPortsFree(o LocalAppOptions) error { for _, p := range ports { if err := pingTCP(fmt.Sprintf("127.0.0.1:%d", p.port), 300*time.Millisecond); err == nil { return fmt.Errorf("port %d (%s) is already in use — stop the running instance first, "+ - "or pass a different port", p.port, p.what) + "or pass a different port.\n%s", + p.port, p.what, portCulpritAdvice(p.port, "127.0.0.1", o.AppPort)) } } return nil diff --git a/cmd/mxcli/docker/portguard.go b/cmd/mxcli/docker/portguard.go new file mode 100644 index 000000000..16dd4f0c5 --- /dev/null +++ b/cmd/mxcli/docker/portguard.go @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: Apache-2.0 + +package docker + +import ( + "fmt" + "strings" +) + +// portCulpritAdvice renders the "how do I get out of this" half of a +// port-already-in-use error, naming the offending process when it can be +// resolved (see portowner_linux.go). +// +// Why this is worth the code: the guard's diagnosis used to be a guess ("a +// previous 'mxcli run --local' … is likely still serving on it") followed by a +// pgrep hunt. The guess is wrong as often as it is right — an orphan of a +// previous run and a colleague's unrelated server on 8080 need opposite +// remedies — and one of the suggested commands, `pgrep -f 'mxcli run'`, matches +// the shell it is typed into, so following the advice literally can kill your +// own session. Naming the pid turns three commands into one. +// +// Every line is indented two spaces to sit under the error's first line. +func portCulpritAdvice(port int, host string, appPort int) string { + confirm := fmt.Sprintf( + " # confirm it is gone: curl -s -o /dev/null -w '%%{http_code}' http://%s:%d (want 000)\n", + host, appPort) + + owner, ok := listenerOnPort(port) + if !ok { + // Could not resolve — another user's process, or not Linux. Keep the + // generic hunt, minus the pgrep pattern that matches the caller's own + // shell. + return " Find and stop whatever is holding it, then retry:\n" + + " pgrep -af 'mxbuild|runtimelauncher' # a previous run's orphans, if any\n" + + " kill \n" + confirm + } + + var b strings.Builder + fmt.Fprintf(&b, " Held by pid %d", owner.PID) + if owner.Cmdline != "" { + fmt.Fprintf(&b, ": %s", owner.Cmdline) + } + b.WriteString("\n") + + if owner.Ours { + // mxcli reaps its own children on Ctrl-C/SIGTERM, so reaching this state + // means the previous run did not exit gracefully (kill -9, a crash, or a + // reaped container). Saying so stops the user looking for a bug in the + // guard. + b.WriteString(" That is a leftover from an earlier run that did not shut down cleanly " + + "(a kill -9 or a reaped container skips mxcli's own teardown).\n") + fmt.Fprintf(&b, " kill %d\n", owner.PID) + b.WriteString(confirm) + return b.String() + } + + b.WriteString(" That is not a process mxcli started, so it is not a leftover run — " + + "pick another port rather than killing it.\n") + return b.String() +} diff --git a/cmd/mxcli/docker/portowner_linux.go b/cmd/mxcli/docker/portowner_linux.go new file mode 100644 index 000000000..f47517519 --- /dev/null +++ b/cmd/mxcli/docker/portowner_linux.go @@ -0,0 +1,201 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package docker + +import ( + "fmt" + "os" + "path/filepath" + "strconv" + "strings" +) + +// portowner_linux.go answers "which process is holding this port?" so the warm +// loop's port guard can name it. +// +// The guard itself is correct and stays detection-only — reaping someone else's +// process is the user's call. What was missing is the diagnosis. `mxcli run +// --local` reaps its children on SIGINT/SIGTERM (see procgroup_unix.go), so a +// held port after a *graceful* stop is someone else's process; but a `kill -9`, +// a crash, or a reaped container runs no handler at all, and then the offender +// is the previous run's orphaned mxbuild/JVM. Both cases print the same message +// today, and it tells the user to go hunting with pgrep — three commands, one of +// which (`pgrep -f 'mxcli run'`) matches the shell you typed it in. +// +// Resolution is via /proc, not lsof/ss: those are frequently absent from slim +// containers, and shelling out to find out why a boot failed is its own failure +// mode. /proc/net/tcp gives the socket inode of the listener; scanning +// /proc//fd for a link to that inode gives the owner. + +// portOwner describes the process listening on a local port. +type portOwner struct { + PID int + Cmdline string // argv joined with spaces, truncated + Ours bool // command line looks like a process mxcli spawns +} + +// listenerOnPort identifies the process listening on 127.0.0.1:port (or on +// 0.0.0.0/[::]:port). Returns ok=false when nothing can be resolved — a listener +// owned by another user is the common case, since /proc//fd is unreadable +// then. Callers must degrade to the generic message rather than assert. +func listenerOnPort(port int) (portOwner, bool) { + inodes := listeningInodes(port) + if len(inodes) == 0 { + return portOwner{}, false + } + pid, ok := pidForSocketInode(inodes) + if !ok { + return portOwner{}, false + } + cmd := processCmdline(pid) + return portOwner{PID: pid, Cmdline: cmd, Ours: looksLikeWarmLoopChild(cmd)}, true +} + +// listeningInodes returns the socket inodes of every LISTEN socket bound to +// port, across IPv4 and IPv6. Both are checked because a JVM binding "localhost" +// usually lands on [::1] or a dual-stack [::], not 127.0.0.1. +// procNetTCPFiles are the kernel tables listeningInodes reads. A var, not a +// literal, so a test can prove BOTH are consulted — parsing tcp6 correctly is +// worth nothing if the file is never opened, and a test that calls the parser +// directly cannot tell the difference. +var procNetTCPFiles = []string{"/proc/net/tcp", "/proc/net/tcp6"} + +func listeningInodes(port int) map[string]bool { + out := map[string]bool{} + for _, f := range procNetTCPFiles { + data, err := os.ReadFile(f) + if err != nil { + continue + } + for ino := range parseListeningInodes(string(data), port) { + out[ino] = true + } + } + return out +} + +// parseListeningInodes extracts the socket inodes of LISTEN sockets on port from +// the contents of /proc/net/tcp or /proc/net/tcp6. +// +// Split out from the file reading so both address families can be tested +// wherever the suite runs: this container has no IPv6, and a test that only ever +// skips proves nothing about the tcp6 path — which is the one that matters, since +// a JVM binding "localhost" usually lands on [::1] or a dual-stack [::]. +func parseListeningInodes(contents string, port int) map[string]bool { + out := map[string]bool{} + lines := strings.Split(contents, "\n") + if len(lines) > 0 { + lines = lines[1:] // header + } + for _, line := range lines { + fields := strings.Fields(line) + // sl(0) local_address(1) remote_address(2) st(3) … inode(9) + if len(fields) < 10 { + continue + } + // 0A is TCP_LISTEN. An established connection *to* the port is not the + // owner of it, and counting one would name a client as the culprit. + if fields[3] != "0A" { + continue + } + if p, ok := hexPort(fields[1]); !ok || p != port { + continue + } + out[fields[9]] = true + } + return out +} + +// hexPort extracts the port from a /proc/net/tcp local_address ("0100007F:1F90"). +func hexPort(addr string) (int, bool) { + i := strings.LastIndex(addr, ":") + if i < 0 { + return 0, false + } + n, err := strconv.ParseUint(addr[i+1:], 16, 32) + if err != nil { + return 0, false + } + return int(n), true +} + +// pidForSocketInode finds the process holding any of the given socket inodes. +// +// A socket may be shared by several processes (a forked pre-forking server), and +// any of them will do for the message — the user needs a thread to pull, not a +// census. The lowest PID is chosen so repeated runs name the same one. +func pidForSocketInode(inodes map[string]bool) (int, bool) { + want := map[string]bool{} + for ino := range inodes { + want["socket:["+ino+"]"] = true + } + entries, err := os.ReadDir("/proc") + if err != nil { + return 0, false + } + best := 0 + for _, e := range entries { + pid, err := strconv.Atoi(e.Name()) + if err != nil { + continue // not a pid directory + } + fds, err := os.ReadDir(filepath.Join("/proc", e.Name(), "fd")) + if err != nil { + continue // another user's process, or it exited mid-scan + } + for _, fd := range fds { + link, err := os.Readlink(filepath.Join("/proc", e.Name(), "fd", fd.Name())) + if err != nil { + continue + } + if want[link] { + if best == 0 || pid < best { + best = pid + } + break + } + } + } + return best, best != 0 +} + +// processCmdline reads a process's argv, NUL-separated in /proc, and renders it +// on one line. Long Java command lines are truncated: the JVM's is thousands of +// characters of classpath, and printing it buries the error it is attached to. +func processCmdline(pid int) string { + data, err := os.ReadFile(fmt.Sprintf("/proc/%d/cmdline", pid)) + if err != nil { + return "" + } + args := strings.FieldsFunc(string(data), func(r rune) bool { return r == 0 }) + line := strings.Join(args, " ") + const max = 120 + if len(line) > max { + line = line[:max] + "…" + } + return line +} + +// looksLikeWarmLoopChild reports whether a command line matches something the +// warm loop starts. This decides which advice to print, and the two are +// genuinely different: an orphan of a previous run is safe to kill, while a +// foreign listener means the port is simply taken and the right move is +// --app-port. +func looksLikeWarmLoopChild(cmdline string) bool { + if cmdline == "" { + return false + } + for _, sig := range []string{ + "mxbuild", // the serve process (a wrapper script, or its JVM) + "runtimelauncher", // the standalone runtime + "RuntimeLauncher", + "mxcli", // a previous run still alive + } { + if strings.Contains(cmdline, sig) { + return true + } + } + return false +} diff --git a/cmd/mxcli/docker/portowner_linux_test.go b/cmd/mxcli/docker/portowner_linux_test.go new file mode 100644 index 000000000..507106104 --- /dev/null +++ b/cmd/mxcli/docker/portowner_linux_test.go @@ -0,0 +1,239 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package docker + +import ( + "fmt" + "net" + "os" + "path/filepath" + "strings" + "testing" +) + +// listenOnLoopback binds an ephemeral port and returns it, closing on cleanup. +func listenOnLoopback(t *testing.T, network, addr string) int { + t.Helper() + ln, err := net.Listen(network, addr) + if err != nil { + t.Skipf("cannot bind %s %s here: %v", network, addr, err) + } + t.Cleanup(func() { _ = ln.Close() }) + _, portStr, err := net.SplitHostPort(ln.Addr().String()) + if err != nil { + t.Fatalf("splitting %s: %v", ln.Addr(), err) + } + var port int + if _, err := fmt.Sscanf(portStr, "%d", &port); err != nil { + t.Fatalf("parsing port %q: %v", portStr, err) + } + return port +} + +// The whole point of the lookup is to name a real pid, so the test holds the +// port itself and demands its own — an assertion that cannot pass by accident. +// +// mxcli-formula1 suggested issue 8: the guard was right to refuse, but told the +// user to go hunting with pgrep, and one of the suggested patterns +// (`pgrep -f 'mxcli run'`) matches the shell it is typed into. +func TestListenerOnPort_IdentifiesThisProcess(t *testing.T) { + port := listenOnLoopback(t, "tcp", "127.0.0.1:0") + + owner, ok := listenerOnPort(port) + if !ok { + t.Fatalf("port %d is held by this test process but no owner was resolved", port) + } + if owner.PID != os.Getpid() { + t.Errorf("owner pid = %d, want this process (%d)", owner.PID, os.Getpid()) + } + if owner.Cmdline == "" { + t.Error("no command line resolved; the message would name a bare pid") + } +} + +// A Mendix runtime binding "localhost" lands on IPv6 as often as not, so +// /proc/net/tcp6 has to be parsed too — and its local_address is a 128-bit hex +// blob, not the 32-bit one in /proc/net/tcp. Both are asserted against captured +// kernel output rather than a live socket, because this container has no IPv6 +// and a test that only skips proves nothing about the path it names. +// +// 8090 is 0x1F9A; 6543 is 0x198F. +func TestParseListeningInodes_BothAddressFamilies(t *testing.T) { + const tcp4 = ` sl local_address rem_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode + 0: 0100007F:198F 00000000:0000 0A 00000000:00000000 00:00000000 00000000 0 0 4242 1 0000000000000000 100 0 0 10 0 +` + const tcp6 = ` sl local_address remote_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode + 0: 00000000000000000000000000000000:1F9A 00000000000000000000000000000000:0000 0A 00000000:00000000 00:00000000 00000000 0 0 9999 1 0000000000000000 100 0 0 10 0 +` + if got := parseListeningInodes(tcp4, 6543); !got["4242"] { + t.Errorf("IPv4 listener on 6543 not found, got %v", got) + } + if got := parseListeningInodes(tcp6, 8090); !got["9999"] { + t.Errorf("IPv6 listener on 8090 not found — the 128-bit local_address is not being parsed, got %v", got) + } +} + +// An established connection to a port is not the owner of it. Without the +// TCP_LISTEN filter, a client of the app would be named as the culprit and the +// advice would tell the user to kill their own browser. +func TestParseListeningInodes_IgnoresEstablishedConnections(t *testing.T) { + // 01 is TCP_ESTABLISHED. Same port (0x1F90 = 8080), different state. + const established = ` sl local_address rem_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode + 0: 0100007F:1F90 0100007F:C001 01 00000000:00000000 00:00000000 00000000 0 0 7777 1 0000000000000000 100 0 0 10 0 +` + if got := parseListeningInodes(established, 8080); len(got) != 0 { + t.Errorf("an ESTABLISHED socket was treated as the port's owner: %v", got) + } +} + +// Parsing tcp6 correctly is worth nothing if the file is never opened, and the +// parser tests above cannot tell the difference — dropping "/proc/net/tcp6" from +// the list left every one of them green. This points the reader at fixtures and +// asserts an inode is picked up from EACH table. +func TestListeningInodes_ReadsBothKernelTables(t *testing.T) { + dir := t.TempDir() + v4 := filepath.Join(dir, "tcp") + v6 := filepath.Join(dir, "tcp6") + // Same port (0x198F = 6543) in both tables, distinct inodes. + if err := os.WriteFile(v4, []byte( + " sl local_address rem_address st … inode\n"+ + " 0: 0100007F:198F 00000000:0000 0A 00000000:00000000 00:00000000 00000000 0 0 4242 1 0 100 0 0 10 0\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(v6, []byte( + " sl local_address remote_address st … inode\n"+ + " 0: 00000000000000000000000000000000:198F 00000000000000000000000000000000:0000 0A 00000000:00000000 00:00000000 00000000 0 0 9999 1 0 100 0 0 10 0\n"), 0o600); err != nil { + t.Fatal(err) + } + + orig := procNetTCPFiles + // Overriding the list is what makes the rest of this testable, and it is also + // what stops the override from proving anything about the SHIPPED list — so + // the default is asserted first. Dropping "/proc/net/tcp6" from it left every + // other test here green. + for _, want := range []string{"/proc/net/tcp", "/proc/net/tcp6"} { + found := false + for _, f := range orig { + if f == want { + found = true + } + } + if !found { + t.Errorf("procNetTCPFiles does not include %s, so that family is never read: %v", want, orig) + } + } + + procNetTCPFiles = []string{v4, v6} + t.Cleanup(func() { procNetTCPFiles = orig }) + + got := listeningInodes(6543) + if !got["4242"] { + t.Errorf("/proc/net/tcp is not being read, got %v", got) + } + if !got["9999"] { + t.Errorf("/proc/net/tcp6 is not being read — an IPv6-bound runtime would go unnamed, got %v", got) + } +} + +// A live IPv6 listener, where the environment has one. Skipped here (no IPv6), +// which is why the parser and wiring tests above carry the real weight. +func TestListenerOnPort_FindsAnIPv6Listener(t *testing.T) { + port := listenOnLoopback(t, "tcp6", "[::1]:0") + + owner, ok := listenerOnPort(port) + if !ok { + t.Fatalf("IPv6 listener on %d not resolved — /proc/net/tcp6 is not being read", port) + } + if owner.PID != os.Getpid() { + t.Errorf("owner pid = %d, want this process (%d)", owner.PID, os.Getpid()) + } +} + +// Nothing listening must resolve to nothing. If any socket state matched, a free +// port would name some unrelated process and the advice would be worse than the +// generic message it replaced. +func TestListenerOnPort_QuietWhenNothingIsListening(t *testing.T) { + port := listenOnLoopback(t, "tcp", "127.0.0.1:0") + // Re-resolve after the listener is closed: the port is now free. + owner, ok := listenerOnPort(port) + if ok { + // Only fail if it resolved to something while the port is still ours to + // claim — a racing process could legitimately take it. + if ln, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", port)); err == nil { + _ = ln.Close() + t.Errorf("free port %d resolved to pid %d (%s)", port, owner.PID, owner.Cmdline) + } + } +} + +func TestLooksLikeWarmLoopChild(t *testing.T) { + for _, tc := range []struct { + name string + cmdline string + want bool + }{ + {"mxbuild serve", "/root/.mxcli/mxbuild/11.13.0/modeler/mxbuild --serve", true}, + {"runtime launcher", "java -cp … com.mendix.runtimelauncher.RuntimeLauncher", true}, + {"a previous mxcli", "mxcli run --local -p app.mpr", true}, + {"someone else's server", "python3 -m http.server 8080", false}, + {"unresolvable", "", false}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := looksLikeWarmLoopChild(tc.cmdline); got != tc.want { + t.Errorf("looksLikeWarmLoopChild(%q) = %v, want %v", tc.cmdline, got, tc.want) + } + }) + } +} + +// The advice is the deliverable, not the lookup, so it is asserted through the +// same function the error message calls. +func TestPortCulpritAdvice_NamesThePidAndGivesOneCommand(t *testing.T) { + port := listenOnLoopback(t, "tcp", "127.0.0.1:0") + + advice := portCulpritAdvice(port, "127.0.0.1", port) + if !strings.Contains(advice, fmt.Sprintf("pid %d", os.Getpid())) { + t.Errorf("advice does not name the holding pid %d:\n%s", os.Getpid(), advice) + } + if strings.Contains(advice, "pgrep") { + t.Errorf("the pid is known, so the pgrep hunt must be gone:\n%s", advice) + } +} + +// A foreign listener and a leftover child need opposite remedies — kill it, or +// leave it alone and move ports — so the advice must not offer `kill` for a +// process mxcli did not start. +func TestPortCulpritAdvice_DoesNotOfferToKillAForeignProcess(t *testing.T) { + port := listenOnLoopback(t, "tcp", "127.0.0.1:0") + owner, ok := listenerOnPort(port) + if !ok { + t.Skip("owner not resolvable here") + } + if owner.Ours { + // The test binary happens to match the warm-loop signature (it can, when + // run from a path containing "mxcli"); this case cannot be exercised. + t.Skipf("test process looks like a warm-loop child: %s", owner.Cmdline) + } + advice := portCulpritAdvice(port, "127.0.0.1", port) + if strings.Contains(advice, fmt.Sprintf("kill %d", os.Getpid())) { + t.Errorf("offered to kill a foreign process:\n%s", advice) + } + if !strings.Contains(advice, "not a process mxcli started") { + t.Errorf("advice does not say the holder is foreign:\n%s", advice) + } +} + +// End to end through the real guard: the error a user sees carries the pid. +func TestCheckTargetPortsFree_ErrorNamesTheHolder(t *testing.T) { + port := listenOnLoopback(t, "tcp", "127.0.0.1:0") + + err := checkTargetPortsFree(LocalRunOptions{AppPort: port, AdminPort: 0, ServePort: 0}) + if err == nil { + t.Fatal("a held app port must be refused") + } + if !strings.Contains(err.Error(), fmt.Sprintf("pid %d", os.Getpid())) { + t.Errorf("the refusal does not name the holder:\n%v", err) + } +} diff --git a/cmd/mxcli/docker/portowner_other.go b/cmd/mxcli/docker/portowner_other.go new file mode 100644 index 000000000..0976d4900 --- /dev/null +++ b/cmd/mxcli/docker/portowner_other.go @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build !linux + +package docker + +// portOwner describes the process listening on a local port. Only Linux can +// resolve it from /proc without shelling out, so elsewhere the port guard keeps +// its generic wording rather than depending on lsof being installed. +type portOwner struct { + PID int + Cmdline string + Ours bool +} + +// listenerOnPort always reports "unknown" off Linux. +func listenerOnPort(port int) (portOwner, bool) { return portOwner{}, false } diff --git a/cmd/mxcli/docker/runlocal.go b/cmd/mxcli/docker/runlocal.go index a8725dc66..8911a41b0 100644 --- a/cmd/mxcli/docker/runlocal.go +++ b/cmd/mxcli/docker/runlocal.go @@ -423,14 +423,11 @@ func checkTargetPortsFree(o LocalRunOptions) error { } { hostPort := fmt.Sprintf("%s:%d", host, c.port) if err := pingTCP(hostPort, 500*time.Millisecond); err == nil { - return fmt.Errorf("port %d (%s) is already in use — a previous 'mxcli run --local' "+ - "or a stray mxbuild --serve/runtime is likely still serving on it.\n"+ + return fmt.Errorf("port %d (%s) is already in use.\n"+ " A stale process is silently adopted otherwise, so edits appear to do nothing (looks like a stale cache — it isn't).\n"+ - " Free the ports, then retry:\n"+ - " pgrep -af 'mxbuild --serve|runtimelauncher|mxcli run' # find them\n"+ - " kill # stop each; confirm with: curl -s -o /dev/null -w '%%{http_code}' http://%s:%d (want 000)\n"+ + "%s"+ " Or run on different ports with %s (and --admin-port/--serve-port).", - c.port, c.role, host, o.AppPort, c.flag) + c.port, c.role, portCulpritAdvice(c.port, host, o.AppPort), c.flag) } } return nil diff --git a/docs-site/src/tools/run-local.md b/docs-site/src/tools/run-local.md index d5de0f008..f08e2153c 100644 --- a/docs-site/src/tools/run-local.md +++ b/docs-site/src/tools/run-local.md @@ -282,16 +282,42 @@ If you started `run --local` in the background and the wrapping shell exited non (e.g. a chained `sleep`/`curl` that failed), the `run --local` process can die while its `mxbuild --serve` + runtime keep serving on `:8080`. Launch `run --local` as the **sole** command in its own invocation — don't chain a `sleep`/status check after it in -the same shell — and poll separately. To recover from a stale process: +the same shell — and poll separately. + +**The refusal names the offending process.** On Linux mxcli resolves the port's +listener through `/proc` and prints its pid and command line, so recovery is one +command rather than a `pgrep` hunt: + +``` +port 8080 (app) is already in use. + A stale process is silently adopted otherwise, so edits appear to do nothing + (looks like a stale cache — it isn't). + Held by pid 11893: /root/.mxcli/mxbuild/11.13.0/modeler/mxbuild --serve … + That is a leftover from an earlier run that did not shut down cleanly + (a kill -9 or a reaped container skips mxcli's own teardown). + kill 11893 + # confirm it is gone: curl -s -o /dev/null -w '%{http_code}' http://127.0.0.1:8080 (want 000) + Or run on different ports with --app-port (and --admin-port/--serve-port). +``` + +The two cases need opposite remedies, so they are reported differently. A **leftover +of a previous run** is safe to kill, as above. A **foreign** listener — anything mxcli +did not start — is not, and mxcli says so instead of offering a `kill`: -```bash -pgrep -af 'mxbuild --serve|runtimelauncher|mxcli run' # find them -kill # stop each -curl -s -o /dev/null -w '%{http_code}\n' http://127.0.0.1:8080 # want 000 (port free) ``` + Held by pid 4820: python3 -m http.server 8080 + That is not a process mxcli started, so it is not a leftover run — + pick another port rather than killing it. +``` + +Off Linux, or when the listener belongs to another user (so `/proc//fd` cannot be +read), it falls back to a generic hint. -Then start `run --local` again. Or run on different ports with `--app-port` / -`--admin-port` / `--serve-port`. +A *graceful* stop needs none of this: each child — mxbuild's JVM, the runtime, the +rollup bundler — is started in its own process group and the whole group is killed on +Ctrl-C/SIGTERM. Seeing this error means the previous run never got to run its teardown: +`kill -9`, a crash, or a reaped container. Avoid `pkill -f 'mxcli run'` — that pattern +also matches the shell you type it into. ## Pages render in the browser diff --git a/docs-site/src/tools/starlark-rules.md b/docs-site/src/tools/starlark-rules.md index bec2d5af3..8dffbaabe 100644 --- a/docs-site/src/tools/starlark-rules.md +++ b/docs-site/src/tools/starlark-rules.md @@ -28,7 +28,7 @@ In addition to the built-in Go rules, mxcli bundles 27 Starlark-based lint rules | Rule | Description | |------|-------------| | **QUAL001** | McCabe complexity -- Flags microflows with high cyclomatic complexity | -| **QUAL002** | Documentation -- Checks for missing documentation on public elements | +| **QUAL002** | Documentation -- Missing documentation across every document type: modules, entities, pages, microflows, workflows, Java/JavaScript actions and their parameters, REST services, mappings, constants ([options](#qual002-options)) | | **QUAL003** | Long microflows -- Warns about microflows with too many activities | | **QUAL004** | Orphaned elements -- Detects unused entities, microflows, or pages | @@ -49,6 +49,82 @@ In addition to the built-in Go rules, mxcli bundles 27 Starlark-based lint rules Additional convention rules cover access rule constraints, role mapping, microflow size and content. +### QUAL002 options {#qual002-options} + +Undocumented model elements are invisible to `mxcli check` and to the build — +nothing fails, so nothing reminds you. QUAL002 is that reminder, and it sweeps +**every document type a user authors**, not just the domain model. + +**On by default** — one option per document type: + +| Option | Element | +|---|---| +| `check_modules` | Module | +| `check_entities` | Entity | +| `check_pages` | Page | +| `check_snippets` | Snippet | +| `check_building_blocks` | Building block | +| `check_layouts` | Layout | +| `check_enumerations` | Enumeration | +| `check_microflows` | Microflow (nanoflows always exempt) | +| `check_java_actions` | Java action | +| `check_java_action_params` | Java action **parameter** | +| `check_javascript_actions` | JavaScript action | +| `check_workflows` | Workflow | +| `check_constants` | Constant | +| `check_image_collections` | Image collection | +| `check_data_transformers` | Data transformer | +| `check_business_event_services` | Business event service | +| `check_rest_clients` | Consumed REST client | +| `check_published_rest_services` | Published REST service | +| `check_json_structures` | JSON structure | +| `check_import_mappings` | Import mapping | +| `check_export_mappings` | Export mapping | + +**Off by default** — members, suppressed for volume rather than for being +unimportant: + +| Option | Element | +|---|---| +| `check_attributes` | Entity attribute | +| `check_associations` | Association | + +Plus `min_activities` (default `3`): microflows with fewer activities are exempt, +on the grounds that a three-step flow's name says what a description would. + +Why the split. A Java action has a handful of parameters, and Studio Pro renders +each description in the dialog where someone wires up the call — an undocumented +parameter is a blank field next to a name like `pInput` at exactly the moment a +caller has to decide what to pass. A domain model, by contrast, has hundreds of +attributes and associations, so the same check there is a wall of text rather +than a signal. Turn those two on when you are actively working through +documentation debt: + +```yaml +rules: + QUAL002: + enabled: true + options: + check_attributes: true + check_associations: true + check_pages: false # e.g. if page names are the convention here + min_activities: 5 +``` + +Elements in System and Marketplace modules are never reported — that is code you +did not write, and flagging Community Commons would bury the findings you can +act on. This applies to **every** rule, not just QUAL002: the `System` module is +not distinguishable by its `Source` (which is empty, exactly like your own +modules), so it used to slip past the Marketplace filter into every rule that +walks entities, pages, microflows or widgets. On a blank Mendix 9.24 project that +was the difference between 60 findings and 8. + +Adding a document type to the sweep is two rows: one in `documentableSources` +(`mdl/linter/context.go`) naming the catalog table and its documentation column, +and one in `_DOC_KINDS` (`missing_documentation.star`) giving the option name and +the suggestion text. `TestQUAL002_SweepsEveryAdvertisedDocumentType` fails if the +Go side advertises a kind the tests do not cover. + ## Where Starlark Rules Live When you run `mxcli init`, Starlark rules are installed to: diff --git a/mdl/catalog/builder_modules.go b/mdl/catalog/builder_modules.go index 23fab80a1..438d25c5c 100644 --- a/mdl/catalog/builder_modules.go +++ b/mdl/catalog/builder_modules.go @@ -309,7 +309,19 @@ func (b *Builder) buildJavaActions() error { } defer stmt.Close() + paramStmt, err := b.tx.Prepare(` + INSERT INTO java_action_parameters_data (Id, JavaActionId, JavaActionName, + QualifiedName, ModuleName, Name, Description, ParameterType, IsRequired, + Ordinal, ProjectId, SnapshotId) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + `) + if err != nil { + return err + } + defer paramStmt.Close() + projectID, snapshotID := b.snapshotMeta() + paramCount := 0 for _, ja := range actions { moduleID := b.hierarchy.findModuleID(ja.ContainerID) @@ -337,9 +349,38 @@ func (b *Builder) buildJavaActions() error { if err != nil { return err } + + for i, p := range ja.Parameters { + if p == nil { + continue + } + paramType := "" + if p.ParameterType != nil { + paramType = p.ParameterType.TypeString() + } + if _, err := paramStmt.Exec( + string(p.ID), + string(ja.ID), + ja.Name, + qualifiedName+"."+p.Name, + moduleName, + p.Name, + p.Description, + paramType, + boolToInt(p.IsRequired), + i, + projectID, snapshotID, + ); err != nil { + return err + } + paramCount++ + } } b.report("Java Actions", len(actions)) + if paramCount > 0 { + b.report("Java Action Parameters", paramCount) + } return nil } diff --git a/mdl/catalog/catalog.go b/mdl/catalog/catalog.go index 528f97a7f..07383a3d7 100644 --- a/mdl/catalog/catalog.go +++ b/mdl/catalog/catalog.go @@ -112,6 +112,7 @@ func (c *Catalog) Tables() []string { "CATALOG.LAYOUTS", "CATALOG.ENUMERATIONS", "CATALOG.JAVA_ACTIONS", + "CATALOG.JAVA_ACTION_PARAMETERS", "CATALOG.JAVASCRIPT_ACTIONS", "CATALOG.IMAGE_COLLECTIONS", "CATALOG.DATA_TRANSFORMERS", diff --git a/mdl/catalog/tables.go b/mdl/catalog/tables.go index 8d1dcbfab..b39426c6b 100644 --- a/mdl/catalog/tables.go +++ b/mdl/catalog/tables.go @@ -7,12 +7,16 @@ package catalog // // History: // +// 9 — java_action_parameters_data + view: a Java action's parameters, each +// with its own Description. Without the bump a cached catalog silently +// reports zero parameters, so QUAL002 would under-report rather than +// fail (mxcli-formula1: missing documentation on Java actions). // 2 — split each domain table into _data + view that JOINs // snapshots, removing the denormalized ProjectName / SnapshotDate / // SnapshotSource / SourceId / SourceBranch / SourceRevision columns // from every row (issue #576). // 1 — initial flat schema with denormalized snapshot columns on every row. -const CatalogSchemaVersion = "8" +const CatalogSchemaVersion = "9" // MetaSchemaVersion is the catalog_meta key that records the schema version // the cache was built against. @@ -262,6 +266,26 @@ func (c *Catalog) createTables() error { )`, viewWithFullSnapshot("java_actions"), + // java_action_parameters — one row per parameter of a Java action. + // Separate from java_actions (which only counts them) because a + // parameter carries its own Description, and an undocumented parameter + // is the thing a caller actually sees in Studio Pro's action dialog. + `CREATE TABLE IF NOT EXISTS java_action_parameters_data ( + Id TEXT PRIMARY KEY, + JavaActionId TEXT, + JavaActionName TEXT, + QualifiedName TEXT, + ModuleName TEXT, + Name TEXT, + Description TEXT, + ParameterType TEXT, + IsRequired INTEGER DEFAULT 0, + Ordinal INTEGER DEFAULT 0, + ProjectId TEXT, + SnapshotId TEXT + )`, + viewWithFullSnapshot("java_action_parameters"), + // javascript_actions `CREATE TABLE IF NOT EXISTS javascript_actions_data ( Id TEXT PRIMARY KEY, diff --git a/mdl/executor/cmd_contract.go b/mdl/executor/cmd_contract.go index 5df71619c..f733c63d1 100644 --- a/mdl/executor/cmd_contract.go +++ b/mdl/executor/cmd_contract.go @@ -612,8 +612,11 @@ func createExternalEntities(ctx *ExecContext, s *ast.CreateExternalEntitiesStmt) // ("'latitude' is marked Filterable=False in the OData service, but // True in the app") — one per property, on an import whose whole job // is to match the contract. - nonFilterable := make(map[string]bool) - nonSortable := make(map[string]bool) + // Both restrictions have two shapes — a per-property exclusion list and + // a whole-set boolean — so they are resolved by EdmEntitySet rather + // than read field by field here; consulting one shape and not the + // other is what produced 28 × CE6630 on a single service + // (mxcli-formula1 §48). AttrFilterable/AttrSortable are nil-safe. if entitySet != nil { for _, name := range entitySet.NonInsertableProperties { nonInsertable[name] = true @@ -621,12 +624,6 @@ func createExternalEntities(ctx *ExecContext, s *ast.CreateExternalEntitiesStmt) for _, name := range entitySet.NonUpdatableProperties { nonUpdatable[name] = true } - for _, name := range entitySet.NonFilterableProperties { - nonFilterable[name] = true - } - for _, name := range entitySet.NonSortableProperties { - nonSortable[name] = true - } } // Build attributes from merged properties @@ -669,8 +666,8 @@ func createExternalEntities(ctx *ExecContext, s *ast.CreateExternalEntitiesStmt) Type: edmToDomainModelAttrType(p, keyPropSet[p.Name]), RemoteName: p.Name, RemoteType: p.Type, - Filterable: !nonFilterable[p.Name], - Sortable: !nonSortable[p.Name], + Filterable: entitySet.AttrFilterable(p.Name), + Sortable: entitySet.AttrSortable(p.Name), Creatable: creatable, Updatable: updatable, } diff --git a/mdl/linter/context.go b/mdl/linter/context.go index 8ed515f3d..e38e774fb 100644 --- a/mdl/linter/context.go +++ b/mdl/linter/context.go @@ -4,7 +4,9 @@ package linter import ( "database/sql" + "fmt" "iter" + "sync" "github.com/mendixlabs/mxcli/mdl/catalog" "github.com/mendixlabs/mxcli/mdl/types" @@ -41,6 +43,12 @@ type LintContext struct { fullMFCache map[model.ID]*microflows.Microflow fullMFErr error fullMFLoaded bool + + // queryErrors collects catalog queries that failed, so an iterator that + // could not run is distinguishable from one that legitimately found + // nothing. See QueryError. + queryErrors []QueryError + queryErrMu sync.Mutex } // FullMicroflow returns the fully-parsed microflow (with its object collection) @@ -164,7 +172,7 @@ type Entity struct { // Entities returns an iterator over all entities (excluding system modules). func (ctx *LintContext) Entities() iter.Seq[Entity] { return func(yield func(Entity) bool) { - rows, err := ctx.db.Query(` + rows, err := ctx.db.Query(fmt.Sprintf(` SELECT e.Id, e.Name, e.QualifiedName, e.ModuleName, e.Folder, CASE e.EntityType WHEN 'PERSISTENT' THEN 'Persistent' @@ -177,10 +185,11 @@ func (ctx *LintContext) Entities() iter.Seq[Entity] { e.HasEventHandlers, e.IsExternal FROM entities e LEFT JOIN modules m ON e.ModuleName = m.Name - WHERE COALESCE(m.Source, '') = '' + WHERE %s ORDER BY e.ModuleName, e.Name - `) + `, notPlatformModule("m"))) if err != nil { + ctx.recordQueryError("Entities", err) return } defer rows.Close() @@ -194,6 +203,7 @@ func (ctx *LintContext) Entities() iter.Seq[Entity] { &e.AccessRuleCount, &e.ValidationRuleCount, &hasEventHandlers, &isExternal) if err != nil { + ctx.recordQueryError("Entities (row scan)", err) continue } e.Folder = folder.String @@ -241,6 +251,7 @@ func (ctx *LintContext) AttributesFor(entityQualifiedName string) iter.Seq[Attri ORDER BY Name `, entityQualifiedName) if err != nil { + ctx.recordQueryError("AttributesFor", err) return } defer rows.Close() @@ -254,6 +265,7 @@ func (ctx *LintContext) AttributesFor(entityQualifiedName string) iter.Seq[Attri &a.ModuleName, &dataType, &length, &isUnique, &isRequired, &defaultVal, &isCalculated, &desc) if err != nil { + ctx.recordQueryError("AttributesFor (row scan)", err) continue } a.DataType = dataType.String @@ -292,6 +304,7 @@ func (ctx *LintContext) PermissionsFor(entityQualifiedName string) iter.Seq[Perm ORDER BY ModuleRoleName, AccessType `, entityQualifiedName) if err != nil { + ctx.recordQueryError("PermissionsFor", err) return } defer rows.Close() @@ -301,6 +314,7 @@ func (ctx *LintContext) PermissionsFor(entityQualifiedName string) iter.Seq[Perm var memberName, xpathConstraint, moduleName sql.NullString err := rows.Scan(&p.ModuleRoleName, &p.EntityName, &memberName, &p.AccessType, &xpathConstraint, &moduleName) if err != nil { + ctx.recordQueryError("PermissionsFor (row scan)", err) continue } p.MemberName = memberName.String @@ -341,6 +355,7 @@ func (ctx *LintContext) Permissions() iter.Seq[AllPermission] { ORDER BY ElementType, ElementName, ModuleRoleName, AccessType `) if err != nil { + ctx.recordQueryError("Permissions", err) return } defer rows.Close() @@ -407,12 +422,14 @@ func (ctx *LintContext) RoleMappings() iter.Seq[RoleMappingInfo] { ORDER BY UserRoleName, ModuleRoleName `) if err != nil { + ctx.recordQueryError("RoleMappings", err) return } defer rows.Close() for rows.Next() { var rm RoleMappingInfo if err := rows.Scan(&rm.UserRoleName, &rm.ModuleRoleName, &rm.ModuleName); err != nil { + ctx.recordQueryError("RoleMappings (row scan)", err) continue } if ctx.IsExcluded(rm.ModuleName) { @@ -445,12 +462,14 @@ func (ctx *LintContext) ModuleRoles() iter.Seq[ModuleRoleInfo] { ORDER BY ModuleName, ModuleRoleName `) if err != nil { + ctx.recordQueryError("ModuleRoles", err) return } defer rows.Close() for rows.Next() { var mr ModuleRoleInfo if err := rows.Scan(&mr.Name, &mr.ModuleName); err != nil { + ctx.recordQueryError("ModuleRoles (row scan)", err) continue } if ctx.IsExcluded(mr.ModuleName) { @@ -481,16 +500,17 @@ type Microflow struct { // Microflows returns an iterator over all microflows (excluding system modules). func (ctx *LintContext) Microflows() iter.Seq[Microflow] { return func(yield func(Microflow) bool) { - rows, err := ctx.db.Query(` + rows, err := ctx.db.Query(fmt.Sprintf(` SELECT mf.Id, mf.Name, mf.QualifiedName, mf.ModuleName, mf.Folder, mf.MicroflowType, mf.Description, mf.ReturnType, mf.ParameterCount, mf.ActivityCount, mf.Complexity FROM microflows mf LEFT JOIN modules m ON mf.ModuleName = m.Name - WHERE COALESCE(m.Source, '') = '' + WHERE %s ORDER BY mf.ModuleName, mf.Name - `) + `, notPlatformModule("m"))) if err != nil { + ctx.recordQueryError("Microflows", err) return } defer rows.Close() @@ -501,6 +521,7 @@ func (ctx *LintContext) Microflows() iter.Seq[Microflow] { err := rows.Scan(&mf.ID, &mf.Name, &mf.QualifiedName, &mf.ModuleName, &folder, &mf.MicroflowType, &desc, &retType, &mf.ParameterCount, &mf.ActivityCount, &mf.Complexity) if err != nil { + ctx.recordQueryError("Microflows (row scan)", err) continue } mf.Folder = folder.String @@ -534,15 +555,16 @@ type Page struct { // Pages returns an iterator over all pages (excluding system modules). func (ctx *LintContext) Pages() iter.Seq[Page] { return func(yield func(Page) bool) { - rows, err := ctx.db.Query(` + rows, err := ctx.db.Query(fmt.Sprintf(` SELECT p.Id, p.Name, p.QualifiedName, p.ModuleName, p.Folder, p.Title, p.URL, p.Description, p.WidgetCount FROM pages p LEFT JOIN modules m ON p.ModuleName = m.Name - WHERE COALESCE(m.Source, '') = '' + WHERE %s ORDER BY p.ModuleName, p.Name - `) + `, notPlatformModule("m"))) if err != nil { + ctx.recordQueryError("Pages", err) return } defer rows.Close() @@ -554,6 +576,7 @@ func (ctx *LintContext) Pages() iter.Seq[Page] { err := rows.Scan(&pg.ID, &pg.Name, &pg.QualifiedName, &pg.ModuleName, &folder, &title, &url, &desc, &widgetCount) if err != nil { + ctx.recordQueryError("Pages (row scan)", err) continue } pg.Folder = folder.String @@ -587,15 +610,16 @@ type Enumeration struct { // Enumerations returns an iterator over all enumerations (excluding system modules). func (ctx *LintContext) Enumerations() iter.Seq[Enumeration] { return func(yield func(Enumeration) bool) { - rows, err := ctx.db.Query(` + rows, err := ctx.db.Query(fmt.Sprintf(` SELECT en.Id, en.Name, en.QualifiedName, en.ModuleName, en.Folder, en.Description, en.ValueCount FROM enumerations en LEFT JOIN modules m ON en.ModuleName = m.Name - WHERE COALESCE(m.Source, '') = '' + WHERE %s ORDER BY en.ModuleName, en.Name - `) + `, notPlatformModule("m"))) if err != nil { + ctx.recordQueryError("Enumerations", err) return } defer rows.Close() @@ -606,6 +630,7 @@ func (ctx *LintContext) Enumerations() iter.Seq[Enumeration] { err := rows.Scan(&en.ID, &en.Name, &en.QualifiedName, &en.ModuleName, &folder, &desc, &en.ValueCount) if err != nil { + ctx.recordQueryError("Enumerations (row scan)", err) continue } en.Folder = folder.String @@ -637,15 +662,16 @@ type LintConstant struct { // Constants returns an iterator over all constants (excluding system modules). func (ctx *LintContext) Constants() iter.Seq[LintConstant] { return func(yield func(LintConstant) bool) { - rows, err := ctx.db.Query(` + rows, err := ctx.db.Query(fmt.Sprintf(` SELECT c.Id, c.Name, c.QualifiedName, c.ModuleName, c.Folder, c.Description, c.DefaultValue, c.ExposedToClient FROM constants c LEFT JOIN modules m ON c.ModuleName = m.Name - WHERE COALESCE(m.Source, '') = '' + WHERE %s ORDER BY c.ModuleName, c.Name - `) + `, notPlatformModule("m"))) if err != nil { + ctx.recordQueryError("Constants", err) return } defer rows.Close() @@ -657,6 +683,7 @@ func (ctx *LintContext) Constants() iter.Seq[LintConstant] { err := rows.Scan(&c.ID, &c.Name, &c.QualifiedName, &c.ModuleName, &folder, &desc, &defaultVal, &exposedToClient) if err != nil { + ctx.recordQueryError("Constants (row scan)", err) continue } c.Folder = folder.String @@ -693,16 +720,17 @@ type Widget struct { // Widgets returns an iterator over all widgets (excluding system modules). func (ctx *LintContext) Widgets() iter.Seq[Widget] { return func(yield func(Widget) bool) { - rows, err := ctx.db.Query(` + rows, err := ctx.db.Query(fmt.Sprintf(` SELECT w.Id, w.Name, w.WidgetType, w.ContainerId, w.ContainerQualifiedName, w.ContainerType, w.ModuleName, w.EntityRef, w.AttributeRef, w.MicroflowRef, w.NanoflowRef FROM widgets w LEFT JOIN modules m ON w.ModuleName = m.Name - WHERE COALESCE(m.Source, '') = '' + WHERE %s ORDER BY w.ModuleName, w.ContainerQualifiedName, w.Name - `) + `, notPlatformModule("m"))) if err != nil { + ctx.recordQueryError("Widgets", err) return } defer rows.Close() @@ -713,6 +741,7 @@ func (ctx *LintContext) Widgets() iter.Seq[Widget] { err := rows.Scan(&w.ID, &w.Name, &w.WidgetType, &containerID, &containerQName, &containerType, &w.ModuleName, &entityRef, &attrRef, &mfRef, &nfRef) if err != nil { + ctx.recordQueryError("Widgets (row scan)", err) continue } w.ContainerID = containerID.String @@ -747,14 +776,15 @@ type Snippet struct { // Snippets returns an iterator over all snippets (excluding system modules). func (ctx *LintContext) Snippets() iter.Seq[Snippet] { return func(yield func(Snippet) bool) { - rows, err := ctx.db.Query(` + rows, err := ctx.db.Query(fmt.Sprintf(` SELECT s.Id, s.Name, s.QualifiedName, s.ModuleName, s.Folder, s.WidgetCount FROM snippets s LEFT JOIN modules m ON s.ModuleName = m.Name - WHERE COALESCE(m.Source, '') = '' + WHERE %s ORDER BY s.ModuleName, s.Name - `) + `, notPlatformModule("m"))) if err != nil { + ctx.recordQueryError("Snippets", err) return } defer rows.Close() @@ -765,6 +795,7 @@ func (ctx *LintContext) Snippets() iter.Seq[Snippet] { var widgetCount sql.NullInt64 err := rows.Scan(&s.ID, &s.Name, &s.QualifiedName, &s.ModuleName, &folder, &widgetCount) if err != nil { + ctx.recordQueryError("Snippets (row scan)", err) continue } s.Folder = folder.String @@ -845,6 +876,7 @@ func (ctx *LintContext) ScheduledEvents() iter.Seq[ScheduledEvent] { events, err := ctx.reader.ListScheduledEvents() if err != nil { + ctx.recordQueryError("ScheduledEvents", err) return } @@ -901,6 +933,7 @@ func (ctx *LintContext) XPathExpressions() iter.Seq[XPathExpressionEntry] { ORDER BY ModuleName, DocumentQualifiedName `) if err != nil { + ctx.recordQueryError("XPathExpressions", err) return } defer rows.Close() @@ -916,6 +949,7 @@ func (ctx *LintContext) XPathExpressions() iter.Seq[XPathExpressionEntry] { &isParam, &e.UsageType, &moduleName, ) if err != nil { + ctx.recordQueryError("XPathExpressions (row scan)", err) continue } e.ComponentName = componentName.String @@ -949,15 +983,16 @@ type DatabaseConnection struct { // DatabaseConnections returns an iterator over all database connections (excluding system modules). func (ctx *LintContext) DatabaseConnections() iter.Seq[DatabaseConnection] { return func(yield func(DatabaseConnection) bool) { - rows, err := ctx.db.Query(` + rows, err := ctx.db.Query(fmt.Sprintf(` SELECT dc.Id, dc.Name, dc.QualifiedName, dc.ModuleName, dc.Folder, dc.DatabaseType, dc.QueryCount FROM database_connections dc LEFT JOIN modules m ON dc.ModuleName = m.Name - WHERE COALESCE(m.Source, '') = '' + WHERE %s ORDER BY dc.ModuleName, dc.Name - `) + `, notPlatformModule("m"))) if err != nil { + ctx.recordQueryError("DatabaseConnections", err) return } defer rows.Close() @@ -968,6 +1003,7 @@ func (ctx *LintContext) DatabaseConnections() iter.Seq[DatabaseConnection] { err := rows.Scan(&dc.ID, &dc.Name, &dc.QualifiedName, &dc.ModuleName, &folder, &dc.DatabaseType, &dc.QueryCount) if err != nil { + ctx.recordQueryError("DatabaseConnections (row scan)", err) continue } dc.Folder = folder.String @@ -1007,6 +1043,7 @@ func (ctx *LintContext) ActivitiesFor(microflowQualifiedName string) iter.Seq[Ac ORDER BY Sequence `, microflowQualifiedName) if err != nil { + ctx.recordQueryError("ActivitiesFor", err) return } defer rows.Close() @@ -1017,6 +1054,7 @@ func (ctx *LintContext) ActivitiesFor(microflowQualifiedName string) iter.Seq[Ac err := rows.Scan(&a.ID, &name, &caption, &a.ActivityType, &actionType, &a.MicroflowID, &a.MicroflowQualifiedName, &a.ModuleName, &entityRef) if err != nil { + ctx.recordQueryError("ActivitiesFor (row scan)", err) continue } a.Name = name.String @@ -1094,6 +1132,7 @@ func (ctx *LintContext) FindReferences(targetName string) []Reference { err := rows.Scan(&r.SourceType, &srcID, &r.SourceName, &r.TargetType, &tgtID, &r.TargetName, &r.RefKind, &r.ModuleName) if err != nil { + ctx.recordQueryError("FindReferences (row scan)", err) continue } r.SourceID = srcID.String @@ -1110,41 +1149,42 @@ func (ctx *LintContext) FindUnused(kind string) []string { var query string switch kind { case "entity": - query = ` + query = fmt.Sprintf(` SELECT e.QualifiedName FROM entities e LEFT JOIN modules m ON e.ModuleName = m.Name - WHERE COALESCE(m.Source, '') = '' + WHERE %s AND e.QualifiedName NOT IN ( SELECT DISTINCT TargetName FROM refs WHERE TargetType = 'ENTITY' ) - ` + `, notPlatformModule("m")) case "microflow": - query = ` + query = fmt.Sprintf(` SELECT mf.QualifiedName FROM microflows mf LEFT JOIN modules m ON mf.ModuleName = m.Name - WHERE COALESCE(m.Source, '') = '' + WHERE %s AND mf.QualifiedName NOT IN ( SELECT DISTINCT TargetName FROM refs WHERE TargetType IN ('MICROFLOW', 'NANOFLOW') ) - ` + `, notPlatformModule("m")) case "page": - query = ` + query = fmt.Sprintf(` SELECT p.QualifiedName FROM pages p LEFT JOIN modules m ON p.ModuleName = m.Name - WHERE COALESCE(m.Source, '') = '' + WHERE %s AND p.QualifiedName NOT IN ( SELECT DISTINCT TargetName FROM refs WHERE TargetType = 'PAGE' ) - ` + `, notPlatformModule("m")) default: return unused } rows, err := ctx.db.Query(query) if err != nil { + ctx.recordQueryError("FindUnused("+kind+")", err) return unused } defer rows.Close() @@ -1200,3 +1240,299 @@ func (ctx *LintContext) ModuleDependencies() map[string][]string { return deps } + +// JavaActionParameter represents one parameter of a Java action. +type JavaActionParameter struct { + Name string + Description string + ParameterType string + IsRequired bool +} + +// JavaAction represents a Java action from the catalog, with its parameters. +// +// Parameters are carried on the action rather than offered as a separate +// iterator: a rule that reports an undocumented parameter has to name the action +// it belongs to, and pairing them here keeps a rule from having to join. +type JavaAction struct { + ID string + Name string + QualifiedName string + ModuleName string + Folder string + Documentation string + ExportLevel string + ReturnType string + Parameters []JavaActionParameter +} + +// JavaActions returns an iterator over all Java actions (excluding system and +// marketplace modules, as every other document iterator here does — a rule must +// not report undocumented code the user did not write). +func (ctx *LintContext) JavaActions() iter.Seq[JavaAction] { + return func(yield func(JavaAction) bool) { + params := ctx.javaActionParameters() + + rows, err := ctx.db.Query(fmt.Sprintf(` + SELECT ja.Id, ja.Name, ja.QualifiedName, ja.ModuleName, ja.Folder, + ja.Documentation, ja.ExportLevel, ja.ReturnType + FROM java_actions ja + LEFT JOIN modules m ON ja.ModuleName = m.Name + WHERE %s + ORDER BY ja.ModuleName, ja.Name + `, notPlatformModule("m"))) + if err != nil { + ctx.recordQueryError("JavaActions", err) + return + } + defer rows.Close() + + for rows.Next() { + var ja JavaAction + var folder, doc, exportLevel, retType sql.NullString + if err := rows.Scan(&ja.ID, &ja.Name, &ja.QualifiedName, &ja.ModuleName, + &folder, &doc, &exportLevel, &retType); err != nil { + continue + } + ja.Folder = folder.String + ja.Documentation = doc.String + ja.ExportLevel = exportLevel.String + ja.ReturnType = retType.String + ja.Parameters = params[ja.ID] + + if ctx.IsExcluded(ja.ModuleName) { + continue + } + if !yield(ja) { + return + } + } + } +} + +// javaActionParameters indexes every parameter by its owning action's ID, in +// declaration order. +func (ctx *LintContext) javaActionParameters() map[string][]JavaActionParameter { + out := map[string][]JavaActionParameter{} + rows, err := ctx.db.Query(` + SELECT JavaActionId, Name, Description, ParameterType, IsRequired + FROM java_action_parameters + ORDER BY JavaActionId, Ordinal + `) + if err != nil { + return out + } + defer rows.Close() + + for rows.Next() { + var actionID string + var p JavaActionParameter + var desc, ptype sql.NullString + var required int + if err := rows.Scan(&actionID, &p.Name, &desc, &ptype, &required); err != nil { + ctx.recordQueryError("javaActionParameters (row scan)", err) + continue + } + p.Description = desc.String + p.ParameterType = ptype.String + p.IsRequired = required != 0 + out[actionID] = append(out[actionID], p) + } + return out +} + +// QueryError records a catalog query that failed inside an iterator. +// +// The iterators return iter.Seq[T] with no error channel, so a failed query +// used to be indistinguishable from an empty result: `if err != nil { return }` +// yielded nothing and said nothing. That is the worst shape for a linter, whose +// entire output is "here is what I found" — a broken query reads as a clean +// project. Errors are collected here and surfaced by the runner instead. +type QueryError struct { + Iterator string // the iterator that failed, e.g. "Entities" + Err error +} + +func (e QueryError) Error() string { return e.Iterator + ": " + e.Err.Error() } + +// recordQueryError notes a failed query. Iterators still degrade to "no rows" +// so one broken query cannot take down the whole run, but the failure is no +// longer silent. +// +// The mutex is not currently required (Linter.Run is sequential) but the Linter +// carries a maxWorkers knob, so this stays safe if rules ever run in parallel. +func (ctx *LintContext) recordQueryError(iterator string, err error) { + if err == nil { + return + } + ctx.queryErrMu.Lock() + defer ctx.queryErrMu.Unlock() + // Deduplicate: several rules iterate the same accessor, so one broken view + // would otherwise be reported once per rule. The fact matters, not how many + // rules tripped over it. + for _, existing := range ctx.queryErrors { + if existing.Iterator == iterator && existing.Err.Error() == err.Error() { + return + } + } + ctx.queryErrors = append(ctx.queryErrors, QueryError{Iterator: iterator, Err: err}) +} + +// QueryErrors returns every catalog query that failed during this lint run. +// A non-empty result means the findings are incomplete. +func (ctx *LintContext) QueryErrors() []QueryError { + ctx.queryErrMu.Lock() + defer ctx.queryErrMu.Unlock() + return append([]QueryError(nil), ctx.queryErrors...) +} + +// systemModuleID is the fixed sentinel Mendix gives the built-in System module +// (mirrors modelsdk/meta.SystemModuleID, not imported to keep the linter free of +// an engine dependency). +// +// System is NOT distinguishable by modules.Source — that column carries +// "Marketplace …" for downloaded modules and is empty for System exactly as it +// is for the user's own modules. Filtering on Source alone therefore lets every +// System element through, which is how QUAL002 came to report FileDocument, +// HttpRequest and 35 other platform entities as undocumented. +const systemModuleID = "00000000-0000-0000-0000-000000000001" + +// notPlatformModule is the WHERE fragment that keeps a query to modules the user +// actually owns: not downloaded from the Marketplace, and not System. +// +// `alias` is the modules-table alias to test. +func notPlatformModule(alias string) string { + return fmt.Sprintf( + `COALESCE(%[1]s.Source, '') = '' AND COALESCE(%[1]s.Id, '') <> '%[2]s'`, + alias, systemModuleID) +} + +// Documentable is one model element that can carry documentation, projected +// uniformly across catalog tables so a rule can sweep every document type +// without a query per kind. +type Documentable struct { + Kind string // Mendix term: "Page", "Enumeration", "Workflow", … + Name string + QualifiedName string + ModuleName string + Description string +} + +// documentableSource maps a catalog table to the Mendix term for what it holds +// and the column its documentation lives in. +// +// The doc column is NOT uniform — Mendix says "Documentation" for some element +// types and "Description" for others, and the catalog faithfully mirrors that. +// A sweep that assumes one spelling silently reports every element of the other +// half as undocumented. +type documentableSource struct { + Table string + Kind string + DocCol string +} + +// documentableSources is every document type a user authors and can document. +// +// Deliberately absent, and why: +// - microflows, java_actions — swept by the rule separately, because they +// carry exemptions (activity thresholds) and children (parameters) that a +// uniform projection cannot express. +// - attributes, java_action_parameters — members of a document, not +// documents; the rule checks them under their own options. +// - activities, widgets, widget_definition_properties, xpath_expressions — +// sub-elements INSIDE a document. Mendix offers no documentation field for +// most of them, and flagging every widget would drown the rule. +// - contract_entities — generated from a remote service's $metadata. Not the +// user's text to write, so not the user's omission to report. +var documentableSources = []documentableSource{ + {"modules", "Module", "Description"}, + {"entities", "Entity", "Description"}, + {"associations", "Association", "Description"}, + {"pages", "Page", "Description"}, + {"snippets", "Snippet", "Description"}, + {"building_blocks", "BuildingBlock", "Description"}, + {"layouts", "Layout", "Description"}, + {"enumerations", "Enumeration", "Description"}, + {"javascript_actions", "JavaScriptAction", "Description"}, + {"image_collections", "ImageCollection", "Description"}, + {"data_transformers", "DataTransformer", "Description"}, + {"workflows", "Workflow", "Description"}, + {"business_event_services", "BusinessEventService", "Documentation"}, + {"rest_clients", "RestClient", "Documentation"}, + {"published_rest_services", "PublishedRestService", "Documentation"}, + {"constants", "Constant", "Description"}, + {"json_structures", "JsonStructure", "Documentation"}, + {"import_mappings", "ImportMapping", "Documentation"}, + {"export_mappings", "ExportMapping", "Documentation"}, +} + +// DocumentableKinds returns the Kind of every source, for tests and for the +// `mxcli lint` help text to stay in step with the code. +func DocumentableKinds() []string { + kinds := make([]string, 0, len(documentableSources)) + for _, s := range documentableSources { + kinds = append(kinds, s.Kind) + } + return kinds +} + +// DocumentableElements iterates every documentable element across all document +// types, excluding System and Marketplace modules. +// +// Each source is queried separately rather than UNIONed so that a catalog +// missing one table (an older cache, or a Mendix version without that document +// type) loses only that kind instead of the whole sweep. +func (ctx *LintContext) DocumentableElements() iter.Seq[Documentable] { + return func(yield func(Documentable) bool) { + for _, src := range documentableSources { + // The modules table has no module to join to — it IS the module + // list — so it filters on its own Source column. + query := fmt.Sprintf(` + SELECT t.Name, t.QualifiedName, t.ModuleName, COALESCE(t.%s, '') + FROM %s t + LEFT JOIN modules m ON t.ModuleName = m.Name + WHERE %s + ORDER BY t.ModuleName, t.Name + `, src.DocCol, src.Table, notPlatformModule("m")) + if src.Table == "modules" { + query = fmt.Sprintf(` + SELECT t.Name, t.QualifiedName, t.Name, COALESCE(t.%s, '') + FROM %s t + WHERE %s + ORDER BY t.Name + `, src.DocCol, src.Table, notPlatformModule("t")) + } + + rows, err := ctx.db.Query(query) + if err != nil { + // Skip this kind only — one absent table must not take down the + // whole sweep — but say so, because "this document type has no + // undocumented elements" and "this document type could not be + // read" look identical in the output otherwise. + ctx.recordQueryError("DocumentableElements("+src.Table+")", err) + continue + } + for rows.Next() { + d := Documentable{Kind: src.Kind} + var qn, mod sql.NullString + if err := rows.Scan(&d.Name, &qn, &mod, &d.Description); err != nil { + ctx.recordQueryError("DocumentableElements("+src.Table+") row scan", err) + continue + } + d.QualifiedName = qn.String + d.ModuleName = mod.String + if d.QualifiedName == "" { + d.QualifiedName = d.Name + } + if ctx.IsExcluded(d.ModuleName) { + continue + } + if !yield(d) { + rows.Close() + return + } + } + rows.Close() + } + } +} diff --git a/mdl/linter/context_queryerror_test.go b/mdl/linter/context_queryerror_test.go new file mode 100644 index 000000000..6dc4279ff --- /dev/null +++ b/mdl/linter/context_queryerror_test.go @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: Apache-2.0 + +package linter + +import ( + "database/sql" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/catalog" +) + +// brokenCatalogDB is a catalog whose `modules` table is missing the Id column +// that platform filtering reads, so every joined query fails with +// "no such column". This is not hypothetical: three test fixtures in this repo +// drifted exactly this way, and the failure reached the tests as +// "expected 1 violation, got 0". +func brokenCatalogDB(t *testing.T) catalog.CatalogDB { + t.Helper() + db, err := sql.Open("sqlite", ":memory:") + if err != nil { + t.Fatalf("open: %v", err) + } + t.Cleanup(func() { db.Close() }) + + for _, q := range []string{ + `CREATE TABLE modules (Name TEXT PRIMARY KEY, Source TEXT)`, // no Id + `CREATE TABLE entities (Id TEXT, Name TEXT, QualifiedName TEXT, ModuleName TEXT, + Folder TEXT, EntityType TEXT, Description TEXT, Generalization TEXT, + AttributeCount INTEGER, AccessRuleCount INTEGER, ValidationRuleCount INTEGER, + HasEventHandlers INTEGER, IsExternal INTEGER)`, + `INSERT INTO modules VALUES ('ModA', '')`, + `INSERT INTO entities VALUES ('e1','E','ModA.E','ModA','','PERSISTENT','','',0,0,0,0,0)`, + } { + if _, err := db.Exec(q); err != nil { + t.Fatalf("exec %s: %v", q, err) + } + } + return catalog.WrapSqlDB(db) +} + +// The defect: an iterator that cannot run its query returned no rows and said +// nothing, so "the catalog is broken" and "your project is clean" produced +// identical output. For a linter that is the worst possible failure — the tool +// reports success precisely when it has checked nothing. +func TestQueryError_BrokenQueryIsReportedNotSilent(t *testing.T) { + ctx := NewLintContextFromDB(brokenCatalogDB(t)) + + var n int + for range ctx.Entities() { + n++ + } + if n != 0 { + t.Fatalf("fixture is not broken: Entities() yielded %d rows", n) + } + + errs := ctx.QueryErrors() + if len(errs) == 0 { + t.Fatal("Entities() failed and reported nothing — a broken query is " + + "indistinguishable from a clean project") + } + if errs[0].Iterator != "Entities" { + t.Errorf("Iterator = %q, want %q — the report must name what failed", errs[0].Iterator, "Entities") + } + if !strings.Contains(errs[0].Err.Error(), "no such column") { + t.Errorf("underlying cause was lost: %v", errs[0].Err) + } + // The message a user sees has to carry both halves. + if msg := errs[0].Error(); !strings.Contains(msg, "Entities") || !strings.Contains(msg, "no such column") { + t.Errorf("QueryError.Error() = %q, want it to name the iterator and the cause", msg) + } +} + +// A healthy catalog must not report anything, or the signal is worthless. +func TestQueryError_HealthyCatalogReportsNothing(t *testing.T) { + ctx := NewLintContextFromDB(setupModuleFilterDB(t)) + + var n int + for range ctx.Entities() { + n++ + } + if n == 0 { + t.Fatal("healthy fixture yielded no entities") + } + if errs := ctx.QueryErrors(); len(errs) != 0 { + t.Errorf("healthy catalog reported query errors: %v", errs) + } +} + +// Every iterator must report, not just the one that happened to be fixed first. +// A partial rollout leaves the same silent hole behind a different accessor. +func TestQueryError_AllIteratorsReport(t *testing.T) { + ctx := NewLintContextFromDB(brokenCatalogDB(t)) + + // Drain the iterators that join modules. Each should fail and say so. + for range ctx.Entities() { + } + for range ctx.Microflows() { + } + for range ctx.Pages() { + } + for range ctx.Enumerations() { + } + for range ctx.Constants() { + } + for range ctx.Snippets() { + } + for range ctx.JavaActions() { + } + for range ctx.DocumentableElements() { + } + ctx.FindUnused("entity") + + seen := map[string]bool{} + for _, e := range ctx.QueryErrors() { + // DocumentableElements labels per table; collapse to the iterator name. + name := e.Iterator + if i := strings.IndexByte(name, '('); i > 0 { + name = name[:i] + } + seen[name] = true + } + + for _, want := range []string{ + "Entities", "Microflows", "Pages", "Enumerations", "Constants", + "Snippets", "JavaActions", "DocumentableElements", "FindUnused", + } { + if !seen[want] { + t.Errorf("%s failed silently — no QueryError recorded (have: %v)", want, seen) + } + } +} + +// Several rules iterate the same accessor. Without dedup, one broken view is +// reported once per rule that touched it, so the user counts failures instead +// of reading them. +func TestQueryError_DeduplicatesRepeatedFailures(t *testing.T) { + ctx := NewLintContextFromDB(brokenCatalogDB(t)) + + for i := 0; i < 3; i++ { + for range ctx.Entities() { + } + } + + errs := ctx.QueryErrors() + if len(errs) != 1 { + t.Errorf("draining Entities() 3 times recorded %d errors, want 1: %v", len(errs), errs) + } +} + +// Dedup must key on the cause, not just the iterator, or a second distinct +// failure in the same accessor is dropped. +func TestQueryError_DistinctCausesBothRecorded(t *testing.T) { + ctx := NewLintContextFromDB(brokenCatalogDB(t)) + ctx.recordQueryError("X", errStub("first")) + ctx.recordQueryError("X", errStub("second")) + ctx.recordQueryError("X", errStub("first")) + + if got := len(ctx.QueryErrors()); got != 2 { + t.Errorf("recorded %d errors, want 2 (two distinct causes): %v", got, ctx.QueryErrors()) + } +} + +type errStub string + +func (e errStub) Error() string { return string(e) } diff --git a/mdl/linter/context_systemmodule_test.go b/mdl/linter/context_systemmodule_test.go new file mode 100644 index 000000000..d6d13275a --- /dev/null +++ b/mdl/linter/context_systemmodule_test.go @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: Apache-2.0 + +package linter_test + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/catalog" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// systemModuleID is the sentinel Mendix gives the built-in System module. +// Restated here rather than exported from the linter: a test that reads the +// constant under test cannot detect that constant being wrong. +const systemModuleID = "00000000-0000-0000-0000-000000000001" + +// systemLeakFixture builds a catalog holding one user module and the System +// module, each with one element in every table the LintContext iterates. +// +// The System rows carry an EMPTY Source, which is the whole point: that column +// cannot distinguish System from a user module, so a filter written only against +// it lets all of System through. +func systemLeakFixture(t *testing.T) *catalog.Catalog { + t.Helper() + + cat, err := catalog.NewFromFile(filepath.Join(t.TempDir(), "cat.db")) + if err != nil { + t.Fatalf("NewFromFile: %v", err) + } + t.Cleanup(func() { cat.Close() }) + db := cat.CatalogDB() + + exec := func(q string, args ...any) { + t.Helper() + if _, err := db.Exec(q, args...); err != nil { + t.Fatalf("exec: %v\n%s", err, q) + } + } + + exec(`INSERT INTO modules_data (Id, Name, QualifiedName, ModuleName, Source, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?)`, "mod-user", "Racing", "Racing", "Racing", "", "default", "s1") + exec(`INSERT INTO modules_data (Id, Name, QualifiedName, ModuleName, Source, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?)`, systemModuleID, "System", "System", "System", "", "default", "s1") + exec(`INSERT INTO modules_data (Id, Name, QualifiedName, ModuleName, Source, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?)`, "mod-mp", "CommunityCommons", "CommunityCommons", + "CommunityCommons", "Marketplace v1.0", "default", "s1") + + // One row per module in each iterated table. Names are suffixed with the + // module so a leak is identifiable from the value alone. + for _, mod := range []string{"Racing", "System", "CommunityCommons"} { + exec(`INSERT INTO entities_data (Id, Name, QualifiedName, ModuleName, EntityType, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?)`, "e-"+mod, "Entity"+mod, mod+".Entity"+mod, mod, "PERSISTENT", "default", "s1") + exec(`INSERT INTO microflows_data (Id, Name, QualifiedName, ModuleName, MicroflowType, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?)`, "mf-"+mod, "MF"+mod, mod+".MF"+mod, mod, "MICROFLOW", "default", "s1") + exec(`INSERT INTO pages_data (Id, Name, QualifiedName, ModuleName, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?)`, "pg-"+mod, "Page"+mod, mod+".Page"+mod, mod, "default", "s1") + exec(`INSERT INTO enumerations_data (Id, Name, QualifiedName, ModuleName, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?)`, "en-"+mod, "Enum"+mod, mod+".Enum"+mod, mod, "default", "s1") + exec(`INSERT INTO constants_data (Id, Name, QualifiedName, ModuleName, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?)`, "c-"+mod, "Const"+mod, mod+".Const"+mod, mod, "default", "s1") + exec(`INSERT INTO snippets_data (Id, Name, QualifiedName, ModuleName, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?)`, "sn-"+mod, "Snip"+mod, mod+".Snip"+mod, mod, "default", "s1") + exec(`INSERT INTO java_actions_data (Id, Name, QualifiedName, ModuleName, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?)`, "ja-"+mod, "JA"+mod, mod+".JA"+mod, mod, "default", "s1") + exec(`INSERT INTO widgets_data (Id, Name, WidgetType, ModuleName, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?)`, "w-"+mod, "Widget"+mod, "TextBox", mod, "default", "s1") + } + + return cat +} + +// Every LintContext iterator must exclude both platform sources. System is the +// one that Source-based filtering misses, and it is by far the larger leak: a +// blank project ships ~40 System entities, so a rule over entities reported 40 +// findings the user could do nothing about. +func TestIterators_ExcludePlatformModules(t *testing.T) { + cat := systemLeakFixture(t) + ctx := linter.NewLintContext(cat, &minimalReader{}) + + // name -> the qualified names that iterator yielded. + got := map[string][]string{} + collect := func(name string, qn ...string) { got[name] = append(got[name], qn...) } + + for e := range ctx.Entities() { + collect("Entities", e.QualifiedName) + } + for mf := range ctx.Microflows() { + collect("Microflows", mf.QualifiedName) + } + for p := range ctx.Pages() { + collect("Pages", p.QualifiedName) + } + for en := range ctx.Enumerations() { + collect("Enumerations", en.QualifiedName) + } + for c := range ctx.Constants() { + collect("Constants", c.QualifiedName) + } + for s := range ctx.Snippets() { + collect("Snippets", s.QualifiedName) + } + for ja := range ctx.JavaActions() { + collect("JavaActions", ja.QualifiedName) + } + for w := range ctx.Widgets() { + collect("Widgets", w.ModuleName+".w") + } + for d := range ctx.DocumentableElements() { + collect("DocumentableElements", d.QualifiedName) + } + for _, kind := range []string{"entity", "microflow", "page"} { + collect("FindUnused("+kind+")", ctx.FindUnused(kind)...) + } + + if len(got) == 0 { + t.Fatal("no iterators were exercised") + } + + for name, names := range got { + joined := strings.Join(names, " ") + if strings.Contains(joined, "System") { + t.Errorf("%s leaked a System element: %v", name, names) + } + if strings.Contains(joined, "CommunityCommons") { + t.Errorf("%s leaked a Marketplace element: %v", name, names) + } + // Guard against the trivial pass: an iterator that returns nothing + // satisfies both assertions above. + if !strings.Contains(joined, "Racing") { + t.Errorf("%s returned no user elements at all (%v) — the assertions above "+ + "would pass even with the filter broken", name, names) + } + } +} diff --git a/mdl/linter/context_test.go b/mdl/linter/context_test.go index 9ad51f18c..72c973132 100644 --- a/mdl/linter/context_test.go +++ b/mdl/linter/context_test.go @@ -20,7 +20,13 @@ func setupModuleFilterDB(t *testing.T) catalog.CatalogDB { t.Fatalf("open db: %v", err) } - _, err = db.Exec(`CREATE TABLE modules (Name TEXT PRIMARY KEY, Source TEXT)`) + // Id is part of the real `modules` view and is load-bearing: platform + // filtering keys off the System module's sentinel Id, because Source cannot + // tell System apart from a user module (both empty). Omitting it here made + // every iterator query error out — and these iterators swallow query errors + // and yield nothing, so the drift surfaced as "no entities were returned" + // rather than "no such column". + _, err = db.Exec(`CREATE TABLE modules (Id TEXT, Name TEXT PRIMARY KEY, Source TEXT)`) if err != nil { t.Fatalf("create modules table: %v", err) } @@ -44,7 +50,7 @@ func setupModuleFilterDB(t *testing.T) catalog.CatalogDB { modules := []string{"ModA", "ModB", "ModC"} for _, mod := range modules { - if _, err := db.Exec(`INSERT INTO modules VALUES (?, '')`, mod); err != nil { + if _, err := db.Exec(`INSERT INTO modules VALUES (?, ?, '')`, mod+"-id", mod); err != nil { t.Fatalf("insert module %s: %v", mod, err) } if _, err := db.Exec(`INSERT INTO entities VALUES (?, ?, ?, ?, '', 'PERSISTENT', '', '', 0, 0, 0, 0, 0)`, diff --git a/mdl/linter/linter.go b/mdl/linter/linter.go index 6faffbedc..aba20fe3c 100644 --- a/mdl/linter/linter.go +++ b/mdl/linter/linter.go @@ -181,6 +181,19 @@ func (l *Linter) Run(ctx context.Context) ([]Violation, error) { return allViolations, nil } +// QueryErrors returns the catalog queries that failed during the run. +// +// A non-empty result means the findings are INCOMPLETE: some iterator could not +// read the catalog and yielded nothing, which is indistinguishable from a clean +// project in the violation list alone. Callers should report these rather than +// present the run as successful. +func (l *Linter) QueryErrors() []QueryError { + if l.ctx == nil { + return nil + } + return l.ctx.QueryErrors() +} + // Summary holds counts of violations by severity. type Summary struct { Errors int diff --git a/mdl/linter/linter_test.go b/mdl/linter/linter_test.go index f023e8a18..1182c3daa 100644 --- a/mdl/linter/linter_test.go +++ b/mdl/linter/linter_test.go @@ -12,11 +12,11 @@ type stubRule struct { id string } -func (r *stubRule) ID() string { return r.id } -func (r *stubRule) Name() string { return r.id } -func (r *stubRule) Description() string { return "" } -func (r *stubRule) DefaultSeverity() Severity { return SeverityWarning } -func (r *stubRule) Category() string { return "test" } +func (r *stubRule) ID() string { return r.id } +func (r *stubRule) Name() string { return r.id } +func (r *stubRule) Description() string { return "" } +func (r *stubRule) DefaultSeverity() Severity { return SeverityWarning } +func (r *stubRule) Category() string { return "test" } func (r *stubRule) Check(_ *LintContext) []Violation { return []Violation{{RuleID: r.id, Severity: SeverityWarning, Message: "hit"}} } diff --git a/mdl/linter/rules/empty_test.go b/mdl/linter/rules/empty_test.go index 6df2611ef..7a2660e06 100644 --- a/mdl/linter/rules/empty_test.go +++ b/mdl/linter/rules/empty_test.go @@ -21,7 +21,11 @@ func setupMicroflowsDB(t *testing.T, rows [][]any) catalog.CatalogDB { t.Fatalf("failed to open in-memory db: %v", err) } - _, err = db.Exec(`CREATE TABLE modules (Name TEXT PRIMARY KEY, Source TEXT)`) + // Id mirrors the real `modules` view. Platform filtering keys off the System + // module's sentinel Id — Source is empty for System exactly as it is for a + // user module — and these iterators swallow query errors, so a missing + // column shows up as "no rows" rather than "no such column". + _, err = db.Exec(`CREATE TABLE modules (Id TEXT, Name TEXT PRIMARY KEY, Source TEXT)`) if err != nil { t.Fatalf("failed to create modules table: %v", err) } @@ -43,7 +47,7 @@ func setupMicroflowsDB(t *testing.T, rows [][]any) catalog.CatalogDB { } // Ensure module exists moduleName := row[3].(string) - if _, err := db.Exec(`INSERT OR IGNORE INTO modules (Name, Source) VALUES (?, '')`, moduleName); err != nil { + if _, err := db.Exec(`INSERT OR IGNORE INTO modules (Id, Name, Source) VALUES (?, ?, '')`, moduleName+"-id", moduleName); err != nil { t.Fatalf("failed to insert module: %v", err) } } diff --git a/mdl/linter/rules/helpers_test.go b/mdl/linter/rules/helpers_test.go index dae123b98..7b233342e 100644 --- a/mdl/linter/rules/helpers_test.go +++ b/mdl/linter/rules/helpers_test.go @@ -30,7 +30,11 @@ func setupEntitiesDB(t *testing.T, entities [][]any) catalog.CatalogDB { t.Fatalf("failed to open in-memory db: %v", err) } - _, err = db.Exec(`CREATE TABLE modules (Name TEXT PRIMARY KEY, Source TEXT)`) + // Id mirrors the real `modules` view. Platform filtering keys off the System + // module's sentinel Id — Source is empty for System exactly as it is for a + // user module — and these iterators swallow query errors, so a missing + // column shows up as "no rows" rather than "no such column". + _, err = db.Exec(`CREATE TABLE modules (Id TEXT, Name TEXT PRIMARY KEY, Source TEXT)`) if err != nil { t.Fatalf("failed to create modules table: %v", err) } @@ -52,7 +56,7 @@ func setupEntitiesDB(t *testing.T, entities [][]any) catalog.CatalogDB { t.Fatalf("failed to insert entity: %v", err) } moduleName := row[3].(string) - if _, err := db.Exec(`INSERT OR IGNORE INTO modules (Name, Source) VALUES (?, '')`, moduleName); err != nil { + if _, err := db.Exec(`INSERT OR IGNORE INTO modules (Id, Name, Source) VALUES (?, ?, '')`, moduleName+"-id", moduleName); err != nil { t.Fatalf("failed to insert module: %v", err) } } diff --git a/mdl/linter/starlark.go b/mdl/linter/starlark.go index 716b6f73a..88623fa7f 100644 --- a/mdl/linter/starlark.go +++ b/mdl/linter/starlark.go @@ -314,16 +314,18 @@ func LoadStarlarkRule(path string) (*StarlarkRule, error) { func (r *StarlarkRule) buildPredeclared() starlark.StringDict { return starlark.StringDict{ // Query functions - "entities": starlark.NewBuiltin("entities", r.builtinEntities), - "microflows": starlark.NewBuiltin("microflows", r.builtinMicroflows), - "pages": starlark.NewBuiltin("pages", r.builtinPages), - "enumerations": starlark.NewBuiltin("enumerations", r.builtinEnumerations), - "constants": starlark.NewBuiltin("constants", r.builtinConstants), - "widgets": starlark.NewBuiltin("widgets", r.builtinWidgets), - "refs_to": starlark.NewBuiltin("refs_to", r.builtinRefsTo), - "refs_from": starlark.NewBuiltin("refs_from", r.builtinRefsFrom), - "attributes_for": starlark.NewBuiltin("attributes_for", r.builtinAttributesFor), - "scheduled_events": starlark.NewBuiltin("scheduled_events", r.builtinScheduledEvents), + "entities": starlark.NewBuiltin("entities", r.builtinEntities), + "microflows": starlark.NewBuiltin("microflows", r.builtinMicroflows), + "java_actions": starlark.NewBuiltin("java_actions", r.builtinJavaActions), + "documentable_elements": starlark.NewBuiltin("documentable_elements", r.builtinDocumentableElements), + "pages": starlark.NewBuiltin("pages", r.builtinPages), + "enumerations": starlark.NewBuiltin("enumerations", r.builtinEnumerations), + "constants": starlark.NewBuiltin("constants", r.builtinConstants), + "widgets": starlark.NewBuiltin("widgets", r.builtinWidgets), + "refs_to": starlark.NewBuiltin("refs_to", r.builtinRefsTo), + "refs_from": starlark.NewBuiltin("refs_from", r.builtinRefsFrom), + "attributes_for": starlark.NewBuiltin("attributes_for", r.builtinAttributesFor), + "scheduled_events": starlark.NewBuiltin("scheduled_events", r.builtinScheduledEvents), // Graph-analysis facts (populated by `refresh catalog communities`). "community_of": starlark.NewBuiltin("community_of", r.builtinCommunityOf), @@ -394,6 +396,49 @@ func (r *StarlarkRule) builtinMicroflows(_ *starlark.Thread, _ *starlark.Builtin return starlark.NewList(microflows), nil } +// builtinJavaActions returns an iterator over Java actions. Each carries its +// parameters, so a rule reporting an undocumented parameter can name its action +// without a second lookup. +func (r *StarlarkRule) builtinJavaActions(_ *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { + if r.ctx == nil { + return starlark.NewList(nil), nil + } + + var actions []starlark.Value + for ja := range r.ctx.JavaActions() { + actions = append(actions, javaActionToStarlark(ja)) + } + + return starlark.NewList(actions), nil +} + +// builtinDocumentableElements returns every element that can carry +// documentation, across all document types, as a uniform (kind, name, +// qualified_name, module_name, description) projection. +// +// One builtin rather than nineteen: a rule sweeping for missing documentation +// wants "every document", and a new Mendix document type should be covered by +// adding a row to documentableSources, not by writing another builtin and +// remembering to call it. +func (r *StarlarkRule) builtinDocumentableElements(_ *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { + if r.ctx == nil { + return starlark.NewList(nil), nil + } + + var out []starlark.Value + for d := range r.ctx.DocumentableElements() { + out = append(out, starlarkstruct.FromStringDict(starlark.String("documentable"), starlark.StringDict{ + "kind": starlark.String(d.Kind), + "name": starlark.String(d.Name), + "qualified_name": starlark.String(d.QualifiedName), + "module_name": starlark.String(d.ModuleName), + "description": starlark.String(d.Description), + })) + } + + return starlark.NewList(out), nil +} + // builtinPages returns an iterator over pages. func (r *StarlarkRule) builtinPages(_ *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { if r.ctx == nil { @@ -745,6 +790,37 @@ func microflowToStarlark(mf Microflow) starlark.Value { }) } +// javaActionToStarlark converts a JavaAction to a Starlark struct. +// +// The doc field is exposed as BOTH `documentation` (the Mendix term, and what +// the metamodel calls it) and `description` (what every other document struct +// here calls it), so a rule that loops over mixed document kinds can read one +// field name throughout. +func javaActionToStarlark(ja JavaAction) starlark.Value { + params := make([]starlark.Value, 0, len(ja.Parameters)) + for _, p := range ja.Parameters { + params = append(params, starlarkstruct.FromStringDict(starlark.String("java_action_parameter"), starlark.StringDict{ + "name": starlark.String(p.Name), + "description": starlark.String(p.Description), + "parameter_type": starlark.String(p.ParameterType), + "is_required": starlark.Bool(p.IsRequired), + })) + } + return starlarkstruct.FromStringDict(starlark.String("java_action"), starlark.StringDict{ + "id": starlark.String(ja.ID), + "name": starlark.String(ja.Name), + "qualified_name": starlark.String(ja.QualifiedName), + "module_name": starlark.String(ja.ModuleName), + "folder": starlark.String(ja.Folder), + "documentation": starlark.String(ja.Documentation), + "description": starlark.String(ja.Documentation), + "export_level": starlark.String(ja.ExportLevel), + "return_type": starlark.String(ja.ReturnType), + "parameter_count": starlark.MakeInt(len(ja.Parameters)), + "parameters": starlark.NewList(params), + }) +} + // pageToStarlark converts a Page to a Starlark struct. func pageToStarlark(p Page) starlark.Value { return starlarkstruct.FromStringDict(starlark.String("page"), starlark.StringDict{ diff --git a/mdl/linter/starlark_javaactions_test.go b/mdl/linter/starlark_javaactions_test.go new file mode 100644 index 000000000..09fb292c9 --- /dev/null +++ b/mdl/linter/starlark_javaactions_test.go @@ -0,0 +1,445 @@ +// SPDX-License-Identifier: Apache-2.0 + +package linter_test + +import ( + "database/sql" + "fmt" + "path/filepath" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/catalog" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// javaActionFixture builds a catalog holding one module, two Java actions (one +// documented, one not) and three parameters (one documented, two not). +// +// Rows are inserted directly rather than round-tripped through a reader because +// the subject here is the query + Starlark surface, not the MPR parser. +func javaActionFixture(t *testing.T) *catalog.Catalog { + t.Helper() + + cat, err := catalog.NewFromFile(filepath.Join(t.TempDir(), "cat.db")) + if err != nil { + t.Fatalf("NewFromFile: %v", err) + } + t.Cleanup(func() { cat.Close() }) + db := cat.CatalogDB() + + exec := func(q string, args ...any) { + t.Helper() + if _, err := db.Exec(q, args...); err != nil { + t.Fatalf("exec %s: %v", q, err) + } + } + + exec(`INSERT INTO modules_data (Id, Name, ProjectId, SnapshotId) VALUES (?,?,?,?)`, + "mod-1", "Formula1Backend", "default", "s1") + + insertAction := func(id, name, doc string) { + exec(`INSERT INTO java_actions_data + (Id, Name, QualifiedName, ModuleName, Folder, Documentation, ExportLevel, + ReturnType, ParameterCount, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?,?,?,?)`, + id, name, "Formula1Backend."+name, "Formula1Backend", "", doc, "Hidden", + "String", 0, "default", "s1") + } + insertParam := func(id, actionID, actionName, name, desc string, ordinal int) { + exec(`INSERT INTO java_action_parameters_data + (Id, JavaActionId, JavaActionName, QualifiedName, ModuleName, Name, + Description, ParameterType, IsRequired, Ordinal, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?,?,?,?,?)`, + id, actionID, actionName, "Formula1Backend."+actionName+"."+name, + "Formula1Backend", name, desc, "String", 1, ordinal, "default", "s1") + } + + insertAction("ja-1", "ParseTop", "") // undocumented + insertAction("ja-2", "FormatLapTime", "Renders a lap time.") // documented + insertParam("p-1", "ja-1", "ParseTop", "pUri", "", 0) // undocumented + insertParam("p-2", "ja-1", "ParseTop", "pDefault", "", 1) // undocumented + insertParam("p-3", "ja-2", "FormatLapTime", "pMillis", "Elapsed milliseconds.", 0) + + return cat +} + +// The catalog stored only a parameter COUNT, so nothing downstream could see a +// parameter's description — the field a caller reads in Studio Pro's action +// dialog. This asserts the rows land and arrive attached to their action, in +// declaration order. +func TestJavaActions_CarryTheirParameters(t *testing.T) { + ctx := linter.NewLintContext(javaActionFixture(t), &minimalReader{}) + + byName := map[string]linter.JavaAction{} + for ja := range ctx.JavaActions() { + byName[ja.Name] = ja + } + if len(byName) != 2 { + t.Fatalf("got %d Java actions, want 2: %v", len(byName), byName) + } + + parse := byName["ParseTop"] + if parse.Documentation != "" { + t.Errorf("ParseTop documentation = %q, want empty", parse.Documentation) + } + if len(parse.Parameters) != 2 { + t.Fatalf("ParseTop has %d parameters, want 2", len(parse.Parameters)) + } + // Ordinal, not alphabetical: a signature read out of order is misleading. + if parse.Parameters[0].Name != "pUri" || parse.Parameters[1].Name != "pDefault" { + t.Errorf("parameters out of declaration order: %v", parse.Parameters) + } + if parse.Parameters[0].ParameterType != "String" || !parse.Parameters[0].IsRequired { + t.Errorf("parameter metadata lost: %+v", parse.Parameters[0]) + } + + fmtLap := byName["FormatLapTime"] + if fmtLap.Documentation == "" { + t.Error("FormatLapTime documentation was dropped") + } + if len(fmtLap.Parameters) != 1 || fmtLap.Parameters[0].Description == "" { + t.Errorf("FormatLapTime parameter description lost: %+v", fmtLap.Parameters) + } +} + +// Marketplace and system modules carry code the user did not write, and every +// other document iterator here excludes them. Reporting "the Community Commons +// Java actions are undocumented" would bury the findings that are actionable. +func TestJavaActions_ExcludeMarketplaceModules(t *testing.T) { + cat := javaActionFixture(t) + db := cat.CatalogDB() + if _, err := db.Exec( + `INSERT INTO modules_data (Id, Name, Source, ProjectId, SnapshotId) VALUES (?,?,?,?,?)`, + "mod-2", "CommunityCommons", "Marketplace", "default", "s1"); err != nil { + t.Fatalf("insert module: %v", err) + } + if _, err := db.Exec(`INSERT INTO java_actions_data + (Id, Name, QualifiedName, ModuleName, Folder, Documentation, ExportLevel, + ReturnType, ParameterCount, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?,?,?,?)`, + "ja-9", "StringUtil", "CommunityCommons.StringUtil", "CommunityCommons", "", + "", "Hidden", "String", 0, "default", "s1"); err != nil { + t.Fatalf("insert action: %v", err) + } + + ctx := linter.NewLintContext(cat, &minimalReader{}) + for ja := range ctx.JavaActions() { + if ja.ModuleName == "CommunityCommons" { + t.Errorf("a Marketplace module's Java action was reported: %s", ja.QualifiedName) + } + } +} + +// The rule under test is the one that ships, loaded from disk — not a copy +// inlined here. A test against an inlined copy proves the Starlark builtin works +// and says nothing about whether QUAL002 uses it. +func runMissingDocRule(t *testing.T, cat *catalog.Catalog, options map[string]any) []linter.Violation { + t.Helper() + rule, err := linter.LoadStarlarkRule("../../.claude/lint-rules/missing_documentation.star") + if err != nil { + t.Fatalf("loading the shipped QUAL002: %v", err) + } + if options != nil { + rule.Configure(options) + } + return rule.Check(linter.NewLintContext(cat, &minimalReader{})) +} + +// mxcli-formula1: entities and microflows were covered; Java actions and their +// parameters were not reachable from Starlark at all. +func TestQUAL002_FlagsUndocumentedJavaActionsAndParameters(t *testing.T) { + vs := runMissingDocRule(t, javaActionFixture(t), nil) + + var messages []string + for _, v := range vs { + messages = append(messages, v.Message) + } + joined := strings.Join(messages, "\n") + + for _, want := range []string{ + "Java action 'ParseTop' has no documentation.", + "Java action parameter 'ParseTop.pUri' has no description.", + "Java action parameter 'ParseTop.pDefault' has no description.", + } { + if !strings.Contains(joined, want) { + t.Errorf("QUAL002 did not report %q; got:\n%s", want, joined) + } + } + // The documented action and its documented parameter must stay quiet, + // otherwise the rule is noise rather than a signal. + if strings.Contains(joined, "FormatLapTime") { + t.Errorf("a documented action was flagged:\n%s", joined) + } +} + +// Each target is switchable, so a project that has decided Java actions are +// self-describing can silence just that half without losing the rest. +func TestQUAL002_TargetsAreIndividuallySwitchable(t *testing.T) { + cat := javaActionFixture(t) + + noParams := runMissingDocRule(t, cat, map[string]any{"check_java_action_params": false}) + for _, v := range noParams { + if strings.Contains(v.Message, "parameter") { + t.Errorf("check_java_action_params=false still reported: %s", v.Message) + } + } + if len(noParams) == 0 { + t.Error("switching off parameters must not switch off the actions too") + } + + noActions := runMissingDocRule(t, cat, map[string]any{"check_java_actions": false}) + for _, v := range noActions { + if strings.Contains(v.Message, "Java action '") { + t.Errorf("check_java_actions=false still reported: %s", v.Message) + } + } +} + +// A guard against the fixture silently rotting: if the schema loses the +// parameters table, every assertion above would pass by reporting nothing. +func TestJavaActionParametersTableExists(t *testing.T) { + cat := javaActionFixture(t) + var n int + if err := cat.CatalogDB().QueryRow( + `SELECT COUNT(*) FROM java_action_parameters`).Scan(&n); err != nil && err != sql.ErrNoRows { + t.Fatalf("java_action_parameters is not queryable: %v", err) + } + if n != 3 { + t.Errorf("java_action_parameters holds %d rows, want 3", n) + } +} + +// allKindsFixture inserts exactly one undocumented element of every kind the +// generic sweep claims to cover, driven off DocumentableKinds() so a kind added +// in Go without a test row fails here rather than shipping unswept. +func allKindsFixture(t *testing.T) (*catalog.Catalog, map[string]string) { + t.Helper() + + cat, err := catalog.NewFromFile(filepath.Join(t.TempDir(), "cat.db")) + if err != nil { + t.Fatalf("NewFromFile: %v", err) + } + t.Cleanup(func() { cat.Close() }) + db := cat.CatalogDB() + + // table -> doc column, mirroring documentableSources. + tables := map[string]struct { + kind string + docCol string + }{ + "entities": {"Entity", "Description"}, + "associations": {"Association", "Description"}, + "pages": {"Page", "Description"}, + "snippets": {"Snippet", "Description"}, + "building_blocks": {"BuildingBlock", "Description"}, + "layouts": {"Layout", "Description"}, + "enumerations": {"Enumeration", "Description"}, + "javascript_actions": {"JavaScriptAction", "Description"}, + "image_collections": {"ImageCollection", "Description"}, + "data_transformers": {"DataTransformer", "Description"}, + "workflows": {"Workflow", "Description"}, + "business_event_services": {"BusinessEventService", "Documentation"}, + "rest_clients": {"RestClient", "Documentation"}, + "published_rest_services": {"PublishedRestService", "Documentation"}, + "constants": {"Constant", "Description"}, + "json_structures": {"JsonStructure", "Documentation"}, + "import_mappings": {"ImportMapping", "Documentation"}, + "export_mappings": {"ExportMapping", "Documentation"}, + } + + if _, err := db.Exec( + `INSERT INTO modules_data (Id, Name, QualifiedName, ModuleName, Description, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?)`, + "mod-1", "Racing", "Racing", "Racing", "", "default", "s1"); err != nil { + t.Fatalf("insert module: %v", err) + } + + // kind -> the element name the sweep should report. + // + // Id is omitted deliberately: json_structures, import_mappings and + // export_mappings declare `Id INTEGER PRIMARY KEY AUTOINCREMENT` while every + // other table uses `Id TEXT PRIMARY KEY`, so a synthetic string id is a + // datatype mismatch on exactly those three. + want := map[string]string{"Module": "Racing"} + for table, meta := range tables { + name := meta.kind + "X" + q := fmt.Sprintf( + `INSERT INTO %s_data (Name, QualifiedName, ModuleName, %s, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?)`, table, meta.docCol) + if _, err := db.Exec(q, name, "Racing."+name, "Racing", + "", "default", "s1"); err != nil { + t.Fatalf("insert into %s: %v", table, err) + } + want[meta.kind] = name + } + return cat, want +} + +// Every kind the Go side advertises must actually be swept. Without this, a new +// documentableSources row that the .star table does not know about is silently +// skipped by the rule's `entry == None` guard — covered in Go, invisible in +// practice. +func TestQUAL002_SweepsEveryAdvertisedDocumentType(t *testing.T) { + cat, want := allKindsFixture(t) + + // The Go-side list and the fixture must agree, or the assertion below is + // only as complete as whichever is shorter. + for _, kind := range linter.DocumentableKinds() { + if _, ok := want[kind]; !ok { + t.Fatalf("DocumentableKinds() advertises %q but the fixture inserts no such row — "+ + "add it here, or the sweep for that kind is untested", kind) + } + } + + var msgs []string + for _, v := range runMissingDocRule(t, cat, map[string]any{"check_associations": true}) { + msgs = append(msgs, v.Message) + } + joined := strings.Join(msgs, "\n") + + for kind, name := range want { + if !strings.Contains(joined, "'"+name+"'") { + t.Errorf("kind %s (element %q) was not reported by QUAL002; got:\n%s", kind, name, joined) + } + } +} + +// A documented element of every kind must produce silence. A sweep that reports +// regardless of content would pass the test above just as well. +func TestQUAL002_DocumentedElementsAreSilent(t *testing.T) { + cat, _ := allKindsFixture(t) + db := cat.CatalogDB() + for _, tbl := range []string{"modules", "pages", "workflows", "constants"} { + col := "Description" + if _, err := db.Exec(fmt.Sprintf( + `UPDATE %s_data SET %s = 'Documented.'`, tbl, col)); err != nil { + t.Fatalf("update %s: %v", tbl, err) + } + } + + var msgs []string + for _, v := range runMissingDocRule(t, cat, nil) { + msgs = append(msgs, v.Message) + } + joined := strings.Join(msgs, "\n") + + for _, quiet := range []string{"'Racing'", "'PageX'", "'WorkflowX'", "'ConstantX'"} { + if strings.Contains(joined, quiet) { + t.Errorf("a documented element %s was still flagged:\n%s", quiet, joined) + } + } + // ...while the ones left blank must still be reported, proving the run + // itself was not simply empty. + if !strings.Contains(joined, "'LayoutX'") { + t.Errorf("the sweep went quiet altogether; expected LayoutX:\n%s", joined) + } +} + +// Associations default OFF, like attributes: a real domain model has as many +// associations as entities and none are documented, so defaulting them on would +// double the rule's output with findings nobody asked for. +func TestQUAL002_AssociationsDefaultOff(t *testing.T) { + cat, _ := allKindsFixture(t) + + var off []string + for _, v := range runMissingDocRule(t, cat, nil) { + off = append(off, v.Message) + } + if strings.Contains(strings.Join(off, "\n"), "AssociationX") { + t.Error("associations were reported without being switched on") + } + + var on []string + for _, v := range runMissingDocRule(t, cat, map[string]any{"check_associations": true}) { + on = append(on, v.Message) + } + if !strings.Contains(strings.Join(on, "\n"), "AssociationX") { + t.Error("check_associations: true did not switch associations on") + } +} + +// The generic sweep needs its own Marketplace test: TestJavaActions_Exclude… +// covers only the Java action query, so dropping the filter from +// DocumentableElements left every other kind unguarded with a green suite. +func TestQUAL002_SweepExcludesMarketplaceModules(t *testing.T) { + cat, _ := allKindsFixture(t) + db := cat.CatalogDB() + + if _, err := db.Exec( + `INSERT INTO modules_data (Id, Name, QualifiedName, ModuleName, Source, Description, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?)`, + "mod-mp", "CommunityCommons", "CommunityCommons", "CommunityCommons", + "Marketplace", "", "default", "s1"); err != nil { + t.Fatalf("insert module: %v", err) + } + // One element per shape: a page (joins modules by name) and the Marketplace + // module row itself (filters on its own Source column — a different code + // path, and the one an all-tables loop is most likely to forget). + if _, err := db.Exec( + `INSERT INTO pages_data (Id, Name, QualifiedName, ModuleName, Description, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?)`, + "pg-mp", "CommonsPage", "CommunityCommons.CommonsPage", "CommunityCommons", + "", "default", "s1"); err != nil { + t.Fatalf("insert page: %v", err) + } + + var msgs []string + for _, v := range runMissingDocRule(t, cat, map[string]any{"check_associations": true}) { + msgs = append(msgs, v.Message) + } + joined := strings.Join(msgs, "\n") + + for _, leaked := range []string{"CommonsPage", "CommunityCommons"} { + if strings.Contains(joined, leaked) { + t.Errorf("Marketplace element %q was reported:\n%s", leaked, joined) + } + } + // The user's own elements must still come through, or this passes trivially. + if !strings.Contains(joined, "PageX") { + t.Errorf("the sweep excluded everything, not just Marketplace:\n%s", joined) + } +} + +// System is the trap that Source-based filtering does not catch: the built-in +// module's Source is empty, exactly like a user module's, so a WHERE on Source +// alone lets all ~40 platform entities through. QUAL002 shipped that way before +// this sweep existed — FileDocument and HttpRequest were reported as +// undocumented on every project. +func TestQUAL002_ExcludesTheSystemModule(t *testing.T) { + cat, _ := allKindsFixture(t) + db := cat.CatalogDB() + + // Note the empty Source: System is indistinguishable from a user module on + // that column alone. Only the sentinel Id separates them. + if _, err := db.Exec( + `INSERT INTO modules_data (Id, Name, QualifiedName, ModuleName, Source, Description, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?)`, + "00000000-0000-0000-0000-000000000001", "System", "System", "System", + "", "", "default", "s1"); err != nil { + t.Fatalf("insert System module: %v", err) + } + if _, err := db.Exec( + `INSERT INTO entities_data (Id, Name, QualifiedName, ModuleName, Description, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?)`, + "sys-e1", "FileDocument", "System.FileDocument", "System", + "", "default", "s1"); err != nil { + t.Fatalf("insert System entity: %v", err) + } + + var msgs []string + for _, v := range runMissingDocRule(t, cat, map[string]any{"check_associations": true}) { + msgs = append(msgs, v.Message) + } + joined := strings.Join(msgs, "\n") + + if strings.Contains(joined, "FileDocument") { + t.Errorf("a System entity was reported:\n%s", joined) + } + if strings.Contains(joined, "'System'") { + t.Errorf("the System module itself was reported:\n%s", joined) + } + if !strings.Contains(joined, "'Racing'") { + t.Errorf("the user's own module stopped being reported:\n%s", joined) + } +} diff --git a/mdl/types/edmx.go b/mdl/types/edmx.go index 2c21d236b..e57f526e3 100644 --- a/mdl/types/edmx.go +++ b/mdl/types/edmx.go @@ -94,6 +94,25 @@ type EdmEntitySet struct { NonFilterableProperties []string NonSortableProperties []string + // Filterable / Sortable are the WHOLE-SET form of the same two restrictions, + // carried as the record's own Bool property: + // + // + // + // + // + // Mendix picks the shape by arithmetic, not by preference: it lists + // NonFilterableProperties when SOME attributes are filterable, and emits the + // bare Bool when NONE are, because then there is no list to write. Both + // appear in one document. nil means unstated, which is OData's default of + // true (mxcli-formula1 §48). + Filterable *bool + Sortable *bool + + // Use AttrFilterable / AttrSortable rather than reading the four fields + // above: a consumer that consults only one shape generates an app the + // publisher's own contract contradicts. + // Navigation property names listed under // Org.OData.Capabilities.V1.{Insert,Update}Restrictions/Non*NavigationProperties. NonInsertableNavigationProperties []string @@ -445,14 +464,35 @@ func applyCapabilityAnnotations(es *EdmEntitySet, annotations []xmlCapabilitiesA } case "Org.OData.Capabilities.V1.FilterRestrictions": for _, pv := range ann.Record.PropertyValues { - if pv.Property == "NonFilterableProperties" && pv.Collection != nil { - es.NonFilterableProperties = pv.Collection.PropertyPaths + switch pv.Property { + case "Filterable": + // The whole-set form. Mendix emits this INSTEAD of a + // NonFilterableProperties list when NO attribute is + // filterable — there is nothing to enumerate — so reading + // only the list makes an entirely unfilterable set look + // entirely filterable. + if pv.Bool != "" { + v := pv.Bool == "true" + es.Filterable = &v + } + case "NonFilterableProperties": + if pv.Collection != nil { + es.NonFilterableProperties = pv.Collection.PropertyPaths + } } } case "Org.OData.Capabilities.V1.SortRestrictions": for _, pv := range ann.Record.PropertyValues { - if pv.Property == "NonSortableProperties" && pv.Collection != nil { - es.NonSortableProperties = pv.Collection.PropertyPaths + switch pv.Property { + case "Sortable": + if pv.Bool != "" { + v := pv.Bool == "true" + es.Sortable = &v + } + case "NonSortableProperties": + if pv.Collection != nil { + es.NonSortableProperties = pv.Collection.PropertyPaths + } } } } @@ -629,3 +669,50 @@ type xmlEnumMember struct { Name string `xml:"Name,attr"` Value string `xml:"Value,attr"` } + +// AttrFilterable reports whether a client may filter on the named property. +// +// FilterRestrictions has TWO shapes and a consumer must honour both. Mendix +// chooses between them by arithmetic rather than preference: it lists +// NonFilterableProperties when SOME attributes are filterable, and emits a bare +// `Bool="false" Property="Filterable"` when NONE are, because then there is no +// list to write. Both shapes appear in one document, on different entity sets. +// +// Reading only the list marks every property of a wholly-unfilterable set as +// filterable, and the consuming app then fails to build — one CE6630 per +// property ("'message' is marked Sortable=False in the OData service, but True +// in the app"), 28 of them on one service (mxcli-formula1 §48). This is §42 one +// layer along, so the two live behind one call to keep them from drifting apart +// again. +// +// A nil receiver, or an unstated restriction, means OData's default: allowed. +func (es *EdmEntitySet) AttrFilterable(property string) bool { + if es == nil { + return true + } + if es.Filterable != nil && !*es.Filterable { + return false + } + return !containsString(es.NonFilterableProperties, property) +} + +// AttrSortable reports whether a client may order by the named property. See +// AttrFilterable — SortRestrictions carries the identical pair of shapes. +func (es *EdmEntitySet) AttrSortable(property string) bool { + if es == nil { + return true + } + if es.Sortable != nil && !*es.Sortable { + return false + } + return !containsString(es.NonSortableProperties, property) +} + +func containsString(haystack []string, needle string) bool { + for _, s := range haystack { + if s == needle { + return true + } + } + return false +} diff --git a/mdl/types/edmx_test.go b/mdl/types/edmx_test.go index 3a13b4b01..4d1a963af 100644 --- a/mdl/types/edmx_test.go +++ b/mdl/types/edmx_test.go @@ -590,3 +590,96 @@ func TestParseEdmx_StandaloneHandlingKeepsRecordAnnotations(t *testing.T) { t.Errorf("Countable = %v — the record arm regressed", es.Countable) } } + +// edmxWithAnnotations wraps entity-set annotations in a minimal but real EDMX +// document, so the assertions run through ParseEdmx rather than a hand-built +// struct. +func edmxWithAnnotations(setName, annotations string) string { + return ` + + + + + + + + + +` + annotations + ` + + + + +` +} + +// mxcli-formula1 §48: FilterRestrictions/SortRestrictions carry the SAME +// two-shape problem §42 had. Mendix emits the bare record boolean when NO +// attribute is filterable — there is no list to enumerate — and mxcli read only +// the list, so a wholly-unfilterable set generated a wholly-filterable app: +// 28 × CE6630 on one service. +func TestParseEdmx_WholeSetFilterAndSortRestrictions(t *testing.T) { + doc, err := ParseEdmx(edmxWithAnnotations("Predictions", ` + + + + + + `)) + if err != nil { + t.Fatalf("parsing: %v", err) + } + es := doc.EntitySets[0] + if es.Filterable == nil || *es.Filterable { + t.Errorf("Filterable = %v, want an explicit false", es.Filterable) + } + if es.Sortable == nil || *es.Sortable { + t.Errorf("Sortable = %v, want an explicit false", es.Sortable) + } + // The whole point: no property escapes a whole-set restriction. + if es.AttrFilterable("message") { + t.Error("'message' is filterable against a set that says nothing is — CE6630") + } + if es.AttrSortable("message") { + t.Error("'message' is sortable against a set that says nothing is — CE6630") + } +} + +// The list shape is the one that already worked, and it has to keep working — +// Mendix emits BOTH, in one document, on different entity sets. +func TestParseEdmx_PerPropertyFilterRestrictionsStillHonoured(t *testing.T) { + doc, err := ParseEdmx(edmxWithAnnotations("DriverForm", ` + + + + message + + `)) + if err != nil { + t.Fatalf("parsing: %v", err) + } + es := doc.EntitySets[0] + if got := es.NonFilterableProperties; len(got) != 1 || got[0] != "message" { + t.Errorf("NonFilterableProperties = %v, want [message]", got) + } + if es.AttrFilterable("message") { + t.Error("'message' is named non-filterable and must not be filterable") + } + if !es.AttrFilterable("k") { + t.Error("'k' is not named, and the set says Filterable=true, so it must stay filterable") + } +} + +// Unstated is OData's default of allowed. Defaulting the other way would invert +// CE6630 for every service that annotates nothing — i.e. every one that worked +// before this change. +func TestEdmEntitySet_UnrestrictedByDefault(t *testing.T) { + var nilSet *EdmEntitySet + if !nilSet.AttrFilterable("anything") || !nilSet.AttrSortable("anything") { + t.Error("a nil entity set must not restrict anything") + } + empty := &EdmEntitySet{Name: "Rows"} + if !empty.AttrFilterable("k") || !empty.AttrSortable("k") { + t.Error("an unannotated set must not restrict anything") + } +}