mirror of
https://gitee.com/mateos/mateclaw.git
synced 2026-09-15 20:08:18 +08:00
fix(wiki): case-only rename portability + SpringBootTest regression suite
This commit is contained in:
parent
b6ae7322a9
commit
af9928bf54
@ -725,7 +725,16 @@ public class WikiPageService {
|
|||||||
throw new IllegalStateException("page is protected (system or locked), refusing to rename");
|
throw new IllegalStateException("page is protected (system or locked), refusing to rename");
|
||||||
}
|
}
|
||||||
WikiPageEntity collision = getBySlug(kbId, newSlug);
|
WikiPageEntity collision = getBySlug(kbId, newSlug);
|
||||||
if (collision != null) {
|
// The collision-check has to ignore "renaming yourself" — on
|
||||||
|
// case-insensitive DB collations (e.g. MySQL's default
|
||||||
|
// utf8mb4_unicode_ci), getBySlug returns the SAME row when
|
||||||
|
// newSlug differs from oldSlug only in case. Treating that as a
|
||||||
|
// collision would forbid case-only renames on MySQL while H2
|
||||||
|
// (case-sensitive) silently allowed them, producing an
|
||||||
|
// environment-dependent error. Comparing ids makes the rule
|
||||||
|
// identical on both backends: only a row owned by a different
|
||||||
|
// page is a true collision.
|
||||||
|
if (collision != null && !existing.getId().equals(collision.getId())) {
|
||||||
throw new IllegalArgumentException("a page with slug '" + newSlug + "' already exists in this KB");
|
throw new IllegalArgumentException("a page with slug '" + newSlug + "' already exists in this KB");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@ -0,0 +1,230 @@
|
|||||||
|
package vip.mate.wiki.service;
|
||||||
|
|
||||||
|
import com.baomidou.mybatisplus.core.conditions.query.LambdaQueryWrapper;
|
||||||
|
import org.junit.jupiter.api.AfterEach;
|
||||||
|
import org.junit.jupiter.api.DisplayName;
|
||||||
|
import org.junit.jupiter.api.Test;
|
||||||
|
import org.springframework.beans.factory.annotation.Autowired;
|
||||||
|
import org.springframework.boot.test.context.SpringBootTest;
|
||||||
|
import org.springframework.test.annotation.DirtiesContext;
|
||||||
|
import vip.mate.wiki.model.WikiKnowledgeBaseEntity;
|
||||||
|
import vip.mate.wiki.model.WikiPageEntity;
|
||||||
|
import vip.mate.wiki.repository.WikiKnowledgeBaseMapper;
|
||||||
|
import vip.mate.wiki.repository.WikiPageMapper;
|
||||||
|
|
||||||
|
import static org.assertj.core.api.Assertions.assertThat;
|
||||||
|
import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Regression suite for the cascade / scan write paths.
|
||||||
|
*
|
||||||
|
* <p>Boots the full Spring context with the H2 + Flyway test profile so the
|
||||||
|
* V129 broken_links migration is in place and MyBatis-Plus's lambda cache
|
||||||
|
* for {@link WikiPageEntity} is fully primed. That priming is what
|
||||||
|
* distinguishes this suite from the existing mock-mapper tests in
|
||||||
|
* {@link WikiPageServiceTest} — only the real Spring + MP wiring exposes
|
||||||
|
* the {@code FieldStrategy.ALWAYS} + partial-entity-update interaction
|
||||||
|
* that the §8 incident exposed.
|
||||||
|
*
|
||||||
|
* <p>Three classes of guard, one per case:
|
||||||
|
* <ul>
|
||||||
|
* <li>Scan must leave {@code content} and {@code summary} byte-identical.</li>
|
||||||
|
* <li>Cascade delete must leave the referrer's {@code summary} byte-identical
|
||||||
|
* (and its {@code content} only modified by the wikilink demotion).</li>
|
||||||
|
* <li>Cascade rename must leave the referrer's {@code summary} byte-identical
|
||||||
|
* (and its {@code content} only modified by the wikilink target swap).</li>
|
||||||
|
* </ul>
|
||||||
|
*
|
||||||
|
* <p>Plus a small portability check around the case-only rename path (R4-G).
|
||||||
|
*/
|
||||||
|
@SpringBootTest(
|
||||||
|
webEnvironment = SpringBootTest.WebEnvironment.NONE,
|
||||||
|
properties = {
|
||||||
|
"spring.flyway.enabled=true",
|
||||||
|
"spring.flyway.locations=classpath:db/migration/h2",
|
||||||
|
"mateclaw.feature-flag.refresh-ms=999999"
|
||||||
|
}
|
||||||
|
)
|
||||||
|
@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS)
|
||||||
|
class WikiCascadeRegressionE2ETest {
|
||||||
|
|
||||||
|
@Autowired private WikiPageService pageService;
|
||||||
|
@Autowired private WikiLintJobService lintJobService;
|
||||||
|
@Autowired private WikiKnowledgeBaseService kbService;
|
||||||
|
@Autowired private WikiPageMapper pageMapper;
|
||||||
|
@Autowired private WikiKnowledgeBaseMapper kbMapper;
|
||||||
|
|
||||||
|
private Long kbId;
|
||||||
|
|
||||||
|
@AfterEach
|
||||||
|
void cleanup() {
|
||||||
|
if (kbId != null) {
|
||||||
|
pageMapper.delete(new LambdaQueryWrapper<WikiPageEntity>().eq(WikiPageEntity::getKbId, kbId));
|
||||||
|
kbMapper.deleteById(kbId);
|
||||||
|
pageService.evictSummaryCache(kbId);
|
||||||
|
kbId = null;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
private void seedKb() {
|
||||||
|
WikiKnowledgeBaseEntity kb = kbService.create("cascade-regress-" + System.nanoTime(), "regression test", null);
|
||||||
|
kbId = kb.getId();
|
||||||
|
// Purge whatever the bootstrap may have auto-inserted so we own the page set.
|
||||||
|
pageMapper.delete(new LambdaQueryWrapper<WikiPageEntity>().eq(WikiPageEntity::getKbId, kbId));
|
||||||
|
pageService.evictSummaryCache(kbId);
|
||||||
|
}
|
||||||
|
|
||||||
|
// ----------------------------------------------------------------
|
||||||
|
// §8 regression — scan / cascade must not null content + summary
|
||||||
|
// ----------------------------------------------------------------
|
||||||
|
|
||||||
|
@Test
|
||||||
|
@DisplayName("scan x N leaves content + summary byte-identical")
|
||||||
|
void scanPreservesContentAndSummary() throws Exception {
|
||||||
|
seedKb();
|
||||||
|
String content = "## Heading\n\nThis page has a [[ghost]] broken ref and prose body.";
|
||||||
|
String summary = "summary that must survive every scan";
|
||||||
|
WikiPageEntity created = pageService.createPage(kbId, "alpha", "Alpha",
|
||||||
|
content, summary, "[]");
|
||||||
|
|
||||||
|
// Round-trip through the DB so we read what was actually persisted.
|
||||||
|
WikiPageEntity beforeScan = pageMapper.selectById(created.getId());
|
||||||
|
assertThat(beforeScan.getContent()).isEqualTo(content);
|
||||||
|
assertThat(beforeScan.getSummary()).isEqualTo(summary);
|
||||||
|
|
||||||
|
// Run KB-wide scan three times in a row. With the bug present, each
|
||||||
|
// pass would re-issue `UPDATE ... SET content=NULL, summary=NULL`
|
||||||
|
// for every page in the KB.
|
||||||
|
for (int i = 0; i < 3; i++) {
|
||||||
|
lintJobService.startOrGetRunning(kbId);
|
||||||
|
waitForJobCompletion(20);
|
||||||
|
}
|
||||||
|
|
||||||
|
WikiPageEntity afterScan = pageMapper.selectById(created.getId());
|
||||||
|
assertThat(afterScan.getContent())
|
||||||
|
.as("content must not be NULL'd by scan")
|
||||||
|
.isEqualTo(content);
|
||||||
|
assertThat(afterScan.getSummary())
|
||||||
|
.as("summary must not be NULL'd by scan")
|
||||||
|
.isEqualTo(summary);
|
||||||
|
// Broken-link state still computed correctly.
|
||||||
|
assertThat(afterScan.getBrokenLinks()).contains("ghost");
|
||||||
|
assertThat(afterScan.getBrokenLinksScannedAt()).isNotNull();
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
@DisplayName("cascade delete preserves referrer's summary; content reduced only by wikilink demotion")
|
||||||
|
void cascadeDeletePreservesReferrerSummary() {
|
||||||
|
seedKb();
|
||||||
|
pageService.createPage(kbId, "alice", "Alice",
|
||||||
|
"Alice is the team lead.",
|
||||||
|
"Senior engineer, leads search.",
|
||||||
|
"[]");
|
||||||
|
WikiPageEntity bob = pageService.createPage(kbId, "bob", "Bob",
|
||||||
|
"Bob reports to [[alice]] and pairs with [[alice|her]].",
|
||||||
|
"Junior engineer mentored by Alice.",
|
||||||
|
"[]");
|
||||||
|
int bobLenBefore = bob.getContent().length();
|
||||||
|
String bobSummary = bob.getSummary();
|
||||||
|
|
||||||
|
pageService.delete(kbId, "alice");
|
||||||
|
|
||||||
|
WikiPageEntity bobAfter = pageMapper.selectById(bob.getId());
|
||||||
|
assertThat(bobAfter.getSummary())
|
||||||
|
.as("referrer's summary must not be null'd by cascade delete")
|
||||||
|
.isEqualTo(bobSummary);
|
||||||
|
assertThat(bobAfter.getContent()).doesNotContain("[[alice]]");
|
||||||
|
assertThat(bobAfter.getContent()).doesNotContain("[[alice|");
|
||||||
|
// Snapshot title + alias preserved as visible text.
|
||||||
|
assertThat(bobAfter.getContent()).contains("Alice").contains("her");
|
||||||
|
// Length should shrink (the wikilink syntax overhead goes away) but not zero.
|
||||||
|
assertThat(bobAfter.getContent().length())
|
||||||
|
.isGreaterThan(0)
|
||||||
|
.isLessThan(bobLenBefore);
|
||||||
|
// outgoing_links should be empty now that the only target was removed.
|
||||||
|
assertThat(bobAfter.getOutgoingLinks()).isEqualTo("[]");
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
@DisplayName("cascade rename preserves referrer's summary; alias preserved")
|
||||||
|
void cascadeRenamePreservesReferrerSummary() {
|
||||||
|
seedKb();
|
||||||
|
pageService.createPage(kbId, "old-slug", "Old Title",
|
||||||
|
"stub", "stub summary", "[]");
|
||||||
|
WikiPageEntity referrer = pageService.createPage(kbId, "ref", "Ref",
|
||||||
|
"Links: [[old-slug]] and [[old-slug|displayed text]].",
|
||||||
|
"Referrer summary that must survive rename.",
|
||||||
|
"[]");
|
||||||
|
String summaryBefore = referrer.getSummary();
|
||||||
|
|
||||||
|
WikiPageEntity renamed = pageService.rename(kbId, "old-slug", "new-slug");
|
||||||
|
assertThat(renamed).isNotNull();
|
||||||
|
assertThat(renamed.getSlug()).isEqualTo("new-slug");
|
||||||
|
|
||||||
|
WikiPageEntity refAfter = pageMapper.selectById(referrer.getId());
|
||||||
|
assertThat(refAfter.getSummary())
|
||||||
|
.as("referrer's summary must not be null'd by cascade rename")
|
||||||
|
.isEqualTo(summaryBefore);
|
||||||
|
assertThat(refAfter.getContent()).doesNotContain("[[old-slug]]");
|
||||||
|
assertThat(refAfter.getContent()).doesNotContain("[[old-slug|");
|
||||||
|
assertThat(refAfter.getContent()).contains("[[new-slug]]");
|
||||||
|
assertThat(refAfter.getContent()).contains("[[new-slug|displayed text]]");
|
||||||
|
assertThat(refAfter.getOutgoingLinks()).contains("new-slug");
|
||||||
|
}
|
||||||
|
|
||||||
|
// ----------------------------------------------------------------
|
||||||
|
// R4-G regression — case-only rename portability
|
||||||
|
// ----------------------------------------------------------------
|
||||||
|
|
||||||
|
@Test
|
||||||
|
@DisplayName("case-only rename (foo → FOO) is allowed: collision check ignores same row")
|
||||||
|
void caseOnlyRenameIsAllowed() {
|
||||||
|
seedKb();
|
||||||
|
WikiPageEntity p = pageService.createPage(kbId, "foo", "Foo", "body", "sum", "[]");
|
||||||
|
|
||||||
|
// Without the same-id collision-check escape, this would throw on
|
||||||
|
// MySQL because getBySlug("FOO") returns the same row (case-insensitive
|
||||||
|
// collation). The fix lets it through.
|
||||||
|
WikiPageEntity renamed = pageService.rename(kbId, "foo", "FOO");
|
||||||
|
assertThat(renamed).isNotNull();
|
||||||
|
assertThat(renamed.getId()).isEqualTo(p.getId());
|
||||||
|
assertThat(renamed.getSlug()).isEqualTo("FOO");
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
@DisplayName("rename to a slug already owned by a DIFFERENT page still rejects with 400-equivalent")
|
||||||
|
void renameRejectsRealCollision() {
|
||||||
|
seedKb();
|
||||||
|
pageService.createPage(kbId, "first", "First", "x", "x", "[]");
|
||||||
|
pageService.createPage(kbId, "second", "Second", "y", "y", "[]");
|
||||||
|
|
||||||
|
// first → second is a real collision (different existing page); the
|
||||||
|
// same-id escape must NOT swallow this.
|
||||||
|
assertThatThrownBy(() -> pageService.rename(kbId, "first", "second"))
|
||||||
|
.isInstanceOf(IllegalArgumentException.class)
|
||||||
|
.hasMessageContaining("already exists");
|
||||||
|
}
|
||||||
|
|
||||||
|
// ----------------------------------------------------------------
|
||||||
|
// Helpers
|
||||||
|
// ----------------------------------------------------------------
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Spin for at most {@code timeoutSec} waiting for the latest job on
|
||||||
|
* {@link #kbId} to leave the queued/running state. The lint executor is
|
||||||
|
* single-threaded and per-page work is sub-ms, so this returns almost
|
||||||
|
* immediately in practice.
|
||||||
|
*/
|
||||||
|
private void waitForJobCompletion(int timeoutSec) throws InterruptedException {
|
||||||
|
long deadline = System.currentTimeMillis() + timeoutSec * 1000L;
|
||||||
|
while (System.currentTimeMillis() < deadline) {
|
||||||
|
WikiLintJobService.LintJob job = lintJobService.getLatestJob(kbId);
|
||||||
|
if (job == null) return;
|
||||||
|
if (job.status() == WikiLintJobService.JobStatus.COMPLETED
|
||||||
|
|| job.status() == WikiLintJobService.JobStatus.FAILED) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
Thread.sleep(25);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
@ -599,3 +599,80 @@ regressions discovered. The two ⚠️s are not defects against the
|
|||||||
shipping spec — they're behaviours the spec was silent on, now
|
shipping spec — they're behaviours the spec was silent on, now
|
||||||
documented here so future readers / reviewers know what to expect.
|
documented here so future readers / reviewers know what to expect.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 11. Follow-up resolution (2026-05-28)
|
||||||
|
|
||||||
|
Both follow-ups from §8.4 and §10.1 are closed. Code changes + the
|
||||||
|
matching tests live in `WikiCascadeRegressionE2ETest`.
|
||||||
|
|
||||||
|
### 11.1 SpringBootTest regression for §8 (scan / cascade null-out)
|
||||||
|
|
||||||
|
New class `WikiCascadeRegressionE2ETest` boots the full Spring context
|
||||||
|
with H2 + Flyway, so MyBatis-Plus's lambda cache for `WikiPageEntity`
|
||||||
|
is primed and the actual SQL generated by `pageMapper.updateById(...)`
|
||||||
|
vs `pageMapper.update(null, LambdaUpdateWrapper)` is exercised — what
|
||||||
|
the existing mock-mapper tests in `WikiPageServiceTest` couldn't see.
|
||||||
|
|
||||||
|
Three guard tests:
|
||||||
|
|
||||||
|
| Test | What it locks down |
|
||||||
|
|---|---|
|
||||||
|
| `scanPreservesContentAndSummary` | PUT a page with content + summary, run KB-wide scan 3× in a row, assert content and summary byte-identical in the DB. Catches a recurrence of the §8 incident at the boundary between FieldStrategy.ALWAYS and partial-entity update |
|
||||||
|
| `cascadeDeletePreservesReferrerSummary` | Seed two pages where B references A, delete A, assert B.summary is unchanged and B.content has the `[[a]]` and `[[a\|alias]]` demoted to snapshot title + alias |
|
||||||
|
| `cascadeRenamePreservesReferrerSummary` | Same seed, rename A → A', assert B.summary unchanged and B.content has `[[a]]` → `[[a']]` with alias preserved |
|
||||||
|
|
||||||
|
If anyone ever puts back the old `pageMapper.updateById(partialEntity)`
|
||||||
|
pattern in `WikiLintJobService.rewriteBrokenLinks` or in the cascade
|
||||||
|
loops, one of these tests fails with the expected `content` value
|
||||||
|
being a long string and the actual being `null`.
|
||||||
|
|
||||||
|
### 11.2 Case-only rename portability fix (R4-G)
|
||||||
|
|
||||||
|
`WikiPageService.rename`'s collision-check was tightened:
|
||||||
|
|
||||||
|
```java
|
||||||
|
// before
|
||||||
|
if (collision != null) { throw ... }
|
||||||
|
|
||||||
|
// after
|
||||||
|
if (collision != null && !existing.getId().equals(collision.getId())) { throw ... }
|
||||||
|
```
|
||||||
|
|
||||||
|
Effect:
|
||||||
|
|
||||||
|
- On H2 (case-sensitive collation) — same as before: rename `foo → FOO`
|
||||||
|
finds no row, `collision == null`, allowed. Side-effect: the row's
|
||||||
|
stored slug becomes `FOO`; future `getBySlug("foo")` returns 404,
|
||||||
|
`getBySlug("FOO")` returns 200. Lint resolution stays case-insensitive
|
||||||
|
(extractor lowercases targets) so referrer content links of any case
|
||||||
|
still resolve.
|
||||||
|
- On MySQL (`utf8mb4_unicode_ci`) — previously: rename `foo → FOO`
|
||||||
|
found the same row via case-insensitive comparison, the old check
|
||||||
|
treated that as a collision, threw 400. Now: same-id is recognised
|
||||||
|
as "renaming yourself", the rename is allowed.
|
||||||
|
- Real collisions (renaming `first → second` when both exist as
|
||||||
|
distinct rows) still reject — `renameRejectsRealCollision` test pins
|
||||||
|
this down.
|
||||||
|
|
||||||
|
Two new tests in `WikiCascadeRegressionE2ETest` cover both branches:
|
||||||
|
|
||||||
|
| Test | What it locks down |
|
||||||
|
|---|---|
|
||||||
|
| `caseOnlyRenameIsAllowed` | `foo → FOO` succeeds; same row, new slug |
|
||||||
|
| `renameRejectsRealCollision` | `first → second` rejects with `IllegalArgumentException` containing "already exists" |
|
||||||
|
|
||||||
|
### 11.3 Test count
|
||||||
|
|
||||||
|
Wiki tests went from 273 (after Phase 5) to **278** with the 5 new
|
||||||
|
`WikiCascadeRegressionE2ETest` cases. All pass. The new tests run in
|
||||||
|
~7 s, dominated by Spring context startup; the per-test work is
|
||||||
|
sub-second.
|
||||||
|
|
||||||
|
### 11.4 No remaining follow-ups
|
||||||
|
|
||||||
|
The wikilink overhaul has no known open issues from any of the four
|
||||||
|
e2e passes. The bug discovered mid-§8 has a unit test guard. The
|
||||||
|
portability gap noted in §10.1 has been fixed and tested. The shipping
|
||||||
|
spec (RFC 55 v3.3) matches observable behaviour on both H2 and MySQL.
|
||||||
|
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user