Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 0 additions & 1 deletion lib/src/action_api/event.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import 'package:duit_kernel/duit_kernel.dart';
import 'package:duit_kernel/src/action_api/command.dart';

/// The [ServerEvent] class represents an event that was sent by the server.
///
Expand Down
1 change: 1 addition & 0 deletions lib/src/misc/index.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ export 'logger/index.dart';
export 'parser.dart';
export "annotations.dart";
export "encode.dart";
export 'json_patch/index.dart';
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
3 changes: 3 additions & 0 deletions lib/src/misc/json_patch/index.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export 'json_patch.dart';
export 'semantics.dart';
export 'template.dart';
Comment thread
lesleysin marked this conversation as resolved.
274 changes: 274 additions & 0 deletions lib/src/misc/json_patch/json_patch.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,274 @@
/// JSON Patch operation base type.
///
/// Path is represented as a list of segments where each segment is either
/// a String (for Map key) or an int (for List index).
abstract class PatchOp {
/// JSON path segments from the document root to the target location.
final List<Object> path;

const PatchOp(this.path);
}
Comment thread
lesleysin marked this conversation as resolved.

/// Add operation.
///
/// For Map: sets the value at the key (same as replace if exists).
/// For List: inserts the value at the index (appends if index == length).
class AddOp extends PatchOp {
final Object? value;
const AddOp({required List<Object> path, this.value}) : super(path);
}
Comment on lines +16 to +19

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Guard against empty paths for Add/Remove to prevent runtime errors.

_addAtPath/_removeAtPath access path[depth] unconditionally; an empty path will throw. Replace supports empty path, but Add/Remove do not. Add constructor-time asserts to fail fast.

 class AddOp extends PatchOp {
   final Object? value;
-  const AddOp({required List<Object> path, this.value}) : super(path);
+  const AddOp({required List<Object> path, this.value})
+      : assert(path.length > 0, 'AddOp.path cannot be empty'),
+        super(path);
 }
@@
 class RemoveOp extends PatchOp {
-  const RemoveOp({required List<Object> path}) : super(path);
+  const RemoveOp({required List<Object> path})
+      : assert(path.length > 0, 'RemoveOp.path cannot be empty'),
+        super(path);
 }

Also applies to: 35-37

🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 16 to 19 and 35 to 37,
the AddOp and RemoveOp constructors currently do not guard against empty paths,
which causes runtime errors when _addAtPath or _removeAtPath access path[depth]
unconditionally. Add asserts in both constructors to ensure the path list is not
empty, so the code fails fast and prevents runtime errors from empty paths.


/// Replace operation.
///
/// For Map: sets the value at the key.
/// For List: sets the value at the index (expands with nulls if needed when
/// ensureIntermediate is enabled during apply).
class ReplaceOp extends PatchOp {
final Object? value;
const ReplaceOp({required List<Object> path, this.value}) : super(path);
}

/// Remove operation.
///
/// For Map: removes the key if exists.
/// For List: removes the element at index if exists.
class RemoveOp extends PatchOp {
const RemoveOp({required List<Object> path}) : super(path);
}

/// Convenience factory for creating patch operations.
class PatchOps {
static AddOp add({required List<Object> path, Object? value}) =>
AddOp(path: path, value: value);
static ReplaceOp replace({required List<Object> path, Object? value}) =>
ReplaceOp(path: path, value: value);
static RemoveOp remove({required List<Object> path}) => RemoveOp(path: path);
}
Comment on lines +39 to +68

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Prevent instantiation of the factory class.

The PatchOps class is used only for its static factory methods. Consider making it non-instantiable.

 /// Convenience factory for creating patch operations.
-class PatchOps {
+abstract final class PatchOps {
   static AddOp add({required List<Object> path, Object? value}) =>
       AddOp(path: path, value: value);
   static ReplaceOp replace({required List<Object> path, Object? value}) =>
       ReplaceOp(path: path, value: value);
   static RemoveOp remove({required List<Object> path}) => RemoveOp(path: path);
 }
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 39 to 46, the PatchOps
class is intended only for static factory methods and should not be
instantiated. Make the class non-instantiable by adding a private constructor to
prevent creating instances of PatchOps.


/// A minimal JSON Patch applier with support for add/replace/remove operations
/// and path-copy semantics.
///
/// - Path-copy: only nodes along the modified path are shallow-copied; all
/// other branches are shared with the original document to keep allocations
/// minimal.
/// - Ensure intermediate: when enabled, missing intermediate nodes (Map/List)
/// are created based on the next path segment type.
final class JsonPatchApplier {
/// Applies a list of patch operations to a JSON-like document.
///
/// The [base] document is not mutated. A new Map is returned; nodes that are
/// not on modified paths are shared with [base].
static Map<String, dynamic> apply(
Map<String, dynamic> base,
List<PatchOp> ops, {
bool ensureIntermediate = true,
}) {
var result = base;
for (final op in ops) {
result = _applySingle(result, op, ensureIntermediate);
}
return result;
}

static Map<String, dynamic> _applySingle(
Map<String, dynamic> base,
PatchOp op,
bool ensure,
) =>
switch (op) {
ReplaceOp() => _applyReplace(base, op, ensure),
AddOp() => _applyAdd(base, op, ensure),
RemoveOp() => _applyRemove(base, op, ensure),
_ => base,
};
Comment thread
lesleysin marked this conversation as resolved.

static Map<String, dynamic> _applyReplace(
Map<String, dynamic> base,
ReplaceOp op,
bool ensure,
) {
final updated = _replaceAtPath(base, op.path, op.value, ensure, 0);
return updated is Map<String, dynamic> ? updated : base;
}
Comment on lines +107 to +114

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Root-level replace: avoid silent no-op when value is not a Map.

If op.path is empty and op.value isn’t a Map<String, dynamic>, _applyReplace returns base silently. Prefer a clear error to surface misuse.

 static Map<String, dynamic> _applyReplace(
   Map<String, dynamic> base,
   ReplaceOp op,
   bool ensure,
 ) {
+  // Replacing the document root: require a Map to preserve return type.
+  if (op.path.isEmpty) {
+    final val = op.value;
+    if (val is Map<String, dynamic>) return val;
+    throw StateError(
+      'Replacing document root requires a Map<String, dynamic>, '
+      'but got ${val.runtimeType}.',
+    );
+  }
   final updated = _replaceAtPath(base, op.path, op.value, ensure, 0);
   return updated is Map<String, dynamic> ? updated : base;
 }

Would you like me to update tests (or add new ones) to cover this case?


I’d like to inspect how _replaceAtPath behaves at the root and the definition of ReplaceOp.value to confirm this suggestion. Running:


🏁 Script executed:

#!/usr/bin/env bash
set -e
rg -n "_replaceAtPath" -B2 -A10 lib/src/misc/json_patch/json_patch.dart
rg -n "class ReplaceOp" -B2 -A5 lib/src/misc/json_patch/json_patch.dart

Length of output: 1937


Enforce error on root replace of non-Map values

The current _applyReplace silently no-ops when op.path is empty and op.value isn’t a Map<String, dynamic>. That can mask incorrect usage—better to fail fast.

• File: lib/src/misc/json_patch/json_patch.dart
• Lines: 107–114

Proposed change:

   static Map<String, dynamic> _applyReplace(
     Map<String, dynamic> base,
     ReplaceOp op,
     bool ensure,
   ) {
+    // Replacing the document root must yield a Map<String, dynamic>.
+    if (op.path.isEmpty) {
+      final val = op.value;
+      if (val is Map<String, dynamic>) return val;
+      throw StateError(
+        'Replacing document root requires a Map<String, dynamic>, '
+        'but got ${val.runtimeType}.',
+      );
+    }
     final updated = _replaceAtPath(base, op.path, op.value, ensure, 0);
     return updated is Map<String, dynamic> ? updated : base;
   }

Would you like to add or update tests to cover:

  • Replacing the root with a valid Map<String, dynamic>
  • Attempting root replace with a non-Map (expecting a StateError)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
static Map<String, dynamic> _applyReplace(
Map<String, dynamic> base,
ReplaceOp op,
bool ensure,
) {
final updated = _replaceAtPath(base, op.path, op.value, ensure, 0);
return updated is Map<String, dynamic> ? updated : base;
}
static Map<String, dynamic> _applyReplace(
Map<String, dynamic> base,
ReplaceOp op,
bool ensure,
) {
// Replacing the document root must yield a Map<String, dynamic>.
if (op.path.isEmpty) {
final val = op.value;
if (val is Map<String, dynamic>) return val;
throw StateError(
'Replacing document root requires a Map<String, dynamic>, '
'but got ${val.runtimeType}.',
);
}
final updated = _replaceAtPath(base, op.path, op.value, ensure, 0);
return updated is Map<String, dynamic> ? updated : base;
}
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart lines 107 to 114, the _applyReplace
method currently does not throw an error when replacing the root with a non-Map
value, silently returning the original base. Modify the method to check if
op.path is empty and op.value is not a Map<String, dynamic>, and in that case
throw a StateError to fail fast. Also, add tests to cover replacing the root
with a valid Map and attempting a root replace with a non-Map expecting the
StateError.


static Object? _replaceAtPath(
Object? node,
List<Object> path,
Object? value,
bool ensure,
int depth,
) {
if (depth == path.length) {
// Terminal: return replacement value
return value;
}

final segment = path[depth];
if (segment is String) {
final Map<String, dynamic> current = _asMap(node, ensure);
final Map<String, dynamic> clone = {...current};
final nextNode = current[segment];
final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1);
clone[segment] = replaced;
return clone;
} else if (segment is int) {
Comment on lines +128 to +136

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Avoid unnecessary cloning on replace when no change occurs.

Compute replaced first and clone only if the child actually changes. This reduces allocations along unaffected branches.

-    if (segment is String) {
-      final Map<String, dynamic> current = _asMap(node, ensure);
-      final Map<String, dynamic> clone = {...current};
-      final nextNode = current[segment];
-      final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1);
-      clone[segment] = replaced;
-      return clone;
-    }
+    if (segment is String) {
+      final Map<String, dynamic> current = _asMap(node, ensure);
+      final nextNode = current[segment];
+      final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1);
+      if (identical(nextNode, replaced)) return current; // no change
+      final Map<String, dynamic> clone = {...current};
+      clone[segment] = replaced;
+      return clone;
+    }
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 128 to 136, the code
clones the current map before checking if the replacement actually changes the
child node, causing unnecessary allocations. Modify the code to first compute
the replaced child node, then compare it with the existing child. Only clone the
current map and update the segment if the replaced child differs from the
original; otherwise, return the original current map to avoid unnecessary
cloning.

final List<dynamic> current = _asList(node, ensure);
final List<dynamic> clone = List<dynamic>.of(current);
_ensureListLen(clone, segment, ensure);
final nextNode = segment < clone.length ? clone[segment] : null;
final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1);
if (segment < clone.length) {
clone[segment] = replaced;
} else {
// If ensure=false and index is out of range, keep original list
// but since we already ensured length when ensure=true, this else
// branch is effectively a no-op.
clone.add(replaced);
}
return clone;
Comment on lines +141 to +150

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix inconsistency between comment and code for out-of-range list index.

The comment states this is a no-op when ensure=false and index is out of range, but the code adds the value. This should either not modify the list or the comment should be updated.

       if (segment < clone.length) {
         clone[segment] = replaced;
       } else {
-        // If ensure=false and index is out of range, keep original list
-        // but since we already ensured length when ensure=true, this else
-        // branch is effectively a no-op.
-        clone.add(replaced);
+        // Only add if ensure=true (list was already expanded by _ensureListLen)
+        if (ensure) {
+          clone.add(replaced);
+        }
       }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1);
if (segment < clone.length) {
clone[segment] = replaced;
} else {
// If ensure=false and index is out of range, keep original list
// but since we already ensured length when ensure=true, this else
// branch is effectively a no-op.
clone.add(replaced);
}
return clone;
final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1);
if (segment < clone.length) {
clone[segment] = replaced;
} else {
// Only add if ensure=true (list was already expanded by _ensureListLen)
if (ensure) {
clone.add(replaced);
}
}
return clone;
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 119 to 128, the comment
says that when ensure is false and the index is out of range, the list should
not be modified, but the code adds the replaced value to the list. To fix this
inconsistency, update the code so that if ensure is false and the index is out
of range, the list remains unchanged (do not add the replaced value), or
alternatively, revise the comment to accurately reflect that the list is
modified by adding the value in this case.

} else {
Comment on lines +136 to +151

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix list handling in replace: negative indices and ensure=false out-of-range.

  • Negative indices can cause RangeError when indexing the list.
  • When ensure=false and index is out of range, current code still appends (contradicts comments and intended no-op).
     } else if (segment is int) {
-      final List<dynamic> current = _asList(node, ensure);
-      final List<dynamic> clone = List<dynamic>.of(current);
-      _ensureListLen(clone, segment, ensure);
-      final nextNode = segment < clone.length ? clone[segment] : null;
+      final List<dynamic> current = _asList(node, ensure);
+      // Negative indices are invalid -> no-op
+      if (segment < 0) return current;
+      final List<dynamic> clone = List<dynamic>.of(current);
+      _ensureListLen(clone, segment, ensure);
+      final nextNode =
+          (segment >= 0 && segment < clone.length) ? clone[segment] : null;
       final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1);
-      if (segment < clone.length) {
+      if (segment >= 0 && segment < clone.length) {
         clone[segment] = replaced;
-      } else {
-        // If ensure=false and index is out of range, keep original list
-        // but since we already ensured length when ensure=true, this else
-        // branch is effectively a no-op.
-        clone.add(replaced);
+      } else if (ensure) {
+        // Only append when `ensure=true` expanded/ensured the length.
+        clone.add(replaced);
       }
       return clone;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} else if (segment is int) {
final List<dynamic> current = _asList(node, ensure);
final List<dynamic> clone = List<dynamic>.of(current);
_ensureListLen(clone, segment, ensure);
final nextNode = segment < clone.length ? clone[segment] : null;
final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1);
if (segment < clone.length) {
clone[segment] = replaced;
} else {
// If ensure=false and index is out of range, keep original list
// but since we already ensured length when ensure=true, this else
// branch is effectively a no-op.
clone.add(replaced);
}
return clone;
} else {
} else if (segment is int) {
final List<dynamic> current = _asList(node, ensure);
// Negative indices are invalid -> no-op
if (segment < 0) return current;
final List<dynamic> clone = List<dynamic>.of(current);
_ensureListLen(clone, segment, ensure);
final nextNode =
(segment >= 0 && segment < clone.length) ? clone[segment] : null;
final replaced = _replaceAtPath(nextNode, path, value, ensure, depth + 1);
if (segment >= 0 && segment < clone.length) {
clone[segment] = replaced;
} else if (ensure) {
// Only append when `ensure=true` expanded/ensured the length.
clone.add(replaced);
}
return clone;
} else {
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 136 to 151, the code
does not correctly handle negative indices which can cause RangeError, and when
ensure is false with an out-of-range index, it incorrectly appends to the list
instead of performing a no-op. Fix this by adding a check for negative indices
to prevent invalid access and modify the logic so that if ensure is false and
the index is out of range, the original list is returned unchanged without
appending.

throw ArgumentError('Unsupported path segment: $segment');
}
}

static Map<String, dynamic> _applyAdd(
Map<String, dynamic> base,
AddOp op,
bool ensure,
) {
final updated = _addAtPath(base, op.path, op.value, ensure, 0);
return updated is Map<String, dynamic> ? updated : base;
}

static Object? _addAtPath(
Object? node,
List<Object> path,
Object? value,
bool ensure,
int depth,
) {
final segment = path[depth];
final isLast = depth == path.length - 1;

if (segment is String) {
final Map<String, dynamic> current = _asMap(node, ensure);
final Map<String, dynamic> clone = {...current};
if (isLast) {
clone[segment] = value;
return clone;
}
final nextNode = current[segment];
final next = _addAtPath(nextNode, path, value, ensure, depth + 1);
clone[segment] = next;
return clone;
} else if (segment is int) {
final List<dynamic> current = _asList(node, ensure);
final List<dynamic> clone = List<dynamic>.of(current);
if (isLast) {
// insert semantics: if index within [0, length], insert; if index
// greater than length and ensure=true, expand with nulls and append.
if (segment <= clone.length) {
clone.insert(segment, value);
} else if (ensure) {
while (clone.length < segment) {
clone.add(null);
}
// insert at tail (same as add)
clone.add(value);
} // else: no-op when ensure=false and index>length
return clone;
}
_ensureListLen(clone, segment, ensure);
final nextNode = segment < clone.length ? clone[segment] : null;
final next = _addAtPath(nextNode, path, value, ensure, depth + 1);
if (segment < clone.length) {
clone[segment] = next;
} else {
// ensure=false and index>length -> no-op; but for consistency, append
clone.add(next);
}
Comment on lines +206 to +211

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix inconsistent behavior when ensure=false and index exceeds list length.

Similar to _replaceAtPath, this code adds a value even when ensure=false and the index is out of range, which contradicts the intended no-op behavior.

       if (segment < clone.length) {
         clone[segment] = next;
       } else {
-        // ensure=false and index>length -> no-op; but for consistency, append
-        clone.add(next);
+        // ensure=false and index>length -> no-op
+        if (ensure) {
+          clone.add(next);
+        }
       }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (segment < clone.length) {
clone[segment] = next;
} else {
// ensure=false and index>length -> no-op; but for consistency, append
clone.add(next);
}
if (segment < clone.length) {
clone[segment] = next;
} else {
// ensure=false and index>length -> no-op
if (ensure) {
clone.add(next);
}
}
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 184 to 189, the code
appends a value to the list even when ensure is false and the index exceeds the
list length, causing inconsistent behavior. Modify the logic to check if ensure
is true before adding the value; if ensure is false and the index is out of
range, perform a no-op instead of appending, aligning with the behavior in
_replaceAtPath.

return clone;
} else {
Comment on lines +186 to +213

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix list handling in add: negative indices and ensure=false out-of-range.

  • List.insert with a negative index will throw.
  • Non-last segment path currently appends even when ensure=false; should be a no-op.
     } else if (segment is int) {
       final List<dynamic> current = _asList(node, ensure);
       final List<dynamic> clone = List<dynamic>.of(current);
       if (isLast) {
         // insert semantics: if index within [0, length], insert; if index
         // greater than length and ensure=true, expand with nulls and append.
-        if (segment <= clone.length) {
+        if (segment >= 0 && segment <= clone.length) {
           clone.insert(segment, value);
         } else if (ensure) {
-          while (clone.length < segment) {
+          while (clone.length < segment) {
             clone.add(null);
           }
           // insert at tail (same as add)
           clone.add(value);
-        } // else: no-op when ensure=false and index>length
+        } // else: no-op when ensure=false or index<0
         return clone;
       }
-      _ensureListLen(clone, segment, ensure);
-      final nextNode = segment < clone.length ? clone[segment] : null;
+      // Non-terminal segment
+      if (segment < 0) return clone; // invalid index -> no-op
+      _ensureListLen(clone, segment, ensure);
+      final nextNode = (segment < clone.length) ? clone[segment] : null;
       final next = _addAtPath(nextNode, path, value, ensure, depth + 1);
-      if (segment < clone.length) {
+      if (segment < clone.length) {
         clone[segment] = next;
-      } else {
-        // ensure=false and index>length -> no-op; but for consistency, append
-        clone.add(next);
+      } else if (ensure) {
+        // Only append when `ensure=true` expanded/ensured the length.
+        clone.add(next);
       }
       return clone;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} else if (segment is int) {
final List<dynamic> current = _asList(node, ensure);
final List<dynamic> clone = List<dynamic>.of(current);
if (isLast) {
// insert semantics: if index within [0, length], insert; if index
// greater than length and ensure=true, expand with nulls and append.
if (segment <= clone.length) {
clone.insert(segment, value);
} else if (ensure) {
while (clone.length < segment) {
clone.add(null);
}
// insert at tail (same as add)
clone.add(value);
} // else: no-op when ensure=false and index>length
return clone;
}
_ensureListLen(clone, segment, ensure);
final nextNode = segment < clone.length ? clone[segment] : null;
final next = _addAtPath(nextNode, path, value, ensure, depth + 1);
if (segment < clone.length) {
clone[segment] = next;
} else {
// ensure=false and index>length -> no-op; but for consistency, append
clone.add(next);
}
return clone;
} else {
} else if (segment is int) {
final List<dynamic> current = _asList(node, ensure);
final List<dynamic> clone = List<dynamic>.of(current);
if (isLast) {
// insert semantics: if index within [0, length], insert; if index
// greater than length and ensure=true, expand with nulls and append.
if (segment >= 0 && segment <= clone.length) {
clone.insert(segment, value);
} else if (ensure) {
while (clone.length < segment) {
clone.add(null);
}
// insert at tail (same as add)
clone.add(value);
} // else: no-op when ensure=false or index<0
return clone;
}
// Non-terminal segment
if (segment < 0) return clone; // invalid index -> no-op
_ensureListLen(clone, segment, ensure);
final nextNode = (segment < clone.length) ? clone[segment] : null;
final next = _addAtPath(nextNode, path, value, ensure, depth + 1);
if (segment < clone.length) {
clone[segment] = next;
} else if (ensure) {
// Only append when `ensure=true` expanded/ensured the length.
clone.add(next);
}
return clone;
} else {
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart lines 186 to 213, the code
incorrectly handles negative indices for list insertion and appends elements
even when ensure is false and the index is out of range for non-last segments.
Fix this by adding checks to prevent negative indices from being used in
List.insert, and modify the logic so that when ensure is false and the index is
out of range for non-last segments, the operation becomes a no-op instead of
appending to the list.

throw ArgumentError('Unsupported path segment: $segment');
}
}

static Map<String, dynamic> _applyRemove(
Map<String, dynamic> base,
RemoveOp op,
bool ensure,
) {
final updated = _removeAtPath(base, op.path, /*ensure=*/ false, 0);
return updated is Map<String, dynamic> ? updated : base;
}
Comment on lines +218 to +225

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Clarify that ensureIntermediate is ignored for remove.

_applyRemove hardcodes ensure=false. If intentional, document this so callers don’t expect creation of intermediate nodes during remove.

   static Map<String, dynamic> _applyRemove(
     Map<String, dynamic> base,
     RemoveOp op,
     bool ensure,
   ) {
-    final updated = _removeAtPath(base, op.path, /*ensure=*/ false, 0);
+    // Note: remove never creates intermediates; ignore `ensure`.
+    final updated = _removeAtPath(base, op.path, /*ensure=*/ false, 0);
     return updated is Map<String, dynamic> ? updated : base;
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
static Map<String, dynamic> _applyRemove(
Map<String, dynamic> base,
RemoveOp op,
bool ensure,
) {
final updated = _removeAtPath(base, op.path, /*ensure=*/ false, 0);
return updated is Map<String, dynamic> ? updated : base;
}
static Map<String, dynamic> _applyRemove(
Map<String, dynamic> base,
RemoveOp op,
bool ensure,
) {
// Note: remove never creates intermediate nodes; the `ensure` flag is ignored.
final updated = _removeAtPath(base, op.path, /*ensure=*/ false, 0);
return updated is Map<String, dynamic> ? updated : base;
}
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 218 to 225, the
_applyRemove method hardcodes the ensure parameter as false when calling
_removeAtPath, which means it does not create intermediate nodes during removal.
Add a comment in the method to explicitly document that ensureIntermediate is
intentionally ignored for remove operations, so callers understand that
intermediate node creation is not supported in this context.


static Object? _removeAtPath(
Object? node,
List<Object> path,
bool ensure,
int depth,
) {
if (node == null) return node; // nothing to remove

final segment = path[depth];
final isLast = depth == path.length - 1;

if (segment is String) {
if (node is! Map) return node; // type mismatch -> no-op
final Map<String, dynamic> current = _asMap(node, false);
final Map<String, dynamic> clone = {...current};
if (isLast) {
clone.remove(segment);
return clone;
}
final nextNode = current[segment];
final next = _removeAtPath(nextNode, path, false, depth + 1);
// If child changed by identity, set; else keep as is
if (!identical(nextNode, next)) {
clone[segment] = next;
}
return clone;
} else if (segment is int) {
if (node is! List) return node; // type mismatch -> no-op
final List<dynamic> current = List<dynamic>.of(node);
if (isLast) {
if (segment >= 0 && segment < current.length) {
current.removeAt(segment);
}
return current;
}
if (segment < 0 || segment >= current.length) {
return current; // out of range -> no-op
}
final nextNode = current[segment];
final next = _removeAtPath(nextNode, path, false, depth + 1);
if (!identical(nextNode, next)) {
current[segment] = next;
}
return current;
} else {
throw ArgumentError('Unsupported path segment: $segment');
}
}

static Map<String, dynamic> _asMap(Object? node, bool ensure) {
if (node is Map<String, dynamic>) return node;
if (node is Map) return node.cast<String, dynamic>();
if (ensure) return <String, dynamic>{};
return <String, dynamic>{};
}
Comment on lines +276 to +281

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Respect the ensure parameter in _asMap method.

The method returns an empty map even when ensure=false, which could mask structural mismatches in the document.

   static Map<String, dynamic> _asMap(Object? node, bool ensure) {
     if (node is Map<String, dynamic>) return node;
     if (node is Map) return node.cast<String, dynamic>();
-    if (ensure) return <String, dynamic>{};
-    return <String, dynamic>{};
+    return ensure ? <String, dynamic>{} : throw StateError('Expected Map at path but found ${node.runtimeType}');
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
static Map<String, dynamic> _asMap(Object? node, bool ensure) {
if (node is Map<String, dynamic>) return node;
if (node is Map) return node.cast<String, dynamic>();
if (ensure) return <String, dynamic>{};
return <String, dynamic>{};
}
static Map<String, dynamic> _asMap(Object? node, bool ensure) {
if (node is Map<String, dynamic>) return node;
if (node is Map) return node.cast<String, dynamic>();
return ensure
? <String, dynamic>{}
: throw StateError(
'Expected Map at path but found ${node.runtimeType}');
}
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 254 to 259, the _asMap
method currently returns an empty map regardless of the ensure parameter's
value, which ignores the intended behavior of not ensuring a map. Modify the
method so that when ensure is false and the node is not a Map, it returns null
or an appropriate non-map value instead of an empty map, thereby respecting the
ensure parameter and avoiding masking structural mismatches.


static List<dynamic> _asList(Object? node, bool ensure) {
if (node is List) return List<dynamic>.of(node);
if (ensure) return <dynamic>[];
return <dynamic>[];
}
Comment on lines +283 to +287

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Respect the ensure parameter in _asList method.

Similar to _asMap, this method returns an empty list even when ensure=false, which could mask structural mismatches.

   static List<dynamic> _asList(Object? node, bool ensure) {
     if (node is List) return List<dynamic>.of(node);
-    if (ensure) return <dynamic>[];
-    return <dynamic>[];
+    return ensure ? <dynamic>[] : throw StateError('Expected List at path but found ${node.runtimeType}');
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
static List<dynamic> _asList(Object? node, bool ensure) {
if (node is List) return List<dynamic>.of(node);
if (ensure) return <dynamic>[];
return <dynamic>[];
}
static List<dynamic> _asList(Object? node, bool ensure) {
if (node is List) return List<dynamic>.of(node);
return ensure
? <dynamic>[]
: throw StateError('Expected List at path but found ${node.runtimeType}');
}
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/json_patch.dart around lines 261 to 265, the _asList
method currently returns an empty list regardless of the ensure parameter's
value, which does not respect the intended behavior. Modify the method so that
when ensure is false, it returns null instead of an empty list, and only returns
an empty list when ensure is true. This change will align _asList's behavior
with _asMap and prevent masking structural mismatches.


static void _ensureListLen(List<dynamic> list, int index, bool ensure) {
if (!ensure) return;
if (index < 0) return;
while (list.length <= index) {
list.add(null);
}
}
}
2 changes: 2 additions & 0 deletions lib/src/misc/json_patch/semantics.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/// Semantics for how to apply the patch to the target location.
enum PatchSemantics { replace, add, remove }
Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Document semantics and provide stable (de)serialization helpers

If these map to RFC 6902, explicitly document unsupported ops (test, move, copy). Also add helpers to convert between enum and wire strings to keep I/O stable and avoid scattering string literals.

Example addition (outside this snippet):

/// RFC6902 ops supported: add, remove, replace.
/// Unsupported: test, move, copy.
enum PatchSemantics { replace, add, remove }

extension PatchSemanticsWire on PatchSemantics {
  String get op {
    switch (this) {
      case PatchSemantics.add: return 'add';
      case PatchSemantics.remove: return 'remove';
      case PatchSemantics.replace: return 'replace';
    }
  }

  static PatchSemantics fromOp(String op) {
    switch (op) {
      case 'add': return PatchSemantics.add;
      case 'remove': return PatchSemantics.remove;
      case 'replace': return PatchSemantics.replace;
      default: throw ArgumentError('Unsupported patch op: $op');
    }
  }
}
🤖 Prompt for AI Agents
In lib/src/misc/json_patch/semantics.dart at lines 1 to 2, add documentation
clarifying that the enum PatchSemantics corresponds to supported RFC 6902
operations (add, remove, replace) and explicitly note unsupported operations
(test, move, copy). Then, implement extension methods on PatchSemantics to
provide stable serialization and deserialization helpers: a getter to convert
enum values to their wire string representations and a static method to parse
strings back to enum values, throwing an error for unsupported ops. This will
centralize string literals and ensure consistent I/O handling.

25 changes: 25 additions & 0 deletions lib/src/misc/json_patch/template.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import 'package:duit_kernel/src/misc/json_patch/semantics.dart';

/// Patch template describes where and how to write a value into the component
/// layout when building an instance.
final class PatchTemplate {
/// JSON path segments from component root to the writable field
/// Example: ['children', 0, 'attributes', 'title']
final List<Object> path;

Comment on lines +8 to +9

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Make path truly immutable and segment-typed (String|int).

Right now path can be mutated by callers after construction, and it accepts any Object. This can lead to subtle bugs in a patch-driven system that relies on immutability and valid JSON-path segments.

Suggest:

  • Store a defensive, unmodifiable copy internally.
  • Expose it via a getter.
  • Validate segments are only String or int.
-  final List<Object> path;
+  // Keep a defensive, unmodifiable copy to preserve immutability guarantees.
+  final List<Object> _path;
+  List<Object> get path => _path;
-  const PatchTemplate({
-    required this.path,
+  PatchTemplate({
+    required List<Object> path,
     required this.sourceKey,
     this.defaultValue,
     this.semantics = PatchSemantics.replace,
-  });
+  })  : assert(path.every((s) => s is String || s is int),
+            'Patch path segments must be String or int'),
+       _path = List<Object>.unmodifiable(path) {
+    assert(sourceKey.isNotEmpty, 'sourceKey cannot be empty');
+  }

Trade-off: dropping const on the constructor to enforce immutability at runtime. If const construction is important for your use-cases, we can add a separate const factory for compile-time-known paths. Let me know and I’ll adapt.

🤖 Prompt for AI Agents
In lib/src/misc/json_patch/template.dart around lines 8 to 9, the field 'path'
is currently a mutable List<Object>, which allows external mutation and accepts
any object type. To fix this, change 'path' to a private field that stores an
unmodifiable defensive copy of the input list, validate that each segment is
either a String or int during construction, and expose it via a public getter
returning an unmodifiable list. This will enforce immutability and type safety
for JSON-path segments. Note that this may require removing the const
constructor if immutability is enforced at runtime.

/// Key in the input data map to read the value from
final String sourceKey;

/// Default value used when sourceKey is missing in input data
final Object? defaultValue;

/// Patch semantics (replace/add/remove). Defaults to replace in most cases.
final PatchSemantics semantics;

Comment on lines +16 to +18

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Nit: clarify semantics doc to reflect actual support

Comment lists “replace/add/remove”, but PatchTemplate currently always uses replace in generation. If future-proofing, keep as-is; otherwise note that generation currently uses replace only, and other semantics are reserved.

🤖 Prompt for AI Agents
In lib/src/misc/json_patch/template.dart around lines 16 to 18, the
documentation for the semantics field mentions "replace/add/remove" but the
current implementation only uses "replace" during generation. Update the comment
to clarify that while the semantics field lists replace, add, and remove, the
current generation logic only supports replace, and other semantics are reserved
for future use.

const PatchTemplate({
required this.path,
required this.sourceKey,
this.defaultValue,
this.semantics = PatchSemantics.replace,
});
}
Loading