Skip to content

Commit 6fbfde8

Browse files
Copilothotlong
andcommitted
fix: resolve all TODO/FIXME/HACK comments in source files
- Implement OData V4 batch rollback with compensating transactions - Replace CLI template TODO placeholders with actionable guidance - Replace HACK comment with architectural documentation Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
1 parent 2e8950d commit 6fbfde8

4 files changed

Lines changed: 76 additions & 66 deletions

File tree

packages/protocols/odata-v4/src/index.ts

Lines changed: 67 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1425,28 +1425,56 @@ export class ODataV4Plugin implements RuntimePlugin {
14251425
private async processChangeset(requests: any[]): Promise<string[]> {
14261426
const responses: string[] = [];
14271427
const operations: Array<{ type: string; entitySet: string; key?: string; data?: any }> = [];
1428+
const rollbackState: Array<{ type: string; entitySet: string; createdId?: string; previousData?: any }> = [];
14281429
const tempResults: any[] = [];
14291430

14301431
try {
14311432
// First pass: Execute all operations and collect results
14321433
for (let i = 0; i < requests.length; i++) {
14331434
const request = requests[i];
1434-
const response = await this.processBatchRequest(request);
14351435

14361436
// Parse the operation details for potential rollback
1437+
let entitySet: string | undefined;
1438+
let key: string | undefined;
14371439
if (request.method && request.url) {
14381440
const urlParts = request.url.replace(this.config.basePath, '').split('/').filter((p: string) => p);
1439-
const entitySet = urlParts[0]?.split('(')[0];
1441+
entitySet = urlParts[0]?.split('(')[0];
14401442
const keyMatch = request.url.match(/\(([^)]+)\)/);
1441-
const key = keyMatch ? keyMatch[1].replace(/'/g, '') : undefined;
1443+
key = keyMatch ? keyMatch[1].replace(/'/g, '') : undefined;
14421444

1443-
operations.push({
1444-
type: request.method,
1445-
entitySet,
1446-
key,
1447-
data: request.body ? JSON.parse(request.body) : undefined
1448-
});
1445+
if (entitySet) {
1446+
operations.push({
1447+
type: request.method,
1448+
entitySet,
1449+
key,
1450+
data: request.body ? JSON.parse(request.body) : undefined
1451+
});
1452+
}
1453+
}
1454+
1455+
// Store previous state before destructive operations for rollback
1456+
const state: { type: string; entitySet: string; createdId?: string; previousData?: any } = {
1457+
type: request.method || '',
1458+
entitySet: entitySet || ''
1459+
};
1460+
if (entitySet && key && (request.method === 'DELETE' || request.method === 'PATCH' || request.method === 'PUT')) {
1461+
state.previousData = await this.getData(entitySet, key);
1462+
}
1463+
1464+
const response = await this.processBatchRequest(request);
1465+
1466+
// For POST operations, extract the created record ID from the response
1467+
if (request.method === 'POST' && entitySet) {
1468+
const bodyMatch = response.match(/\r\n\r\n(.+)$/s);
1469+
if (bodyMatch) {
1470+
try {
1471+
const created = JSON.parse(bodyMatch[1]);
1472+
state.createdId = created._id || created.id;
1473+
} catch { /* non-JSON response */ }
1474+
}
14491475
}
1476+
1477+
rollbackState.push(state);
14501478

14511479
// Check if any request failed
14521480
if (response.includes('HTTP/1.1 4') || response.includes('HTTP/1.1 5')) {
@@ -1470,7 +1498,7 @@ export class ODataV4Plugin implements RuntimePlugin {
14701498
// Note: This is a best-effort rollback since we don't have true database transactions
14711499
// In a production system, this would use database transaction support
14721500
try {
1473-
await this.rollbackChangeset(operations, tempResults.length);
1501+
await this.rollbackChangeset(rollbackState, tempResults.length);
14741502
} catch (rollbackError) {
14751503
// Error silently ignored
14761504
}
@@ -1495,46 +1523,45 @@ export class ODataV4Plugin implements RuntimePlugin {
14951523
}
14961524

14971525
/**
1498-
* Attempt to rollback completed changeset operations
1499-
*
1500-
* ⚠️ IMPORTANT: This is a DEMONSTRATION-ONLY implementation.
1501-
* DO NOT use in production without proper database transaction support!
1502-
*
1503-
* This method only LOGS rollback intentions but does NOT actually reverse operations.
1526+
* Attempt to rollback completed changeset operations via compensating transactions.
15041527
*
1505-
* For production use, you must implement ONE of the following:
1506-
* 1. Database transaction support (BEGIN TRANSACTION / ROLLBACK)
1507-
* 2. Compensating transaction pattern with state storage
1508-
* 3. Event sourcing with operation replay capability
1528+
* ⚠️ IMPORTANT: This is a best-effort compensating transaction implementation.
1529+
* For true atomicity in production, use database-level transaction support
1530+
* (BEGIN TRANSACTION / ROLLBACK).
15091531
*
1510-
* Current limitations:
1511-
* - Does not actually reverse operations (logs intentions only)
1512-
* - Requires storing created IDs, deleted records, and previous values
1513-
* - No guaranteed atomicity without database transaction support
1532+
* Rollback strategy (executed in reverse order):
1533+
* - POST operations: deletes the created record using the stored ID
1534+
* - DELETE operations: re-inserts the record using pre-fetched data
1535+
* - PATCH/PUT operations: restores the record to its pre-fetched values
15141536
*
1515-
* Implementation requirements for true rollback:
1516-
* - Store created IDs during POST operations for deletion
1517-
* - Store deleted records before DELETE operations for restoration
1518-
* - Store previous values before PATCH/PUT operations for reversion
1537+
* Individual rollback failures are silently ignored to allow remaining
1538+
* operations to proceed on a best-effort basis.
15191539
*/
1520-
private async rollbackChangeset(operations: Array<{ type: string; entitySet: string; key?: string; data?: any }>, completedCount: number): Promise<void> {
1540+
private async rollbackChangeset(rollbackState: Array<{ type: string; entitySet: string; createdId?: string; previousData?: any }>, completedCount: number): Promise<void> {
15211541
// Rollback in reverse order
15221542
for (let i = completedCount - 1; i >= 0; i--) {
1523-
const op = operations[i];
1543+
const state = rollbackState[i];
15241544
try {
15251545
// Reverse the operation
1526-
if (op.type === 'POST') {
1527-
// Created record - try to delete it
1528-
// TODO: Need to extract and store the created ID from the response
1529-
} else if (op.type === 'DELETE') {
1530-
// Deleted record - try to restore it
1531-
// TODO: Need to store the deleted record data before deletion
1532-
} else if (op.type === 'PATCH' || op.type === 'PUT') {
1533-
// Updated record - try to restore previous values
1534-
// TODO: Need to fetch and store previous values before update
1546+
if (state.type === 'POST') {
1547+
// Created record — delete it using the stored ID
1548+
if (state.createdId) {
1549+
await this.deleteData(state.entitySet, state.createdId);
1550+
}
1551+
} else if (state.type === 'DELETE') {
1552+
// Deleted record — re-insert it using the stored record data
1553+
if (state.previousData) {
1554+
await this.createData(state.entitySet, state.previousData);
1555+
}
1556+
} else if (state.type === 'PATCH' || state.type === 'PUT') {
1557+
// Updated record — restore previous values
1558+
if (state.previousData && (state.previousData._id || state.previousData.id)) {
1559+
const id = state.previousData._id || state.previousData.id;
1560+
await this.updateData(state.entitySet, id, state.previousData);
1561+
}
15351562
}
15361563
} catch (error) {
1537-
// Error silently ignored
1564+
// Best-effort rollback — continue with remaining operations
15381565
}
15391566
}
15401567
}

packages/tools/cli/src/commands/migrate.ts

Lines changed: 4 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -34,29 +34,13 @@ const MIGRATION_TEMPLATE = `import { ObjectQL } from '@objectql/core';
3434
* Created: {{timestamp}}
3535
*/
3636
export async function up(app: ObjectQL) {
37-
// TODO: Implement migration logic
38-
console.log('Running migration: {{name}}');
39-
40-
// Example: Add a new field to an object
41-
// const tasks = app.getObject('tasks');
42-
// await tasks.updateSchema({
43-
// fields: {
44-
// new_field: { type: 'text', label: 'New Field' }
45-
// }
46-
// });
37+
// Add your migration logic here
38+
// Example: app.getObject('tasks').updateSchema({ fields: { new_field: { type: 'text' } } });
4739
}
4840
4941
export async function down(app: ObjectQL) {
50-
// TODO: Implement rollback logic
51-
console.log('Rolling back migration: {{name}}');
52-
53-
// Example: Remove the field
54-
// const tasks = app.getObject('tasks');
55-
// await tasks.updateSchema({
56-
// fields: {
57-
// new_field: undefined
58-
// }
59-
// });
42+
// Add your rollback logic here
43+
// Example: app.getObject('tasks').updateSchema({ fields: { new_field: undefined } });
6044
}
6145
`;
6246

packages/tools/cli/src/commands/new.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ async function createTsImplementation(type: 'action' | 'hook', name: string, dir
178178
export async function action_${name}(context: ActionContext) {
179179
const { record, user } = context;
180180
181-
// TODO: Implement action logic
181+
// Add your action logic here
182182
console.log('Action ${name} triggered for record:', record._id);
183183
184184
return {
@@ -193,7 +193,7 @@ export async function action_${name}(context: ActionContext) {
193193
export async function beforeInsert(context: HookContext) {
194194
const { doc } = context;
195195
196-
// TODO: Implement before insert logic
196+
// Add your before insert validation or transformation logic here
197197
console.log('Before insert hook for ${name}');
198198
199199
// Modify doc as needed
@@ -203,7 +203,7 @@ export async function beforeInsert(context: HookContext) {
203203
export async function afterInsert(context: HookContext) {
204204
const { doc } = context;
205205
206-
// TODO: Implement after insert logic
206+
// Add your after insert side-effect logic here
207207
console.log('After insert hook for ${name}');
208208
}
209209
`;

packages/tools/cli/src/commands/repl.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,8 @@ export async function startRepl(configPath?: string) {
8080
// We use a getter to lazily create context with system privileges
8181
Object.defineProperty(r.context, obj.name, {
8282
get: () => {
83-
// HACK: We need to construct a repository.
84-
// Since `ObjectRepository` is exported from `@objectql/core`, we can use it if we import it.
85-
// But `app` is passed from user land. We can rely on `require('@objectql/core')` here.
83+
// Dynamic require to avoid circular dependency — ObjectRepository is constructed
84+
// from the user-provided app instance to enable convenient REPL access.
8685
const { ObjectRepository } = require('@objectql/core');
8786

8887
const replContext: any = {

0 commit comments

Comments
 (0)