diff --git a/PLAN.md b/PLAN.md index 5c13dd8..a9a3feb 100644 --- a/PLAN.md +++ b/PLAN.md @@ -1123,6 +1123,32 @@ has to be its own commit with its own re-recorded scorecard. agrees. Worth one commit, and it needs its own measurements first: the padding case (`y.3.b` past the end) is a *legal* creation on mongod, so the fix is not "refuse a non-document element". +- **`partialFilterExpression` was accepted and ignored**, and that made + `unique` mean the wrong thing. An index built over every document instead of + the filtered subset still answers reads correctly -- it holds a superset, + never a subset -- but a *unique* one then rejects inserts mongod accepts: + + ```js + createIndex({a: 1}, {unique: true, partialFilterExpression: {t: true}}) + insertMany([{a: 1, t: false}, {a: 1, t: false}]) + // mongod: accepted, neither document is in the index + // ours: E11000 duplicate key error + ``` + + Unique-within-a-subset is the whole point of the option, so every use of it + was a legal insert refused. Refused at creation now, on the same judgement + `cmd_update` already makes about an update spec's `sort`. Nothing in the + repository covered this row: the pinned suite is crud and aggregate, and + neither `e2e5.js` nor `e2e6.js` writes a partial or hashed spec. Hashed, by + contrast, was honestly missing -- refused, with the wrong code (2 where + mongod says 67) but the right answer. + + `docs/M3_INDEX_TYPES_DESIGN_REVIEW.md` measures both features' rules and + argues the planner rule that decides the design: a partial index may only + answer a query whose predicates *imply* its filter, so until that test + exists the safe rule is to maintain the index and never read from it. Too + few documents is the one failure worse than no index at all. + - **M3's second corpus is `tests/spec/operators/`.** The eight operators PLAN §3 names all answered `bad update` with code 2, one message for every question — and `$push`'s `$slice`, `$position` and `$sort` were parsed, diff --git a/src/commands.zig b/src/commands.zig index da6f72b..a0e304b 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -934,6 +934,35 @@ fn cmd_create_indexes(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !vo if (name == .string and std.mem.eql(u8, name.string, "_id_")) { return bad_value(reply, "cannot create index with name '_id_'"); } + // Accepted and ignored until this commit, which is the worst of the + // three possible answers -- the same judgement `cmd_update` already + // makes about an update spec's `sort`. + // + // An index built over every document instead of the filtered subset + // still answers reads correctly: it holds a superset, never a subset. + // `unique` is where that stops being true. Measured on mongod 8.3.7: + // + // createIndex({a: 1}, {unique: true, partialFilterExpression: {t: true}}) + // insertMany([{a: 1, t: false}, {a: 1, t: false}]) + // + // mongod accepts both -- neither document is in the index, so neither + // collides -- and this server answered E11000. Unique-within-a-subset + // is the whole point of the option, so every use of it was a legal + // insert refused. The other two answers available were to keep doing + // that, or to echo the option back from `listIndexes` while not + // honouring it, which is a larger lie than saying no. + // + // See docs/M3_INDEX_TYPES_DESIGN_REVIEW.md. The implementation is the + // rest of M3's last row; this is what stands in until then. + if (bson.get_pair(spec, "partialFilterExpression") != null) { + return reply.put_error( + @intFromEnum(ErrorCode.cannot_create_index), + "CannotCreateIndex", + "partialFilterExpression is not implemented by this server: it would be " ++ + "accepted and ignored, and a unique index would then be enforced over " ++ + "documents the filter excludes", + ); + } const spec_doc = bson.Document{ .arena = undefined, .pairs = spec }; _ = ctx.engine.create_index(db_name, coll_name, &spec_doc) catch |err| switch (err) { @@ -5655,6 +5684,23 @@ test "TTL index round-trips through createIndexes/listIndexes; bad specs give 67 .{ .key = "key", .value = .{ .doc = &.{.{ .key = "_id", .value = .{ .int32 = 1 } }} } }, .{ .key = "expireAfterSeconds", .value = .{ .int32 = 60 } }, } } }, + // Same rule, different option: a partial filter accepted and ignored + // would enforce `unique` over documents the filter excludes. + .{ .code = 67, .spec = .{ .doc = &.{ + .{ .key = "key", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } }, + .{ .key = "partialFilterExpression", .value = .{ .doc = &.{ + .{ .key = "t", .value = .{ .bool = true } }, + } } }, + } } }, + // And with `unique`, which is the combination that made it a wrong + // answer rather than only a missing one. + .{ .code = 67, .spec = .{ .doc = &.{ + .{ .key = "key", .value = .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} } }, + .{ .key = "unique", .value = .{ .bool = true } }, + .{ .key = "partialFilterExpression", .value = .{ .doc = &.{ + .{ .key = "t", .value = .{ .bool = true } }, + } } }, + } } }, }; for (bad) |case| { var ctx = tdb.ctx(io); @@ -5668,6 +5714,10 @@ test "TTL index round-trips through createIndexes/listIndexes; bad specs give 67 try dispatch(&ctx, &msg, &reply); try testing.expectEqual(case.code, bson.get_pair(reply.pairs.items, "code").?.int32); } + // Mutation check for the two `partialFilterExpression` rows: delete the + // guard in `cmd_create_indexes` and both go green on the code -- and the + // second one's index then refuses `{a: 1, t: false}` twice, which mongod + // accepts because neither document is in the index at all. // Nothing partial was registered by the rejected specs. try testing.expectEqual(@as(usize, 1), tdb.engine.get_collection("test", "sessions").?.indexes.items.len); }