8000 fix(parser): reject a fact with more than four elements, PRS-108 (#649) [v2.x backport] by adityamukho · Pull Request #656 · project-minigraf/minigraf · GitHub
[go: up one dir, main page]

Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- **A fact with more than four elements is rejected** (#649). `transact` read a fact's 4th element as its optional valid-time map and ignored everything after it, so `[[:alice :person/age 1 {} 99 :junk]]` stored `[:alice :person/age 1]` and dropped the rest without an error; two facts run together by a misplaced bracket lost one. A fact of more than four elements now fails with the new `PRS-108`, numbered as in v3.0.0, and the transaction writes nothing. A fact of five or more elements whose 4th is not a map now gets `PRS-108` instead of `PRS-047`. Present since v0.4.0.

## v2.0.8 — 2026-10-20

Patch release on the v2.x line under the [support policy](PHILOSOPHY.md#support-policy): seven data-integrity fixes for queries that silently returned too few or too many rows — `:db/valid-at` in `not-join` bodies (#615) and rule definitions (#617), `or` / `or-join` branches (#620, #625, #637), and rules that call or recurse through a rule with `not`, `not-join`, `or` or `or-join` (#628, #633). File format is unchanged (v7). No API changes. Stored data was never affected. Upgrading is recommended for every v2.x user who uses `or`, `not-join`, `:db/valid-at` or rules with negation or disjunction.
Expand Down
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ Auto-migrates v1/v2/v3/v4/v5/v6 → v7 on open/checkpoint.

## Test Coverage

**1311 tests** (1302 passing, 9 ignored; unit + integration + doc).
**1313 tests** (1304 passing, 9 ignored; unit + integration + doc).
See `docs/TEST_COVERAGE.md` for the full per-file breakdown.

**Testing conventions** — see the Testing Conventions section below before writing any tests.
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ let r2 = pq.execute(&[("tx", BindValue::TxCount(2)), ("entity", BindValue::Entit

```bash
cargo run # interactive Datalog REPL
cargo test # run 1311 tests
cargo test # run 1313 tests
cargo run < demos/demo_recursive.txt # recursive rules demo
```

Expand Down
22 changes: 22 additions & 0 deletions docs/ERROR_REFERENCE.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ with no `CodedError` anywhere in its chain.
| PRS-077 | Unexpected trailing input after a complete form | Parser |
| PRS-078 | Query rejects unexpected trailing argument(s) | Parser |
| PRS-079 | Rule rejects unexpected trailing argument(s) | Parser |
| PRS-108 | Fact has more than 4 elements | Parser |
| QRY-001 | Invalid entity | Query Execution |
| QRY-002 | Attribute must be a keyword | Query Execution |
| QRY-003 | Cannot transact a pseudo-attribute | Query Execution |
Expand Down Expand Up @@ -1528,6 +1529,27 @@ $<4097-char-slot-name>
```
*Nothing may follow the rule vector; remove `:unexpected-extra`*

### PRS-108 Fact has more than 4 elements

**Error text**: `Fact must have at most 4 elements (E A V and an optional map), got {}`

**Cause**: A fact vector in `transact` has more than four elements. A fact is
`[E A V]` or `[E A V {:valid-from ... :valid-to ...}]`. Through v2.0.8, a
fact whose 4th element was a map had every element after the map dropped
without an error (#649), and one whose 4th element was not a map was rejected
with `PRS-047`. Added in v2.0.9, numbered as in v3.0.0.

**Resolution**:
- Check the brackets: two facts run together,
`(transact [[e a v {} e2 a2 v2]])`, are two vectors in the facts vector,
`(transact [[e a v {}] [e2 a2 v2]])`.

**Example**:
```datalog
(transact [[:alice :person/age 1 {} 99 :junk]])
```
8000 *Remove `99 :junk`, or make it its own fact*

---

## QRY — Query Execution Errors
Expand Down
4 changes: 3 additions & 1 deletion docs/TEST_COVERAGE.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

**Verified**: 2026-10-20 with `cargo test`

**Result**: 1,302 passing tests and 9 ignored tests (1,311 total), including the golden-file corpus (#391)
**Result**: 1,304 passing tests and 9 ignored tests (1,313 total), including the golden-file corpus (#391)

For v2.0.5, `tests/edge_cases_test.rs` checks that a file of 1 to 4,095 bytes is `STG-045`, unchanged and with no WAL, when it is a database cut below one page or foreign bytes, and opens as a new database that takes a write and reads it back after reopen when it is a prefix of the initial header or all zeros (#506). Rule-body tests in `tests/recursive_rules_test.rs` check that a `:db/valid-at` pattern in a rule definition, in its `not` and `not-join` bodies and in its `or` branches binds the query's valid time, on the semi-naive, stratified and magic-sets paths (#617).

Expand All @@ -20,6 +20,8 @@ For v2.0.5, torn-WAL-tail tests (#513) check that opening a WAL writer cuts off

`tests/disjunction_test.rs` checks that an `or` sharing no variable with the earlier clauses joins as a cross product (#610), before or after them, through the seeded path (a branch with `not`) and in a rule body, keeps the earlier clauses' variables and fact metadata (a pseudo-attribute), and gives no rows when no branch matches. It also checks that `or` / `or-join` branches whose expressions read or bind from a variable only the outer row binds match, nested and in rule bodies (#620); a unit test covers `branch_outer_reads`. Expressions reading a variable only an `or` binds, including a chain and a `not` after them, keep their rows in queries, with rules and in rule bodies (#625); optimizer unit tests cover deferring them ahead of the deferred `not`s. An `or` / `or-join` branch meets a keyword value as the entity it names and the reverse, in a query and in a rule body (#637). Tests in `tests/not_join_test.rs` check that a rule calling a rule with `not-join`, `not` or `or` returns that rule's rows, with a free and a literal argument and through a forwarding rule, and that a rule with its own `not-join` calling one gets the same rows whether or not the query asks for the callee first; stratification unit tests check that such a caller, a rule forwarding to it and a caller of a recursive one sit above the callee, and that recursion through a rule with a `not` still stratifies (#628). Tests in `tests/not_join_test.rs` check that recursion through a rule with `not`, `not-join` or `or`, directly or through another rule, returns every row along a chain (the `or` binding the next step), and that the `not` still removes the rows it should; evaluator unit tests check that such recursion stops at its fixpoint, that `max_iterations` and `max_results` end one that does not with INT-020, and that `max_results` counts the facts every pass derives, not only the mixed rule's (#633).

`tests/error_codes_prs_test.rs` checks that a fact of five or six elements, with or without a map 4th, fails with `PRS-108`, and that a transaction holding one stores none of its facts (#649).

Not counted above: the `BrowserDb` tests run only on wasm32 with `wasm-bindgen-test` in a browser. Two of them check that the IndexedDB flush returns every page a write dirtied, and that a dirty page that cannot be read is an error rather than left out.

The v2.0.8 suite covers the public database API, the Datalog engine, storage and recovery paths, and the v2.0.0 locking and structured-error-code changes, plus v2.0.1's regression test for rebuilding indexes on open after several checkpoints (#370). Since v2.0.1, `tests/point_query_history_test.rs` and new storage unit tests check that attribute-narrowed bound-entity lookups return exactly what a full scan returns, and that `net_asserted_facts` matches its previous implementation on randomized histories (#323). Attribute-scan tests check that the AEVT range covers exactly the queried attribute, including non-ASCII names such as `:丿` and prefix siblings such as `:ab` for `:a` (#381). Incremental B+tree rebuild tests check that checkpoints which copy untouched index leaves produce exactly the entries and file checksum a full rebuild would, including over repeated checkpoints, that single-fact checkpoints do not fragment the index, and that a cyclic leaf chain or damaged slot directory is rejected rather than copied (#315). Directory-sync unit tests check that creating the `.graph` file or the WAL, and deleting the WAL, fsync the parent directory after the file operation, and that reopening an existing file does not (#389). Disjunction tests check that `or-join` returns an empty result, not INT-031, when no rows reach it: on an empty database, after a pattern that matches nothing, after a `not` that removes every row, and in a rule body (#405). A stratification unit test builds the same two-rule negative cycle 64 times and checks that the INT-054 message names the same predicate pair every time (#410). Magic-sets tests check that a recursive rule with a literal start that recurses through another rule returns the same results as without the literal (#297). API tests check that `execute()` and `WriteTransaction::execute()` reject `$slot` bind tokens in every position with API-010 (#407). The REPL `run()` test now runs in a child process with null stdin, so it no longer hangs when the test runner's stdin is an open non-TTY stream (#415). A fault-injection test crashes `save()` at each of its 47 page writes and syncs and checks that every reopen finds exactly the old or the new checkpointed facts (#374). For v2.0.3, storage unit tests and two `edge_cases_test.rs` tests check that more than 65,535 uncheckpointed facts each resolve to their own fact through entity lookups and attribute scans, in memory and file-backed, before and after a checkpoint, and that the widened `FactRef` encodes to the same bytes as before (#445). A WAL test reopens a database next to a WAL whose entries are all checkpointed, writes again, and checks that `:as-of` history is unchanged before and after another reopen; a unit test checks that restoring the transaction counter never lowers it (#447). Since v2.0.3, `tests/golden_corpus_test.rs` opens a copy of each committed golden file in `tests/golden/` (v7 files written by minigraf 2.0.3: one checkpoint, several checkpoints, indexes rebuilt on open, a crash-left WAL, an already-checkpointed WAL, same-transaction multi-values) and checks its manifest's CRC32, `tx_count` and query results after open, after a checkpoint and reopen, and after one more write; a second test checks that every golden file has a manifest (#391). A recursive-rules test runs rule queries on a database with more live facts than `max_results` and checks that they answer, and that the limit still stops a rule that derives more facts than it allows (#528). A WAL test restores a WAL whose entries are all checkpointed and checks that an explicit `checkpoint()` removes it, keeps the fact, and that the next write does not reuse tx 1; the golden-corpus test now requires `v7_stale_wal` to have no WAL after a checkpoint too (#457). A WAL test checks that dropping a handle opened with `wal_checkpoint_threshold(usize::MAX)` leaves the `.graph` file byte for byte unchanged under `Full` and `Normal` sync, keeps the WAL, and that a reopen replays it (#476).
Expand Down
9 changes: 9 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ pub(crate) enum ErrorCode {
Prs077,
Prs078,
Prs079,
Prs108,
Qry001,
Qry002,
Qry003,
Expand Down Expand Up @@ -717,6 +718,12 @@ pub(crate) const REGISTRY: &[(ErrorCode, &str, &str, ErrorCategory)] = &[
"rule takes (rule [...]); found {} unexpected trailing argument(s)",
ErrorCategory::Parser,
),
(
ErrorCode::Prs108,
"PRS-108",
"Fact must have at most 4 elements (E A V and an optional map), got {}",
ErrorCategory::Parser,
),
(
ErrorCode::Qry001,
"QRY-001",
Expand Down Expand Up @@ -1729,6 +1736,7 @@ mod tests {
| ErrorCode::Prs077
| ErrorCode::Prs078
| ErrorCode::Prs079
| ErrorCode::Prs108
| ErrorCode::Qry001
| ErrorCode::Qry002
| ErrorCode::Qry003
Expand Down Expand Up @@ -1922,6 +1930,7 @@ mod tests {
ErrorCode::Prs077,
ErrorCode::Prs078,
ErrorCode::Prs079,
ErrorCode::Prs108,
ErrorCode::Qry001,
ErrorCode::Qry002,
ErrorCode::Qry003,
Expand Down
3 changes: 3 additions & 0 deletions src/query/datalog/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -996,6 +996,9 @@ fn parse_transact(elements: &[EdnValue]) -> Result<DatalogCommand> {
if fact_vec.len() < 3 {
bail_coded!(ErrorCode::Prs046, fact_vec.len());
}
if fact_vec.len() > 4 {
bail_coded!(ErrorCode::Prs108, fact_vec.len());
}

// SAFETY: len >= 3 check above guarantees indices 0, 1, 2 exist
#[allow(clippy::indexing_slicing)]
Expand Down
48 changes: 48 additions & 0 deletions tests/error_codes_prs_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -421,3 +421,51 @@ fn prepare_rejects_transact_command_not_prs() {
// Transact command, it's just the wrong command kind for prepare().
assert_ne!(err.category(), ErrorCategory::Parser);
}

// ── PRS-108: a fact with more than four elements (#649) ─────────────────────

/// Execute `input` and assert it fails with `expected_code` and a message
/// containing `needle`.
fn assert_code_and_message(input: &str, expected_code: &str, needle: &str) {
assert_execute_code(input, expected_code);
let err = db().execute(input).expect_err("input must be rejected");
assert!(
err.to_string().contains(needle),
"message for {} lacks {}",
expected_code,
needle
);
}

#[test]
fn prs_108_fact_with_more_than_four_elements() {
assert_code_and_message(
"(transact [[:alice :person/age 1 {} 99 :junk]])",
"PRS-108",
"got 6",
);
assert_code_and_message(
r#"(transact [[:alice :name "Alice" "bogus" 7]])"#,
"PRS-108",
"got 5",
);
}

#[test]
fn prs_108_rejects_the_whole_transaction() {
// Before #649 the elements after the map were dropped and the fact stored.
let db = db();
db.execute(r#"(transact [[:bob :person/age 2] [:alice :person/age 1 {} 99 :junk]])"#)
.expect_err("a 6-element fact must be rejected");
let result = db
.execute("(query [:find ?e ?v :where [?e :person/age ?v]])")
.expect("query must run");
let minigraf::QueryResult::QueryResults { results, .. } = result else {
panic!("expected query results");
};
assert_eq!(
results.len(),
0,
"no fact of the rejected transaction is stored"
);
}
Loading
0