Implement DL-2: Entity CRUD operations for EntityArchetype and EntityInstance#53
Conversation
Co-authored-by: spuentesp <112034353+spuentesp@users.noreply.github.com>
github-actions
Bot
added
area/data-layer
Co-authored-by: spuentesp <112034353+spuentesp@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive CRUD operations for managing Entity nodes in Neo4j, supporting both EntityArchetype (templates like "Wizard") and EntityInstance (concrete entities like "Gandalf") with dynamic state tracking via tags.
Key Changes
- Entity schemas: New Pydantic models for entity creation, updates, filters, and state tag management with validation logic
- Six Neo4j operations: Complete CRUD implementation including archetype/instance distinction, relationship management, and atomic state tag updates
- Authority controls: Updated permission matrix to enforce CanonKeeper authority for write operations
- Comprehensive testing: 32 unit tests covering all entity types, lifecycle scenarios, and edge cases
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/data-layer/src/monitor_data/schemas/entities.py | Defines Pydantic schemas for entity operations including EntityCreate, EntityUpdate, StateTagsUpdate, EntityFilter, and response models |
| packages/data-layer/src/monitor_data/tools/neo4j_tools.py | Implements six entity operations (create, get, list, update, delete, set_state_tags) with validation, relationship handling, and atomic updates |
| packages/data-layer/src/monitor_data/middleware/auth.py | Updates authority matrix to include entity operations with CanonKeeper permissions for writes and open read access |
| packages/data-layer/tests/test_tools/test_entity_tools.py | Provides 32 unit tests covering all entity types, archetype/instance patterns, state management, and integration scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MATCH (a:Entity {id: $archetype_id}) | ||
| CREATE (e)-[:DERIVES_FROM]->(a) | ||
| """ | ||
| entity_props["archetype_id"] = archetype_id_str |
There was a problem hiding this comment.
The archetype_id is being stored as a property on the Entity node (line 687), but it's already tracked through the DERIVES_FROM relationship. This creates data redundancy where the same information exists in two places (as a property and as a relationship). Consider removing the archetype_id property and relying solely on the DERIVES_FROM relationship to maintain a single source of truth.
There was a problem hiding this comment.
Removed the redundant archetype_id property. Now relying solely on the DERIVES_FROM relationship as the single source of truth. The archetype_id is retrieved dynamically from the relationship in queries. (dacc3be)
| if params.remove_tags: | ||
| update_parts.append( | ||
| "e.state_tags = [tag IN e.state_tags WHERE NOT tag IN $remove_tags]" | ||
| ) | ||
| update_params["remove_tags"] = params.remove_tags | ||
|
|
||
| if params.add_tags: | ||
| # Add tags, avoiding duplicates | ||
| update_parts.append( | ||
| "e.state_tags = e.state_tags + [tag IN $add_tags WHERE NOT tag IN e.state_tags]" | ||
| ) | ||
| update_params["add_tags"] = params.add_tags |
There was a problem hiding this comment.
The order of state_tags operations may produce unexpected results. When both add_tags and remove_tags are specified, remove operations happen first (line 1051), then add operations (line 1058). This means if the same tag appears in both lists, it will be removed and then re-added. Consider documenting this behavior or validating that add_tags and remove_tags don't overlap to prevent confusion.
There was a problem hiding this comment.
Added validation in StateTagsUpdate.model_post_init() to prevent tags from appearing in both add_tags and remove_tags. This raises a clear ValidationError with the overlapping tags listed. (dacc3be)
| # Add state_tags for instances | ||
| if not params.is_archetype: | ||
| entity_props["state_tags"] = params.state_tags | ||
| entity_props["updated_at"] = created_at.isoformat() |
There was a problem hiding this comment.
Archetypes should explicitly set state_tags to an empty list to maintain consistency with instances. Currently, archetypes don't have the state_tags property set at all (it's only added for instances on line 670). This could cause issues when querying, as archetypes would have no state_tags property while instances have an empty or populated list. Consider always setting state_tags, even for archetypes, to an empty list for consistency.
| # Add state_tags for instances | |
| if not params.is_archetype: | |
| entity_props["state_tags"] = params.state_tags | |
| entity_props["updated_at"] = created_at.isoformat() | |
| # Add state_tags for instances; ensure archetypes also have an explicit (empty) list | |
| if not params.is_archetype: | |
| entity_props["state_tags"] = params.state_tags | |
| entity_props["updated_at"] = created_at.isoformat() | |
| else: | |
| entity_props["state_tags"] = [] |
There was a problem hiding this comment.
Archetypes now explicitly set state_tags to an empty list for consistency. This ensures all Entity nodes have the state_tags property, making queries more predictable. (dacc3be)
| if force: | ||
| delete_query = """ | ||
| MATCH (e:Entity {id: $id}) | ||
| DETACH DELETE e | ||
| """ | ||
| else: | ||
| delete_query = """ | ||
| MATCH (e:Entity {id: $id}) | ||
| DETACH DELETE e | ||
| """ |
There was a problem hiding this comment.
The delete queries are identical for both force=True and force=False cases. The conditional logic is redundant - the same DETACH DELETE query can be used in both branches. Consider removing the conditional and always using a single delete query since the dependency check is performed earlier when force=False.
| if force: | |
| delete_query = """ | |
| MATCH (e:Entity {id: $id}) | |
| DETACH DELETE e | |
| """ | |
| else: | |
| delete_query = """ | |
| MATCH (e:Entity {id: $id}) | |
| DETACH DELETE e | |
| """ | |
| delete_query = """ | |
| MATCH (e:Entity {id: $id}) | |
| DETACH DELETE e | |
| """ |
There was a problem hiding this comment.
Removed the redundant conditional - now using a single DETACH DELETE query regardless of the force flag. The dependency check handles the logic earlier when force=False. (dacc3be)
|
@copilot apply changes based on the comments in this thread |
…overlapping tags, set empty state_tags for archetypes, simplify delete logic Co-authored-by: spuentesp <112034353+spuentesp@users.noreply.github.com>
DL-2: Manage Archetypes & Instances Implementation Plan
Overview
Implementing CRUD operations for EntityArchetype and EntityInstance nodes in Neo4j with state_tags support and DERIVES_FROM relationships.
Implementation Complete ✅
All acceptance criteria met and PR review feedback addressed.
Changes from Review Feedback
1. Removed Redundant archetype_id Property
2. Added Validation for Overlapping Tags
3. Set Empty state_tags for Archetypes
[]4. Simplified Delete Logic
Test Results
Files Modified
packages/data-layer/src/monitor_data/schemas/entities.py(added overlap validation)packages/data-layer/src/monitor_data/tools/neo4j_tools.py(3 improvements)packages/data-layer/tests/test_tools/test_entity_tools.py(added test for overlap validation)Acceptance Criteria
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.