|
|
@@ -0,0 +1,302 @@
|
|
|
+---
|
|
|
+name: page-save-origin-semantics
|
|
|
+description: Auto-invoked when modifying origin-based conflict detection, revision validation logic, or isUpdatable() method. Explains the two-stage origin check mechanism for conflict detection and its separation from diff detection.
|
|
|
+---
|
|
|
+
|
|
|
+# Page Save Origin Semantics
|
|
|
+
|
|
|
+## Problem
|
|
|
+
|
|
|
+When modifying page save logic, it's easy to accidentally break the carefully designed origin-based conflict detection system. The system uses a two-stage check mechanism (frontend + backend) to determine when revision validation should be enforced vs. bypassed for collaborative editing (Yjs).
|
|
|
+
|
|
|
+**Key Insight**: **Conflict detection (revision check)** and **diff detection (hasDiffToPrev)** serve different purposes and require separate logic.
|
|
|
+
|
|
|
+## Solution
|
|
|
+
|
|
|
+Understanding the two-stage origin check mechanism:
|
|
|
+
|
|
|
+### Stage 1: Frontend Determines revisionId Requirement
|
|
|
+
|
|
|
+```typescript
|
|
|
+// apps/app/src/client/components/PageEditor/PageEditor.tsx:158
|
|
|
+const isRevisionIdRequiredForPageUpdate = currentPage?.revision?.origin === undefined;
|
|
|
+
|
|
|
+// lines 308-310
|
|
|
+const revisionId = isRevisionIdRequiredForPageUpdate
|
|
|
+ ? currentRevisionId
|
|
|
+ : undefined;
|
|
|
+```
|
|
|
+
|
|
|
+**Logic**: Check the **latest revision's origin** on the page:
|
|
|
+- If `origin === undefined` (legacy/API save) → Send `revisionId`
|
|
|
+- If `origin === "editor"` or `"view"` → Do NOT send `revisionId`
|
|
|
+
|
|
|
+### Stage 2: Backend Determines Conflict Check Behavior
|
|
|
+
|
|
|
+```javascript
|
|
|
+// apps/app/src/server/models/obsolete-page.js:167-172
|
|
|
+const ignoreLatestRevision =
|
|
|
+ origin === Origin.Editor &&
|
|
|
+ (latestRevisionOrigin === Origin.Editor || latestRevisionOrigin === Origin.View);
|
|
|
+
|
|
|
+if (ignoreLatestRevision) {
|
|
|
+ return true; // Bypass revision check
|
|
|
+}
|
|
|
+
|
|
|
+// Otherwise, enforce strict revision matching
|
|
|
+if (revision != previousRevision) {
|
|
|
+ return false; // Reject save
|
|
|
+}
|
|
|
+return true;
|
|
|
+```
|
|
|
+
|
|
|
+**Logic**: Check **current request's origin** AND **latest revision's origin**:
|
|
|
+- If `origin === "editor"` AND latest is `"editor"` or `"view"` → Bypass revision check
|
|
|
+- Otherwise → Enforce strict revision ID matching
|
|
|
+
|
|
|
+## Origin Values
|
|
|
+
|
|
|
+Three types of page update methods (called "origin"):
|
|
|
+
|
|
|
+- **`Origin.Editor = "editor"`** - Save from editor mode (collaborative editing via Yjs)
|
|
|
+- **`Origin.View = "view"`** - Save from view mode
|
|
|
+ - Examples: HandsontableModal, DrawioModal editing
|
|
|
+- **`undefined`** - API-based saves or legacy pages
|
|
|
+
|
|
|
+## Origin Strength (強弱)
|
|
|
+
|
|
|
+**Basic Rule**: Page updates require the previous revision ID in the request. If the latest revision doesn't match, the server rejects the request.
|
|
|
+
|
|
|
+**Exception - Editor origin is stronger than View origin**:
|
|
|
+- **UX Goal**: Avoid `Posted param "revisionId" is outdated` errors when multiple members are using the Editor and View changes interrupt them
|
|
|
+- **Special Case**: When the latest revision's origin is View, Editor origin requests can update WITHOUT requiring revision ID
|
|
|
+
|
|
|
+### Origin Strength Matrix
|
|
|
+
|
|
|
+| | Latest Revision: Editor | Latest Revision: View | Latest Revision: API |
|
|
|
+| ------ | ----------------------- | --------------------- | -------------------- |
|
|
|
+| **Request: Editor** | ⭕️ Bypass revision check | ⭕️ Bypass revision check | ❌ Strict check |
|
|
|
+| **Request: View** | ❌ Strict check | ❌ Strict check | ❌ Strict check |
|
|
|
+| **Request: API** | ❌ Strict check | ❌ Strict check | ❌ Strict check |
|
|
|
+
|
|
|
+**Reading the table**:
|
|
|
+- ⭕️ = Revision check bypassed (revisionId not required)
|
|
|
+- ❌ = Strict revision check required (revisionId must match)
|
|
|
+
|
|
|
+## Behavior by Scenario
|
|
|
+
|
|
|
+| Latest Revision Origin | Request Origin | revisionId Sent? | Revision Check | Use Case |
|
|
|
+|------------------------|----------------|------------------|----------------|----------|
|
|
|
+| `editor` or `view` | `editor` | ❌ No | ✅ Bypassed | Normal Editor use (most common) |
|
|
|
+| `undefined` | `editor` | ✅ Yes | ✅ Enforced | Legacy page in Editor |
|
|
|
+| `undefined` | `undefined` (API) | ✅ Yes (required) | ✅ Enforced | API save |
|
|
|
+
|
|
|
+## Example: Server-Side Logic Respecting Origin Semantics
|
|
|
+
|
|
|
+When adding server-side functionality that needs previous revision data:
|
|
|
+
|
|
|
+```typescript
|
|
|
+// ✅ CORRECT: Separate concerns - conflict detection vs. diff detection
|
|
|
+let previousRevision: IRevisionHasId | null = null;
|
|
|
+
|
|
|
+// Priority 1: Use provided revisionId (for conflict detection)
|
|
|
+if (sanitizeRevisionId != null) {
|
|
|
+ previousRevision = await Revision.findById(sanitizeRevisionId);
|
|
|
+}
|
|
|
+
|
|
|
+// Priority 2: Fallback to currentPage.revision (for other purposes like diff detection)
|
|
|
+if (previousRevision == null && currentPage.revision != null) {
|
|
|
+ previousRevision = await Revision.findById(currentPage.revision);
|
|
|
+}
|
|
|
+
|
|
|
+const previousBody = previousRevision?.body ?? null;
|
|
|
+
|
|
|
+// Continue with existing conflict detection logic (unchanged)
|
|
|
+if (currentPage != null && !(await currentPage.isUpdatable(sanitizeRevisionId, origin))) {
|
|
|
+ // ... return conflict error
|
|
|
+}
|
|
|
+
|
|
|
+// Use previousBody for diff detection or other purposes
|
|
|
+updatedPage = await crowi.pageService.updatePage(
|
|
|
+ currentPage,
|
|
|
+ body,
|
|
|
+ previousBody, // ← Available regardless of conflict detection logic
|
|
|
+ req.user,
|
|
|
+ options,
|
|
|
+);
|
|
|
+```
|
|
|
+
|
|
|
+```typescript
|
|
|
+// ❌ WRONG: Forcing frontend to always send revisionId
|
|
|
+const revisionId = currentRevisionId; // Always send, regardless of origin
|
|
|
+// This breaks Yjs collaborative editing semantics!
|
|
|
+```
|
|
|
+
|
|
|
+```typescript
|
|
|
+// ❌ WRONG: Changing backend conflict detection logic
|
|
|
+// Don't modify isUpdatable() unless you fully understand the implications
|
|
|
+// for collaborative editing
|
|
|
+```
|
|
|
+
|
|
|
+## When to Apply
|
|
|
+
|
|
|
+**Always consider this pattern when**:
|
|
|
+- Modifying page save/update API handlers
|
|
|
+- Adding functionality that needs previous revision data
|
|
|
+- Working on conflict detection or revision validation logic
|
|
|
+- Implementing features that interact with page history
|
|
|
+- Debugging save operation issues
|
|
|
+
|
|
|
+**Key Principles**:
|
|
|
+1. **Do NOT modify frontend revisionId logic** unless explicitly required for conflict detection
|
|
|
+2. **Do NOT modify isUpdatable() logic** unless fixing conflict detection bugs
|
|
|
+3. **Separate concerns**: Conflict detection ≠ Other revision-based features (diff detection, history, etc.)
|
|
|
+4. **Server-side fallback**: If you need previous revision data when revisionId is not provided, fetch from `currentPage.revision`
|
|
|
+
|
|
|
+## Detailed Scenario Analysis
|
|
|
+
|
|
|
+### Scenario A: Normal Editor Mode (Most Common Case)
|
|
|
+
|
|
|
+**Latest revision has `origin=editor`**:
|
|
|
+
|
|
|
+1. **Frontend Logic**:
|
|
|
+ - `isRevisionIdRequiredForPageUpdate = false` (latest revision origin is not undefined)
|
|
|
+ - Does NOT send `revisionId` in request
|
|
|
+ - Sends `origin: Origin.Editor`
|
|
|
+
|
|
|
+2. **API Layer**:
|
|
|
+ ```typescript
|
|
|
+ previousRevision = await Revision.findById(undefined); // → null
|
|
|
+ ```
|
|
|
+ Result: No previousRevision fetched via revisionId
|
|
|
+
|
|
|
+3. **Backend Conflict Check** (`isUpdatable`):
|
|
|
+ ```javascript
|
|
|
+ ignoreLatestRevision =
|
|
|
+ (Origin.Editor === Origin.Editor) &&
|
|
|
+ (latestRevisionOrigin === Origin.Editor || latestRevisionOrigin === Origin.View)
|
|
|
+ // → true (latest revision is editor)
|
|
|
+ return true; // Bypass revision check
|
|
|
+ ```
|
|
|
+ Result: ✅ Save succeeds without revision validation
|
|
|
+
|
|
|
+4. **Impact on Other Features**:
|
|
|
+ - If you need previousRevision data (e.g., for diff detection), it won't be available unless you implement server-side fallback
|
|
|
+ - This is where `currentPage.revision` fallback becomes necessary
|
|
|
+
|
|
|
+### Scenario B: Legacy Page in Editor Mode
|
|
|
+
|
|
|
+**Latest revision has `origin=undefined`**:
|
|
|
+
|
|
|
+1. **Frontend Logic**:
|
|
|
+ - `isRevisionIdRequiredForPageUpdate = true` (latest revision origin is undefined)
|
|
|
+ - Sends `revisionId` in request
|
|
|
+ - Sends `origin: Origin.Editor`
|
|
|
+
|
|
|
+2. **API Layer**:
|
|
|
+ ```typescript
|
|
|
+ previousRevision = await Revision.findById(sanitizeRevisionId); // → revision object
|
|
|
+ ```
|
|
|
+ Result: previousRevision fetched successfully
|
|
|
+
|
|
|
+3. **Backend Conflict Check** (`isUpdatable`):
|
|
|
+ ```javascript
|
|
|
+ ignoreLatestRevision =
|
|
|
+ (Origin.Editor === Origin.Editor) &&
|
|
|
+ (latestRevisionOrigin === undefined)
|
|
|
+ // → false (latest revision is undefined, not editor/view)
|
|
|
+
|
|
|
+ // Strict revision check
|
|
|
+ if (revision != sanitizeRevisionId) {
|
|
|
+ return false; // Reject if mismatch
|
|
|
+ }
|
|
|
+ return true;
|
|
|
+ ```
|
|
|
+ Result: ✅ Save succeeds only if revisionId matches
|
|
|
+
|
|
|
+4. **Impact on Other Features**:
|
|
|
+ - previousRevision data is available
|
|
|
+ - All revision-based features work correctly
|
|
|
+
|
|
|
+### Scenario C: API-Based Save
|
|
|
+
|
|
|
+**Request has `origin=undefined` or omitted**:
|
|
|
+
|
|
|
+1. **Frontend**: Not applicable (API client)
|
|
|
+
|
|
|
+2. **API Layer**:
|
|
|
+ - API client MUST send `revisionId` in request
|
|
|
+ - `previousRevision = await Revision.findById(sanitizeRevisionId)`
|
|
|
+
|
|
|
+3. **Backend Conflict Check** (`isUpdatable`):
|
|
|
+ ```javascript
|
|
|
+ ignoreLatestRevision =
|
|
|
+ (undefined === Origin.Editor) && ...
|
|
|
+ // → false
|
|
|
+
|
|
|
+ // Strict revision check
|
|
|
+ if (revision != sanitizeRevisionId) {
|
|
|
+ return false;
|
|
|
+ }
|
|
|
+ return true;
|
|
|
+ ```
|
|
|
+ Result: Strict validation enforced
|
|
|
+
|
|
|
+## Root Cause: Why This Separation Matters
|
|
|
+
|
|
|
+**Historical Context**: At some point, the frontend stopped sending `previousRevision` (revisionId) for certain scenarios to support Yjs collaborative editing. This broke features that relied on previousRevision data being available.
|
|
|
+
|
|
|
+**The Core Issue**:
|
|
|
+- **Conflict detection** needs to know "Is this save conflicting with another user's changes?" (Answered by revision check)
|
|
|
+- **Diff detection** needs to know "Did the content actually change?" (Answered by comparing body)
|
|
|
+- **Current implementation conflates these**: When conflict detection is bypassed, previousRevision is not fetched, breaking diff detection
|
|
|
+
|
|
|
+**The Solution Pattern**:
|
|
|
+```typescript
|
|
|
+// Separate the two concerns:
|
|
|
+
|
|
|
+// 1. Fetch previousRevision for data purposes (diff detection, history, etc.)
|
|
|
+let previousRevision: IRevisionHasId | null = null;
|
|
|
+if (sanitizeRevisionId != null) {
|
|
|
+ previousRevision = await Revision.findById(sanitizeRevisionId);
|
|
|
+} else if (currentPage.revision != null) {
|
|
|
+ previousRevision = await Revision.findById(currentPage.revision); // Fallback
|
|
|
+}
|
|
|
+
|
|
|
+// 2. Use previousRevision data for your feature
|
|
|
+const previousBody = previousRevision?.body ?? null;
|
|
|
+
|
|
|
+// 3. Conflict detection happens independently via isUpdatable()
|
|
|
+if (currentPage != null && !(await currentPage.isUpdatable(sanitizeRevisionId, origin))) {
|
|
|
+ // Return conflict error
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+## Reference
|
|
|
+
|
|
|
+**Official Documentation**:
|
|
|
+- https://dev.growi.org/651a6f4a008fee2f99187431#origin-%E3%81%AE%E5%BC%B7%E5%BC%B1
|
|
|
+
|
|
|
+**Related Files**:
|
|
|
+- Frontend: `apps/app/src/client/components/PageEditor/PageEditor.tsx` (lines 158, 240, 308-310)
|
|
|
+- Backend: `apps/app/src/server/models/obsolete-page.js` (lines 159-182, isUpdatable method)
|
|
|
+- API: `apps/app/src/server/routes/apiv3/page/update-page.ts` (lines 260-282, conflict check)
|
|
|
+- Interface: `packages/core/src/interfaces/revision.ts` (lines 6-11, Origin definition)
|
|
|
+
|
|
|
+## Common Pitfalls
|
|
|
+
|
|
|
+1. **Assuming revisionId is always available**: It's not! Editor mode with recent editor/view saves omits it by design.
|
|
|
+2. **Conflating conflict detection with other features**: They serve different purposes and need separate logic.
|
|
|
+3. **Breaking Yjs collaborative editing**: Forcing revisionId to always be sent breaks the bypass mechanism.
|
|
|
+4. **Ignoring origin values**: The system behavior changes significantly based on origin combinations.
|
|
|
+
|
|
|
+## Lessons Learned
|
|
|
+
|
|
|
+This pattern was identified during the "improve-unchanged-revision" feature implementation, where the initial assumption was that frontend should always send `revisionId` for diff detection. Deep analysis revealed:
|
|
|
+
|
|
|
+- The frontend logic is correct for conflict detection and should NOT be changed
|
|
|
+- Server-side fallback is the correct approach to get previous revision data
|
|
|
+- Two-stage checking is intentional and critical for Yjs collaborative editing
|
|
|
+- Conflict detection and diff detection must be separated
|
|
|
+
|
|
|
+**Key Takeaway**: Always understand the existing architectural patterns before proposing changes. What appears to be a "fix" might actually break carefully designed functionality.
|
mergify[bot]