Validations were not working with Set parameter#414
Validations were not working with Set parameter#414NiveditJain merged 3 commits intoexospherehost:mainfrom
Conversation
NiveditJain
commented
Sep 18, 2025
NiveditJain
commented
- Removed the use of the Set operator for updating graph template fields.
- Directly assigned values to graph template attributes and called save() to persist changes.
- This change enhances code readability and maintains functionality for setting secrets and updating validation status.
- Removed the use of the Set operator for updating graph template fields. - Directly assigned values to graph template attributes and called save() to persist changes. - This change enhances code readability and maintains functionality for setting secrets and updating validation status.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe controller now mutates and saves GraphTemplate instances instead of using Set-based updates, adds store_config to responses, and sets validation to PENDING on upsert. Tests were updated to expect save(), PENDING status, and to include StoreConfig. GraphTemplate model enables validate_on_save. An additional integration test exercises valid→invalid→valid upserts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as StateManager API
participant Ctl as UpsertGraphTemplate Controller
participant DB as GraphTemplate (Beanie)
participant V as Background Validator
rect rgba(230,240,255,0.5)
Note over Ctl,DB: Changed: in-place mutations + save()
C->>API: UpsertGraphTemplateRequest
API->>Ctl: upsert_graph_template(body)
alt Existing template
Ctl->>DB: load by name
Ctl->>DB: mutate fields (secrets, retry_policy, store_config, nodes,...)
Ctl->>DB: save()
else New template
Ctl->>DB: insert(new template).set_secrets()
end
Ctl-->>API: UpsertGraphTemplateResponse (includes store_config, status=PENDING)
API-->>C: 200 OK
end
rect rgba(240,255,240,0.5)
Note over V: Asynchronous validation
Ctl->>V: schedule validation (async)
V->>DB: validate graph
alt valid
V->>DB: set status=VALID, clear errors
else invalid
V->>DB: set status=INVALID, set errors
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @NiveditJain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the graph template update mechanism to address an issue where validations were not properly triggered when using the Set operator. By transitioning from the Set operator to direct attribute assignments and an explicit save() operation, the code becomes more readable and ensures that all necessary validations are correctly applied during updates, maintaining the functionality for managing secrets and validation statuses.
Highlights
- Operator Removal: The
beanie.operators.Setoperator has been removed from theupsert_graph_templatefunction, as it was causing issues with validation. - Direct Attribute Assignment: Graph template fields are now updated by directly assigning values to their attributes, followed by an explicit
save()call to persist changes. - Improved Readability and Validation: This change enhances code readability and ensures that validations are correctly triggered, resolving previous issues where they were not working with the
Setoperator.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where Pydantic validations were not being triggered when updating a GraphTemplate using beanie.operators.Set. The change replaces the atomic update operation with direct attribute assignments followed by a save() call. This correctly enables validation. However, this introduces a critical race condition in a concurrent environment, where simultaneous updates could lead to data loss. I've added a comment detailing the issue and recommending the implementation of optimistic locking to ensure data consistency. The removal of the unused Set import is a good cleanup.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
state-manager/app/controller/upsert_graph_template.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-03T16:46:04.030Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#132
File: state-manager/app/controller/upsert_graph_template.py:11-27
Timestamp: 2025-08-03T16:46:04.030Z
Learning: In the exospherehost codebase, for upsert operations on graph templates, the team prioritizes API idempotency over avoiding race conditions in the database layer implementation. The approach of separate find and insert/update operations is acceptable when the API behavior remains consistent.
Applied to files:
state-manager/app/controller/upsert_graph_template.py
🧬 Code graph analysis (1)
state-manager/app/controller/upsert_graph_template.py (2)
state-manager/app/models/db/graph_template_model.py (1)
set_secrets(266-268)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)
🪛 GitHub Actions: State Manager Unit Tests
state-manager/app/controller/upsert_graph_template.py
[error] 31-31: Pytest error: TypeError: object MagicMock can't be used in 'await' expression when awaiting graph_template.save() at app/controller/upsert_graph_template.py:31 during tests (TestUpsertGraphTemplate). Ensure GraphTemplate.save() is async or mock with AsyncMock in tests.
🔇 Additional comments (1)
state-manager/app/controller/upsert_graph_template.py (1)
26-27: Resetting validation fields on every update. Confirm intended behavior.Setting
validation_status=PENDINGandvalidation_errors=[]unconditionally discards any prior error history and downgradesONGOING/VALID/INVALIDtoPENDING. If that’s the desired UX, proceed; else gate resets to changes innodes/store_configonly.
- Updated the upsert_graph_template function to include store_config in the graph template attributes. - Enabled validation on save in the State model to ensure data integrity during database operations. - Enhanced unit tests for upsert_graph_template to accommodate changes in validation status and store_config handling.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
state-manager/tests/unit/controller/test_upsert_graph_template.py (1)
121-121: Avoid blocking sleep in async tests.Replace time.sleep with await asyncio.sleep(0) (or remove; add_task is already asserted). Blocking sleep can stall the event loop.
-from time import sleep +import asyncio @@ - sleep(1) # wait for the background task to complete + await asyncio.sleep(0) # yield control; background task is mocked
♻️ Duplicate comments (2)
state-manager/app/controller/upsert_graph_template.py (2)
25-31: Non-atomic read–modify–save; enable optimistic concurrency or handle conflicts.This path can lose concurrent writes. Prior comment already flagged this; consider enabling Beanie revisions and handling conflicts.
Apply OCC and surface 409 on conflict:
@@ - await graph_template.save() + try: + await graph_template.save() + except RevisionIdWasChanged: + logger.warning( + "Optimistic concurrency conflict while saving graph template", + namespace_name=namespace_name, + graph_name=graph_name, + x_exosphere_request_id=x_exosphere_request_id, + ) + raise HTTPException(status_code=409, detail="Graph template was updated concurrently. Please retry.")Add import at top:
from beanie.exceptions import RevisionIdWasChangedAnd enable revisions in the model:
class GraphTemplate(... class Settings: + use_revision = True
25-31: Enable assignment validation on GraphTemplate to prevent direct-assignment from bypassing validatorsDirect assignments in state-manager/app/controller/upsert_graph_template.py (retry_policy, store_config, nodes) can bypass Pydantic validators because GraphTemplate/BaseDatabaseModel do not enable assignment validation. Add one of the following to GraphTemplate:
# Pydantic v1 class GraphTemplate(...): ... class Config: validate_assignment = True# Pydantic v2 from pydantic import ConfigDict class GraphTemplate(...): model_config = ConfigDict(validate_assignment=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
state-manager/app/controller/upsert_graph_template.py(2 hunks)state-manager/app/models/db/state.py(1 hunks)state-manager/tests/unit/controller/test_upsert_graph_template.py(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-03T16:46:04.030Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#132
File: state-manager/app/controller/upsert_graph_template.py:11-27
Timestamp: 2025-08-03T16:46:04.030Z
Learning: In the exospherehost codebase, for upsert operations on graph templates, the team prioritizes API idempotency over avoiding race conditions in the database layer implementation. The approach of separate find and insert/update operations is acceptable when the API behavior remains consistent.
Applied to files:
state-manager/app/controller/upsert_graph_template.py
📚 Learning: 2025-09-03T18:20:45.910Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#343
File: state-manager/app/tasks/verify_graph.py:74-77
Timestamp: 2025-09-03T18:20:45.910Z
Learning: Store field validation for ${store.field} references in GraphTemplate is already handled by Pydantic model validators in the GraphTemplate class, checking against store_config.required_keys and store_config.default_values during template creation.
Applied to files:
state-manager/app/controller/upsert_graph_template.py
🧬 Code graph analysis (2)
state-manager/tests/unit/controller/test_upsert_graph_template.py (3)
state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/models/db/graph_template_model.py (1)
set_secrets(266-268)state-manager/app/models/retry_policy_model.py (1)
RetryPolicyModel(18-69)
state-manager/app/controller/upsert_graph_template.py (2)
state-manager/app/models/db/graph_template_model.py (1)
set_secrets(266-268)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)
🔇 Additional comments (7)
state-manager/tests/unit/controller/test_upsert_graph_template.py (6)
87-89: Good: include store_config on the template mock.
108-108: Fixes CI: make save() awaitable.Switching to AsyncMock here unblocks the await in the controller.
125-125: Expectation updated to PENDING — correct.
133-133: Assert save() called — correct with new persistence path.
172-174: Good: include store_config on the new template mock.
315-319: Validation reset to PENDING with errors cleared — matches behavior.Also applies to: 334-336
state-manager/app/models/db/state.py (1)
66-66: Enable Beanie validation-on-save for GraphTemplate (match State).State sets
validate_on_save = True(state-manager/app/models/db/state.py:66); GraphTemplate does not — addvalidate_on_save = Trueinsideclass GraphTemplatein state-manager/app/models/db/graph_template_model.py (or set it once on BaseDatabaseModel) so Save/Replace runs Pydantic validation.⛔ Skipped due to learnings
Learnt from: NiveditJain PR: exospherehost/exospherehost#343 File: state-manager/app/tasks/verify_graph.py:74-77 Timestamp: 2025-09-03T18:20:45.910Z Learning: Store field validation for ${store.field} references in GraphTemplate is already handled by Pydantic model validators in the GraphTemplate class, checking against store_config.required_keys and store_config.default_values during template creation.
- Added a new test for valid and invalid graph upsert scenarios, ensuring proper validation status handling. - Updated the GraphTemplate model to enable validation on save, improving data integrity during database operations. - Removed validation on save from the State model to streamline the validation process. - Updated dependencies in the project, including cffi and ruff, to their latest versions for improved performance and security.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
integration-tests/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
integration-tests/test_upsert_graph.py(1 hunks)state-manager/app/models/db/graph_template_model.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration-tests/test_upsert_graph.py (4)
integration-tests/conftest.py (2)
running_server(108-121)base_url(95-97)python-sdk/exospherehost/node/BaseNode.py (1)
BaseNode(6-100)python-sdk/exospherehost/statemanager.py (2)
StateManager(9-188)upsert_graph(128-188)python-sdk/exospherehost/models.py (1)
GraphNodeModel(16-75)
🔇 Additional comments (2)
integration-tests/test_upsert_graph.py (2)
54-55: LGTM: assertion aligns with VALID status.
57-161: Add @pytest.mark.asyncio to async testDecorate the async test so pytest-asyncio executes it; without the marker pytest may not run the coroutine and can error.
-async def test_valid_invalid_valid_upsert_graph(running_server): +@pytest.mark.asyncio +async def test_valid_invalid_valid_upsert_graph(running_server):Optional: reduce flakiness by replacing
await asyncio.sleep(2)with a readiness poll against the server before calling StateManager.
| class Settings: | ||
| validate_on_save = True | ||
| indexes = [ |
There was a problem hiding this comment.
🧩 Analysis chain
validate_on_save can misvalidate due to stale caches; invalidate cached views on every save.
Switching to .save() with validate_on_save=True is good, but your validators call getters that rely on cached _root_node, _node_by_identifier, _parents_by_identifier, and _path_by_identifier. If the instance mutates nodes before .save(), these caches can be stale and cause incorrect validation results. Invalidate caches before other @model_validator(mode='after') validators run.
Apply this (place it above other @model_validator(mode='after') validators to guarantee order):
class GraphTemplate(BaseDatabaseModel):
@@
class Settings:
validate_on_save = True
indexes = [
@@
]
+ @model_validator(mode='after')
+ def _reset_cached_graph_views(self) -> Self:
+ # Ensure caches reflect latest `nodes` before any other validators rely on them.
+ self._node_by_identifier = None
+ self._parents_by_identifier = None
+ self._path_by_identifier = None
+ self._root_node = None
+ return selfOptional hardening (guards that self-heal if caches drift):
def get_root_node(self) -> NodeTemplate:
- if self._root_node is None:
+ if self._root_node is None or self._root_node.identifier not in {n.identifier for n in self.nodes}:
self._build_root_node()
@@
def get_node_by_identifier(self, identifier: str) -> NodeTemplate | None:
- if self._node_by_identifier is None:
+ if self._node_by_identifier is None or set(self._node_by_identifier.keys()) != {n.identifier for n in self.nodes}:
self._build_node_by_identifier()
@@
def get_parents_by_identifier(self, identifier: str) -> set[str]:
- if self._parents_by_identifier is None:
+ if self._parents_by_identifier is None or set(self._parents_by_identifier.keys()) != {n.identifier for n in self.nodes}:
self._build_parents_path_by_identifier()
@@
def get_path_by_identifier(self, identifier: str) -> set[str]:
- if self._path_by_identifier is None:
+ if self._path_by_identifier is None or set(self._path_by_identifier.keys()) != {n.identifier for n in self.nodes}:
self._build_parents_path_by_identifier()Verification steps:
🏁 Script executed:
#!/bin/bash
# 1) Ensure Beanie version supports validate_on_save
rg -nP 'beanie\s*[<=>]' -S || true
rg -nP 'validate_on_save\s*=' state-manager/app/models/db/graph_template_model.py
# 2) Find any direct writes to `.secrets` (should use set_secrets)
rg -nP '\.secrets\s*=' -S
rg -nP 'set_secrets\s*\(' -SLength of output: 7496
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- graph_template_model.py (1-520) ----"
nl -ba state-manager/app/models/db/graph_template_model.py | sed -n '1,520p' || true
echo
echo "---- repo-wide matches for cache names / builders / getters / validators ----"
rg -n --hidden --no-ignore -S "_root_node|_node_by_identifier|_parents_by_identifier|_path_by_identifier|_build_root_node|_build_node_by_identifier|_build_parents_path_by_identifier|get_root_node|get_node_by_identifier|get_parents_by_identifier|get_path_by_identifier|model_validator" || trueLength of output: 17983
Invalidate cached graph views before other @model_validator(mode='after') validators run
Verified: pyproject specifies "beanie>=2.0.0". Current validators call getters that only rebuild when caches are None — add an early after validator to clear caches so validations read up-to-date nodes.
Apply (place above the other @model_validator(mode='after') validators in state-manager/app/models/db/graph_template_model.py):
class GraphTemplate(BaseDatabaseModel):
@@
class Settings:
validate_on_save = True
indexes = [
@@
]
+ @model_validator(mode='after')
+ def _reset_cached_graph_views(self) -> Self:
+ # Ensure caches reflect latest `nodes` before any other validators rely on them.
+ self._node_by_identifier = None
+ self._parents_by_identifier = None
+ self._path_by_identifier = None
+ self._root_node = None
+ return selfOptional hardening (recommended):
def get_root_node(self) -> NodeTemplate:
- if self._root_node is None:
+ if self._root_node is None or self._root_node.identifier not in {n.identifier for n in self.nodes}:
self._build_root_node()
@@
def get_node_by_identifier(self, identifier: str) -> NodeTemplate | None:
- if self._node_by_identifier is None:
+ if self._node_by_identifier is None or set(self._node_by_identifier.keys()) != {n.identifier for n in self.nodes}:
self._build_node_by_identifier()
@@
def get_parents_by_identifier(self, identifier: str) -> set[str]:
- if self._parents_by_identifier is None:
+ if self._parents_by_identifier is None or set(self._parents_by_identifier.keys()) != {n.identifier for n in self.nodes}:
self._build_parents_path_by_identifier()
@@
def get_path_by_identifier(self, identifier: str) -> set[str]:
- if self._path_by_identifier is None:
+ if self._path_by_identifier is None or set(self._path_by_identifier.keys()) != {n.identifier for n in self.nodes}:
self._build_parents_path_by_identifier()[state-manager/app/models/db/graph_template_model.py]
📝 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.
| class Settings: | |
| validate_on_save = True | |
| indexes = [ | |
| class Settings: | |
| validate_on_save = True | |
| indexes = [ | |
| # ... existing index definitions ... | |
| ] | |
| @model_validator(mode='after') | |
| def _reset_cached_graph_views(self) -> Self: | |
| # Ensure caches reflect latest `nodes` before any other validators rely on them. | |
| self._node_by_identifier = None | |
| self._parents_by_identifier = None | |
| self._path_by_identifier = None | |
| self._root_node = None | |
| return self |
🤖 Prompt for AI Agents
In state-manager/app/models/db/graph_template_model.py around lines 32-34, add a
new @model_validator(mode='after') method placed above the other
after-validators that clears any cached graph-related attributes (e.g., set
attributes like _graph_views_cache, _nodes_cache or equivalent cache fields to
None or call the model's cache-clear helper) so subsequent after-validators read
fresh nodes; keep the validator minimal and side-effect only (invalidate
caches), and ensure it is declared before the existing
@model_validator(mode='after') methods so it runs first.
1 participant
