[DTP-986] Handle StateObject.tombstone and OBJECT_DELETE messages
#1934
Conversation
WalkthroughThe pull request introduces significant modifications to several classes and interfaces related to the handling of state operations, particularly focusing on the new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/1934/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1934/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1934/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/plugins/liveobjects/liveobject.ts (1)
154-165: Implementtombstone()method to handle object deletionThe
tombstone()method correctly handles tombstoning the object. As a good-to-have improvement, consider implementing the TODO to emit a "deleted" event to notify end users about the object's deletion.test/common/modules/live_objects_helper.js (1)
197-202: Consider using nullish coalescing for tombstone checkThe explicit
=== truecomparison could be simplified using the nullish coalescing operator.Consider this alternative implementation:
- tombstone: tombstone === true, + tombstone: tombstone ?? false,This change would maintain the same behavior while being more idiomatic JavaScript/TypeScript.
Also applies to: 218-223
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
ably.d.ts(1 hunks)src/plugins/liveobjects/livecounter.ts(4 hunks)src/plugins/liveobjects/livemap.ts(7 hunks)src/plugins/liveobjects/liveobject.ts(4 hunks)src/plugins/liveobjects/liveobjects.ts(3 hunks)src/plugins/liveobjects/statemessage.ts(2 hunks)test/common/modules/live_objects_helper.js(3 hunks)test/package/browser/template/src/index-liveobjects.ts(2 hunks)test/realtime/live_objects.test.js(3 hunks)
🔇 Additional comments (20)
src/plugins/liveobjects/livecounter.ts (3)
66-70: Check for tombstoned object before applying operations
The code correctly prevents operations from being applied to a tombstoned LiveCounter by checking isTombstoned().
87-90: Handle OBJECT_DELETE operation in applyOperation
The method correctly processes OBJECT_DELETE operations by calling _applyObjectDelete(), ensuring proper handling of deletions.
Line range hint 105-156: Properly handle tombstoned state in overrideWithStateObject
The method correctly handles tombstoned objects by returning a LiveObjectUpdateNoop when the object is tombstoned, preventing further modifications.
src/plugins/liveobjects/liveobject.ts (4)
55-69: Add _tombstone property to manage tombstoned state
The private _tombstone property is added to track the tombstoned state of the object, initialized to false in the constructor.
166-172: Add isTombstoned() method to check tombstoned state
The isTombstoned() method allows checking whether the object is tombstoned.
202-207: Implement _applyObjectDelete() to handle delete operations
The _applyObjectDelete() method correctly applies object deletion by calling tombstone() and generating an update based on data difference.
231-231: Update overrideWithStateObject signature to include LiveObjectUpdateNoop
The method signature now accounts for no-op updates when the object is tombstoned by including LiveObjectUpdateNoop in the return type.
src/plugins/liveobjects/livemap.ts (5)
119-123: Ensure get method does not return tombstoned objects
The get method correctly checks for tombstoned referenced objects and returns undefined, avoiding exposure of tombstoned objects.
137-147: Exclude tombstoned objects from size() count
The size() method excludes tombstoned objects from the count, providing an accurate size of the LiveMap.
183-187: Prevent operations on tombstoned LiveMap in applyOperation
The applyOperation method properly checks if the LiveMap is tombstoned before applying operations, ensuring no further changes are made to a tombstoned map.
224-227: Handle OBJECT_DELETE operation in applyOperation
The method correctly processes OBJECT_DELETE operations by calling _applyObjectDelete().
286-305: Handle tombstoned state in overrideWithStateObject
The method correctly handles tombstoned LiveMap by returning a LiveObjectUpdateNoop when the map is tombstoned, skipping further processing.
test/realtime/live_objects.test.js (1)
1693-2021: Add comprehensive test cases for OBJECT_DELETE operations
The added test scenarios thoroughly verify the handling of OBJECT_DELETE state operations, ensuring that tombstoned objects are correctly managed and not accessible.
test/package/browser/template/src/index-liveobjects.ts (1)
27-33: Update type annotations to reflect possible undefined values
The type annotations for root.get() and explicitRoot.get() are correctly updated to include undefined, reflecting the possibility of tombstoned or non-valid objects.
Also applies to: 66-66
src/plugins/liveobjects/statemessage.ts (1)
11-11: LGTM: Clean foundation for object deletion functionality
The addition of OBJECT_DELETE to StateOperationAction and tombstone to StateObject provides a solid foundation for implementing the tombstone functionality.
Also applies to: 110-111
src/plugins/liveobjects/liveobjects.ts (2)
7-7: LGTM: Proper type handling for no-op updates
The addition of LiveObjectUpdateNoop to handle updates on tombstoned objects is a clean approach that maintains type safety.
Also applies to: 198-198
258-258: LGTM: Robust handling of OBJECT_DELETE operations
The implementation correctly creates zero-value objects before applying delete operations, ensuring consistent behavior regardless of object existence state.
Also applies to: 259-260, 261-262
test/common/modules/live_objects_helper.js (2)
15-15: LGTM: Consistent action value
The OBJECT_DELETE action value matches the enum in statemessage.ts, maintaining consistency.
184-194: LGTM: Clean implementation of delete operation
The objectDeleteOp method follows the established pattern of other operation methods and properly handles optional parameters.
ably.d.ts (1)
2106-2113: LGTM! The simplified return type and documentation improvements look good.
The changes correctly implement the tombstoned object handling by:
- Simplifying the return type to make it clear that any key access can return
undefined - Documenting that deleted/tombstoned objects will always return
undefined
Let's verify the implementation of this behavior:
✅ Verification successful
Implementation correctly handles tombstoned objects and returns undefined as documented
The implementation in livemap.ts correctly handles all cases where undefined should be returned in the get method:
- When the element doesn't exist in the map
- When the element is tombstoned (
element.tombstone === true) - When the referenced object doesn't exist in the pool
- When the referenced object is not valid (
!refObject.isValid()) - When the referenced object is tombstoned (
refObject.isTombstoned())
The implementation aligns perfectly with the type definition and documentation in ably.d.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of tombstoned object handling in LiveMap.get()
# Test: Search for the implementation to confirm it handles tombstoned objects
ast-grep --pattern $'class LiveMap {
$$$
get($_, $_) {
$$$
if ($TOMBSTONE_CHECK) {
$$$
return undefined;
$$$
}
$$$
}
$$$
}'
Length of output: 166
Script:
#!/bin/bash
# Let's try a different approach to find the LiveMap implementation
# First, let's find files that might contain the LiveMap implementation
fd -t f "LiveMap"
# Then, let's search for the get method implementation in LiveMap
rg -l "class LiveMap"
# As a backup, let's also search for any file containing LiveMap
rg --type ts --type js "LiveMap.*get"
Length of output: 1647
Script:
#!/bin/bash
# Let's examine the implementation in livemap.ts
cat src/plugins/liveobjects/livemap.ts
Length of output: 22548
VeskeR
force-pushed
the
DTP-986/handle-tombstone-and-object-delete
branch
from
ab93b81 to
f370f62
Compare
VeskeR
changed the base branch from
DTP-1104/surface-only-valid-objects
to
integration/liveobjects
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/plugins/liveobjects/liveobject.ts (2)
104-113: Consider emitting the deleted event immediatelyThe TODO comment about emitting a "deleted" event should be addressed as it's crucial for notifying users about object deletion.
Would you like me to help implement the deleted event emission? Here's a suggested implementation:
tombstone(): void { this._tombstone = true; this._dataRef = this._getZeroValueData(); - // TODO: emit "deleted" event so that end users get notified about this object getting deleted + this._eventEmitter.emit('deleted', { data: this._dataRef }); }
170-170: Update return type documentationThe
overrideWithStateObjectmethod's return type has been updated to includeLiveObjectUpdateNoop, but the documentation hasn't been updated to reflect this change.Add documentation for the noop case:
/** * Overrides internal data for live object with data from the given state object. * Provided state object should hold a valid data for current live object, e.g. counter data for LiveCounter, map data for LiveMap. * * State objects are received during SYNC sequence, and SYNC sequence is a source of truth for the current state of the objects, * so we can use the data received from the SYNC sequence directly and override any data values or site timeserials this live object has * without the need to merge them. * * Returns an update object that describes the changes applied based on the object's previous value. + * Returns LiveObjectUpdateNoop if the object is already tombstoned. * * @internal */src/plugins/liveobjects/livecounter.ts (1)
133-140: Consider adding debug logging for tombstone state changesWhile the implementation is correct, adding debug logs would help with troubleshooting.
if (this.isTombstoned()) { + this._client.Logger.logAction( + this._client.logger, + this._client.Logger.LOG_MICRO, + 'LiveCounter.overrideWithStateObject()', + `Skipping state override for tombstoned object; objectId=${this._objectId}` + ); return { noop: true }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
ably.d.ts(1 hunks)src/plugins/liveobjects/livecounter.ts(4 hunks)src/plugins/liveobjects/livemap.ts(7 hunks)src/plugins/liveobjects/liveobject.ts(4 hunks)src/plugins/liveobjects/liveobjects.ts(5 hunks)src/plugins/liveobjects/liveobjectspool.ts(0 hunks)src/plugins/liveobjects/statemessage.ts(2 hunks)test/common/modules/live_objects_helper.js(3 hunks)test/package/browser/template/src/ably.config.d.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(2 hunks)test/realtime/live_objects.test.js(3 hunks)
💤 Files with no reviewable changes (1)
- src/plugins/liveobjects/liveobjectspool.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/plugins/liveobjects/statemessage.ts
- src/plugins/liveobjects/liveobjects.ts
- ably.d.ts
- test/package/browser/template/src/index-liveobjects.ts
👮 Files not reviewed due to content moderation or server errors (3)
- test/common/modules/live_objects_helper.js
- src/plugins/liveobjects/livemap.ts
- test/realtime/live_objects.test.js
🔇 Additional comments (6)
src/plugins/liveobjects/liveobject.ts (2)
42-42: LGTM: Proper initialization of tombstone state
The initialization of the tombstone flag and zero-value data in the constructor is well-implemented.
Also applies to: 51-55
141-145: Verify data cleanup in _applyObjectDelete
The method correctly handles the deletion process by:
- Storing previous data for diff calculation
- Calling tombstone()
- Generating update based on data difference
However, we should verify that no references to the old data are retained.
✅ Verification successful
Let me gather more information about the data cleanup process by checking the implementation of the tombstone method and related data handling.
Let me check the actual implementation of the tombstone method and how _dataRef is handled in it.
Let me try again with correct command syntax:
Based on the gathered information, I can now provide the final response:
No memory leak concerns in _applyObjectDelete
The implementation properly handles data cleanup during object deletion:
- The
tombstone()method correctly sets_dataRefto zero value data using_getZeroValueData() - The previous data reference is only temporarily held for diff calculation
- No lingering references are maintained after the update is generated
The code follows a clean pattern where:
- Old data is captured for diff calculation
- Object is tombstoned (clearing data)
- Diff is calculated and returned
- Previous reference goes out of scope
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential memory leaks by searching for retained references
ast-grep --pattern 'this._dataRef = $_' | grep -v '_getZeroValueData'
Length of output: 303
Script:
#!/bin/bash
# Check the tombstone implementation and data cleanup
ast-grep --pattern 'tombstone() {
$$$
}'
# Also check for any other methods that might be cleaning up _dataRef
ast-grep --pattern 'protected _cleanup() {
$$$
}'
# Look for the base class implementation
rg "abstract class.*LiveObject" -A 10
Length of output: 993
Script:
#!/bin/bash
# Search for tombstone implementation in the LiveObject class and its subclasses
rg "tombstone\(\)" -A 5 --type typescript
# Check how _dataRef is used in the context of deletion
rg "_dataRef" -A 2 -B 2 --type typescript
# Look for any cleanup or disposal patterns
ast-grep --pattern 'protected _dataRef: $_'
Length of output: 201
Script:
#!/bin/bash
# Search for tombstone implementation
rg "tombstone\(\)" -A 5
# Check how _dataRef is used in the context of deletion
rg "_dataRef" -A 2 -B 2
# Look for the complete LiveObject class implementation
rg "class LiveObject" -A 20
Length of output: 16617
src/plugins/liveobjects/livecounter.ts (4)
66-69: LGTM: Early return for tombstoned objects
Good implementation of early return when attempting to apply operations to a tombstoned object.
87-90: LGTM: Clean implementation of OBJECT_DELETE operation
The OBJECT_DELETE case is properly handled by delegating to _applyObjectDelete.
155-156: LGTM: Clear documentation of update object behavior
Good documentation explaining the update object contents for both tombstone and non-tombstone cases.
142-152: Verify atomic state transition during tombstone operation
The state transition logic looks correct but we should verify that there are no race conditions between checking tombstone state and applying changes.
✅ Verification successful
No race conditions in tombstone state transitions
After analyzing the codebase, the tombstone state transitions are properly handled:
- The
tombstone()operation is atomic, simply setting a boolean flag and resetting data - All operations check
isTombstoned()first and treat it as a terminal state:- Operations are rejected if the object is already tombstoned
- Once tombstoned, the state cannot be overridden (early returns with noop)
- The tombstone check happens before any state modifications
The implementation ensures that state transitions are unidirectional (active → tombstoned) and there's no way to revert or modify a tombstoned object, preventing any race conditions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential race conditions in tombstone state handling
ast-grep --pattern 'if ($_.isTombstoned()) {
$$$
} else {
$$$
this.tombstone();
$$$
}'
Length of output: 96
Script:
#!/bin/bash
# Let's search for tombstone-related operations and state transitions
rg -A 5 "tombstone\(\)" src/plugins/liveobjects/
# Also search for any synchronization mechanisms around tombstone
rg -A 5 "isTombstoned|_tombstoned" src/plugins/liveobjects/
# Look for the LiveCounter class implementation
ast-grep --pattern 'class LiveCounter {
$$$
}'
Length of output: 4998
VeskeR
changed the base branch from
integration/liveobjects
to
DTP-1104/surface-only-valid-objects
VeskeR
changed the base branch from
DTP-1104/surface-only-valid-objects
to
integration/liveobjects
Tombstoned objects clear their underlying data by setting it to a zero value: 0 for a counter object, empty map for a map. No state operations can be applied on a tombstoned object. Tombstoned objects are not surfaced to the end users. When deleted, object triggers a subscription callback with cleared data. Resolves DTP-986
This is a temporary solution for handling the access API for tombstoned objects. See comment in Confluence for the relevant discussion [1]. The return type for the LiveCounter.value is not changed as part of this commit, as the tombstoned LiveCounter has a value of 0 for its data. [1] https://ably.atlassian.net/wiki/spaces/LOB/pages/3556671496/LODR-026+Correctness+of+OBJECT_DELETE?focusedCommentId=3593928705
VeskeR
force-pushed
the
DTP-986/handle-tombstone-and-object-delete
branch
from
f370f62 to
4ff3647
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/plugins/liveobjects/liveobject.ts (2)
104-113: TODO comment needs to be addressedThe TODO comment indicates that end users should be notified when an object is deleted. This is an important user experience consideration that should be implemented.
Would you like me to help implement the "deleted" event emission functionality or create a GitHub issue to track this requirement?
141-145: Consider adding validation in_applyObjectDeleteThe method should validate that the object isn't already tombstoned before applying the delete operation to prevent unnecessary state updates.
protected _applyObjectDelete(): TUpdate { + if (this.isTombstoned()) { + throw new this._client.ErrorInfo( + `Cannot delete already tombstoned object with objectId=${this.getObjectId()}`, + 50000, + 500, + ); + } const previousDataRef = this._dataRef; this.tombstone(); return this._updateFromDataDiff(previousDataRef, this._dataRef); }src/plugins/liveobjects/livecounter.ts (1)
133-156: Consider refactoring the state override logicThe state override logic has grown complex with multiple conditions. Consider extracting the tombstone handling into a separate method for better maintainability.
+ private _handleTombstoneState(stateObject: StateObject, previousDataRef: LiveCounterData): LiveCounterUpdate | LiveObjectUpdateNoop { + if (this.isTombstoned()) { + return { noop: true }; + } + + if (stateObject.tombstone) { + this.tombstone(); + return this._updateFromDataDiff(previousDataRef, this._dataRef); + } + + return null; // indicate that normal processing should continue + } overrideWithStateObject(stateObject: StateObject): LiveCounterUpdate | LiveObjectUpdateNoop { // ... validation code ... this._siteTimeserials = stateObject.siteTimeserials ?? {}; - if (this.isTombstoned()) { - return { noop: true }; - } - const previousDataRef = this._dataRef; - if (stateObject.tombstone) { - this.tombstone(); - } else { + + const tombstoneResult = this._handleTombstoneState(stateObject, previousDataRef); + if (tombstoneResult !== null) { + return tombstoneResult; + } + this._createOperationIsMerged = false; this._dataRef = { data: stateObject.counter?.count ?? 0 }; if (!this._client.Utils.isNil(stateObject.createOp)) { this._mergeInitialDataFromCreateOperation(stateObject.createOp); } - } return this._updateFromDataDiff(previousDataRef, this._dataRef); }test/common/modules/live_objects_helper.js (1)
197-202: Consider adding validation for tombstone parameterBoth
mapObjectandcounterObjectmethods should validate the tombstone parameter to ensure it's a boolean when provided.mapObject(opts) { const { objectId, siteTimeserials, initialEntries, materialisedEntries, tombstone } = opts; + if (tombstone !== undefined && typeof tombstone !== 'boolean') { + throw new Error('tombstone parameter must be a boolean when provided'); + } const obj = { object: { objectId, siteTimeserials, tombstone: tombstone === true, // ... rest of the code counterObject(opts) { const { objectId, siteTimeserials, initialCount, materialisedCount, tombstone } = opts; + if (tombstone !== undefined && typeof tombstone !== 'boolean') { + throw new Error('tombstone parameter must be a boolean when provided'); + } const obj = { object: { objectId, siteTimeserials, tombstone: tombstone === true, // ... rest of the codeAlso applies to: 218-223
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
ably.d.ts(1 hunks)src/plugins/liveobjects/livecounter.ts(4 hunks)src/plugins/liveobjects/livemap.ts(7 hunks)src/plugins/liveobjects/liveobject.ts(4 hunks)src/plugins/liveobjects/liveobjects.ts(5 hunks)src/plugins/liveobjects/liveobjectspool.ts(0 hunks)src/plugins/liveobjects/statemessage.ts(2 hunks)test/common/modules/live_objects_helper.js(3 hunks)test/package/browser/template/src/ably.config.d.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(2 hunks)test/realtime/live_objects.test.js(3 hunks)
💤 Files with no reviewable changes (1)
- src/plugins/liveobjects/liveobjectspool.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/plugins/liveobjects/statemessage.ts
- test/package/browser/template/src/ably.config.d.ts
- ably.d.ts
- test/package/browser/template/src/index-liveobjects.ts
🔇 Additional comments (11)
src/plugins/liveobjects/liveobject.ts (1)
42-42: LGTM: Proper initialization of tombstone state
The _tombstone property is correctly initialized to false in the constructor, maintaining a clean initial state for new objects.
Also applies to: 51-55
src/plugins/liveobjects/livecounter.ts (1)
66-69: LGTM: Early return for tombstoned objects
The check prevents operations on tombstoned objects, maintaining the integrity of the deleted state.
test/common/modules/live_objects_helper.js (1)
184-194: LGTM: Clean implementation of object delete operation
The objectDeleteOp method follows the same pattern as other operation methods, maintaining consistency in the codebase.
src/plugins/liveobjects/liveobjects.ts (3)
7-7: LGTM: Import statements are correctly updated
The new imports for LiveObjectUpdateNoop and StateOperationAction are properly added to support the tombstone functionality.
Also applies to: 9-9
236-276: LGTM: Well-structured state message handling implementation
The new _applyStateMessages method provides a robust implementation for handling state messages:
- Proper validation of operation field
- Comprehensive error logging
- Creates zero-value objects before applying operations
- Handles all state operation actions including the new
OBJECT_DELETE
104-104: LGTM: Clean refactoring of state message handling
The handleStateMessages method now properly delegates to the new _applyStateMessages method while maintaining the existing buffering logic.
Also applies to: 162-162
src/plugins/liveobjects/livemap.ts (3)
Line range hint 80-119: LGTM: Robust implementation of tombstone checks in get method
The get method now properly handles tombstoned states:
- Return type correctly updated to include undefined
- Comprehensive checks for tombstoned entries and referenced objects
- Clear documentation of all cases where undefined is returned
130-140: LGTM: Size method correctly handles tombstoned objects
The size method now properly excludes tombstoned objects from the count, maintaining consistency with the tombstone functionality.
267-290: LGTM: Proper handling of tombstone state in overrideWithStateObject
The method correctly:
- Updates site timeserials even for tombstoned objects
- Returns early with noop for tombstoned objects
- Handles new tombstone states appropriately
test/realtime/live_objects.test.js (2)
Line range hint 519-1738: LGTM: Comprehensive test coverage for tombstone functionality
The test suite provides excellent coverage of the new tombstone functionality:
- Tests for tombstone creation and deletion
- Validation of state operations on tombstoned objects
- Subscription callback verification
- Edge case handling
Line range hint 1-2079: Consider adding performance tests for large datasets
While the implementation is robust, it would be beneficial to verify performance with large datasets, particularly:
- Memory usage with many tombstoned objects
- Performance impact of tombstone checks in get/size methods
| * | ||
| * - If no entry is associated with the specified key, `undefined` is returned. | ||
| * - If map entry is tombstoned (deleted), `undefined` is returned. | ||
| * - If this map object is tombstoned, `undefined` is returned. |
There was a problem hiding this comment.
It doesn't look like this method handles this case? Although I can see tests asserting on this behaviour, am I missing something?
There was a problem hiding this comment.
if (refObject.isTombstoned()) {
// tombstoned objects must not be surfaced to the end users
return undefined as T[TKey];
}from the lines below.
probably just missed it among other changes?
There was a problem hiding this comment.
if "this map object", the lines I posted are for the referenced object
There was a problem hiding this comment.
when we tombstone an object, we remove all its data. so a tombstoned map doesn't have anything in its _dataRef, so there is no entry for any key when we call .get.
so If this map object is tombstoned, 'undefined' is returned. case is an implicit consequence of tombstoning a map
There was a problem hiding this comment.
I can add a comment to the method body to clarify this behavior if you'd like
There was a problem hiding this comment.
Yes please, I think that's worth calling out. Thanks
Resolves DTP-986
Summary by CodeRabbit
Release Notes
New Features
OBJECT_DELETEoperation to enhance state management.LiveMap,LiveCounter, andLiveObjectclasses to handle tombstoned states effectively.LiveMapmethods to includeundefinedfor tombstoned entries.Bug Fixes
Documentation
Tests