New 'personal' targets #712
Conversation
|
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. WalkthroughExtends ZooKeeperLibraryProvider with Authz-aware helpers and credential-scoped semantics: read/list now consider personal → tenant → global precedence, process_nodes and subscribe support personal targets, subscribe watches personal scopes, and change handling and docstrings updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Provider as ZooKeeperLibraryProvider
participant ZK as ZooKeeper
Caller->>Provider: list(path)
rect rgb(250,252,255)
note right of Provider #cfe9ff: iterate targets in precedence\n(personal → tenant → global)
loop for each target
Provider->>Provider: resolve tenant / credentials (if needed)
Provider->>Provider: build target_path(path, target)
Provider->>ZK: get_children(target_path)
ZK-->>Provider: children[]
Provider->>Provider: process_nodes(children, base_path, target)
alt size/data required
Provider->>ZK: get_data(size_path_for_target)
ZK-->>Provider: data or NoNode
end
Provider->>Provider: label items with target layer
end
end
Provider-->>Caller: aggregated items (personal → tenant → global)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for "personal" targets in the Zookeeper provider, enabling user-specific data storage and retrieval alongside existing global and tenant-specific functionality.
- Adds "personal" target type that uses authenticated user's credential ID for path construction
- Refactors the
listmethod to process personal, tenant, and global nodes in sequence - Updates the
build_pathmethod to handle personal target with credential-based path resolution
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
asab/library/providers/zookeeper.py
Outdated
| if target == "personal": | ||
| try: | ||
| from ...contextvars import Authz | ||
| authz = Authz.get() | ||
| cred_id = getattr(authz, "CredentialsId", None) | ||
| except LookupError: | ||
| cred_id = None | ||
| if cred_id not in (None, ""): | ||
| node_path = "{}/.personal/{}{}".format(self.BasePath, cred_id, path) |
There was a problem hiding this comment.
The import statement should be moved to the top of the file rather than being imported within the function. Local imports can impact performance and make dependencies less clear.
Copilot uses AI. Check for mistakes.
asab/library/providers/zookeeper.py
Outdated
| if cred_id not in (None, ""): | ||
| node_path = "{}/.personal/{}{}".format(self.BasePath, cred_id, path) |
There was a problem hiding this comment.
When cred_id is None or empty, the personal target falls back to using the original node_path, which means it would use the same path as global target. This could lead to unintended access to global data when personal authentication fails.
| if cred_id not in (None, ""): | |
| node_path = "{}/.personal/{}{}".format(self.BasePath, cred_id, path) | |
| if cred_id in (None, ""): | |
| raise PermissionError("Personal authentication failed: cred_id is missing or empty. Access to personal data is denied.") | |
| node_path = "{}/.personal/{}{}".format(self.BasePath, cred_id, path) |
Copilot uses AI. Check for mistakes.
asab/library/providers/zookeeper.py
Outdated
| # Assign correct layer | ||
| if self.Layer == 0: # Only apply this logic to layer `0` | ||
| layer_label = "0:global" if target == "global" else "0:tenant" | ||
| # 👇 Add your block here |
There was a problem hiding this comment.
This comment appears to be a development placeholder and should be removed before merging to production.
| # 👇 Add your block here |
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
asab/library/providers/zookeeper.py (1)
376-406: Update callers after thebuild_pathsignature changeChanging the signature to
target="..."breaks existing call sites that still passtenant_specific=.... For example,read()now raisesTypeError: build_path() got an unexpected keyword argument 'tenant_specific'. Please migrateread()to the new target-aware flow and keep the personal/tenant/global fallback logic consistent.@@ - try: - # Try tenant-specific path first - node_path = self.build_path(path, tenant_specific=True) - node_data = await self.Zookeeper.get_data(node_path) - - # If not found, try the normal path - if node_data is None: - node_path = self.build_path(path, tenant_specific=False) - node_data = await self.Zookeeper.get_data(node_path) - - if node_data is not None: - return io.BytesIO(initial_bytes=node_data) - else: - return None + try: + global_node_path = self.build_path(path, target="global") + target_paths = [] + + personal_node_path = self.build_path(path, target="personal") + if personal_node_path != global_node_path: + target_paths.append(personal_node_path) + + tenant_node_path = self.build_path(path, target="tenant") + if tenant_node_path not in {global_node_path, personal_node_path}: + target_paths.append(tenant_node_path) + + target_paths.append(global_node_path) + + for node_path in target_paths: + node_data = await self.Zookeeper.get_data(node_path) + if node_data is not None: + return io.BytesIO(initial_bytes=node_data) + return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
asab/library/providers/zookeeper.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
asab/library/providers/zookeeper.py (2)
asab/zookeeper/wrapper.py (2)
get_children(80-91)get(59-77)asab/zookeeper/container.py (1)
get_children(249-250)
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)
asab/library/providers/zookeeper.py (1)
388-436: Fix root-path handling and strengthen assertions in build_path.For BasePath == "" and path == "/", node_path becomes "", and
node_path[0]raises IndexError. Also, prefer startswith for safety.Apply this diff:
@@ - if path != '/': - node_path = "{}{}".format(self.BasePath, path) - else: - node_path = "{}".format(self.BasePath) + if path != '/': + node_path = f"{self.BasePath}{path}" + else: + node_path = (self.BasePath or "/") @@ - assert '//' not in node_path, "Directory path cannot contain double slashes (//). Example format: /library/Templates/" - assert node_path[0] == '/', "Directory path must start with a forward slash (/). For example: /library/Templates/" + assert '//' not in node_path, "Directory path cannot contain double slashes (//). Example format: /library/Templates/" + assert node_path.startswith('/'), "Directory path must start with a forward slash (/). For example: /library/Templates/"Additionally, consider validating identifiers to prevent malformed paths:
- Reject '/' in cred_id and tenant values.
Proposed snippet:
if target == "personal": ... if cred_id not in (None, ""): if "/" in cred_id: raise ValueError("Invalid credentials id contains '/'.") node_path = f"{self.BasePath}/.personal/{cred_id}{path}" elif target == "tenant": ... if tenant: if "/" in tenant: raise ValueError("Invalid tenant id contains '/'.") node_path = f"{self.BasePath}/.tenants/{tenant}{path}"
🧹 Nitpick comments (2)
asab/library/providers/zookeeper.py (2)
498-560: Narrow broad exception handlers in change processing.Replace blind
except Exceptionwith kazoo-specific exceptions and only catch generic exceptions at the outermost boundary if needed. This improves signal/noise and satisfies BLE001.Apply this diff in both personal branches:
- except Exception as e: + except kazoo.exceptions.KazooException as e: L.exception( "Failed to process library changes: '{}'".format(e), struct_data={"path": path, "cred_id": cred_id}, )And similarly for the single-cred branch.
562-574: LGTM on _get_personals.Consider sorting for deterministic order, if useful for tests/logs.
- return cred_ids + return sorted(cred_ids)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
asab/library/providers/zookeeper.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
asab/library/providers/zookeeper.py (5)
asab/library/providers/abc.py (3)
LibraryProviderABC(4-66)read(19-28)subscribe(49-66)asab/library/item.py (1)
LibraryItem(6-25)asab/zookeeper/container.py (3)
ZooKeeperContainer(25-259)get_data(252-256)get_children(249-250)asab/library/service.py (2)
read(203-239)subscribe(702-746)asab/zookeeper/wrapper.py (4)
get_data(94-105)get_children(80-91)exists(49-56)get(59-77)
🪛 Ruff (0.13.1)
asab/library/providers/zookeeper.py
284-284: Avoid specifying long messages outside the exception class
(TRY003)
292-292: Consider moving this statement to an else block
(TRY300)
296-296: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
296-296: Avoid specifying long messages outside the exception class
(TRY003)
312-312: Avoid specifying long messages outside the exception class
(TRY003)
360-360: Do not catch blind exception: Exception
(BLE001)
543-543: Do not catch blind exception: Exception
(BLE001)
553-553: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (6)
asab/library/providers/zookeeper.py (6)
8-9: LGTM on imports for Optional/BytesIO.Consistent with the new return type and usage.
17-17: LGTM on importing Tenant/Authz.Needed for scope resolution.
299-309: Docstring update looks good.Precedence and return semantics are clearly stated.
314-331: Avoid duplicate/global-mislabeled items when personal/tenant resolve to global.As previously noted, when personal/tenant contexts are absent, their paths equal the global path, causing duplicates and misleading layer tags.
Apply this diff to guard against re-processing identical paths:
@@ - items = [] - - # Personal scope - personal_node_path = self.build_path(path, target="personal") - personal_nodes = await self.Zookeeper.get_children(personal_node_path) or [] - items += await self.process_nodes(personal_nodes, path, target="personal") - - # Tenant scope - tenant_node_path = self.build_path(path, target="tenant") - tenant_nodes = await self.Zookeeper.get_children(tenant_node_path) or [] - items += await self.process_nodes(tenant_nodes, path, target="tenant") - - # Global scope - global_node_path = self.build_path(path, target="global") - global_nodes = await self.Zookeeper.get_children(global_node_path) or [] - items += await self.process_nodes(global_nodes, path, target="global") + items = [] + global_node_path = self.build_path(path, target="global") + + # Personal scope + personal_node_path = self.build_path(path, target="personal") + if personal_node_path != global_node_path: + personal_nodes = await self.Zookeeper.get_children(personal_node_path) or [] + items += await self.process_nodes(personal_nodes, path, target="personal") + + # Tenant scope + tenant_node_path = self.build_path(path, target="tenant") + if tenant_node_path not in {global_node_path, personal_node_path}: + tenant_nodes = await self.Zookeeper.get_children(tenant_node_path) or [] + items += await self.process_nodes(tenant_nodes, path, target="tenant") + + # Global scope + global_nodes = await self.Zookeeper.get_children(global_node_path) or [] + items += await self.process_nodes(global_nodes, path, target="global")
409-419: Don’t fall back to global path on missing cred_id for 'personal'.Falling back to global can yield unintended access and mislabeling. Either skip personal scope (let caller fall back to tenant/global) or raise a permission error when 'personal' is explicitly requested without cred.
Suggested change (option 1: skip silently):
- if cred_id not in (None, ""): - node_path = "{}/.personal/{}{}".format(self.BasePath, cred_id, path) + if cred_id not in (None, ""): + node_path = "{}/.personal/{}{}".format(self.BasePath, cred_id, path) + else: + # Leave node_path unchanged; callers should detect and skip this as 'no personal' + passOr (option 2: strict):
- if cred_id not in (None, ""): - node_path = "{}/.personal/{}{}".format(self.BasePath, cred_id, path) + if cred_id in (None, ""): + raise PermissionError("Personal authentication failed: cred_id is missing or empty.") + node_path = "{}/.personal/{}{}".format(self.BasePath, cred_id, path)
437-473: LGTM on subscribe API and scope handling.Covers global, tenant(s), and personal(s); NodeDigests prefilled appropriately.
Please confirm intended behavior for large numbers of credentials/tenants; we may need batching/backoff to avoid long subscribe times if thousands exist.
asab/library/providers/zookeeper.py
Outdated
| async def read(self, path: str) -> Optional[BytesIO]: | ||
| """ | ||
| Read a node with precedence personal → tenant → global. | ||
| Args: | ||
| path: Logical library path starting with '/'. | ||
| Returns: | ||
| BytesIO or None if not found in any scope. | ||
| Raises: | ||
| RuntimeError: If ZooKeeper is not ready. | ||
| """ | ||
| if self.Zookeeper is None: | ||
| L.warning("Zookeeper Client has not been established (yet). Cannot read {}".format(path)) | ||
| raise RuntimeError("Zookeeper Client has not been established (yet). Not ready.") | ||
|
|
||
| try: | ||
| # Try tenant-specific path first | ||
| node_path = self.build_path(path, tenant_specific=True) | ||
| node_data = await self.Zookeeper.get_data(node_path) | ||
|
|
||
| # If not found, try the normal path | ||
| if node_data is None: | ||
| node_path = self.build_path(path, tenant_specific=False) | ||
| for target in ("personal", "tenant", "global"): | ||
| node_path = self.build_path(path, target=target) | ||
| node_data = await self.Zookeeper.get_data(node_path) | ||
|
|
||
| if node_data is not None: | ||
| return io.BytesIO(initial_bytes=node_data) | ||
| else: | ||
| return None | ||
| if node_data is not None: | ||
| return io.BytesIO(initial_bytes=node_data) | ||
| return None | ||
|
|
||
| except kazoo.exceptions.ConnectionClosedError: | ||
| L.warning("Zookeeper library provider is not ready") | ||
| raise RuntimeError("Zookeeper library provider is not ready") | ||
|
|
There was a problem hiding this comment.
Fix precedence bug: global can overshadow tenant when personal/tenant context is absent.
When Authz/Tenant are not set, build_path("personal"/"tenant") resolves to the global path. The current loop will read global at the “personal” step and return early, skipping tenant. This breaks the intended personal → tenant → global precedence.
Also, raise from the original exception to preserve traceback (B904).
Apply this diff:
@@
- try:
- for target in ("personal", "tenant", "global"):
- node_path = self.build_path(path, target=target)
- node_data = await self.Zookeeper.get_data(node_path)
- if node_data is not None:
- return io.BytesIO(initial_bytes=node_data)
- return None
+ try:
+ global_node_path = self.build_path(path, target="global")
+ candidates = []
+
+ # Include personal only if a CredentialsId is available
+ try:
+ authz = Authz.get()
+ cred_id = getattr(authz, "CredentialsId", None)
+ except LookupError:
+ cred_id = None
+ if cred_id not in (None, ""):
+ candidates.append(self.build_path(path, target="personal"))
+
+ # Include tenant only if a tenant is available
+ try:
+ tenant = Tenant.get()
+ except LookupError:
+ tenant = None
+ if tenant:
+ candidates.append(self.build_path(path, target="tenant"))
+
+ # Always include global last
+ candidates.append(global_node_path)
+
+ # De-duplicate while preserving order
+ seen = set()
+ for node_path in candidates:
+ if node_path in seen:
+ continue
+ seen.add(node_path)
+ node_data = await self.Zookeeper.get_data(node_path)
+ if node_data is not None:
+ return io.BytesIO(initial_bytes=node_data)
+ return None
except kazoo.exceptions.ConnectionClosedError:
L.warning("Zookeeper library provider is not ready")
- raise RuntimeError("Zookeeper library provider is not ready")
+ raise RuntimeError("Zookeeper library provider is not ready") from None📝 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.
| async def read(self, path: str) -> Optional[BytesIO]: | |
| """ | |
| Read a node with precedence personal → tenant → global. | |
| Args: | |
| path: Logical library path starting with '/'. | |
| Returns: | |
| BytesIO or None if not found in any scope. | |
| Raises: | |
| RuntimeError: If ZooKeeper is not ready. | |
| """ | |
| if self.Zookeeper is None: | |
| L.warning("Zookeeper Client has not been established (yet). Cannot read {}".format(path)) | |
| raise RuntimeError("Zookeeper Client has not been established (yet). Not ready.") | |
| try: | |
| # Try tenant-specific path first | |
| node_path = self.build_path(path, tenant_specific=True) | |
| node_data = await self.Zookeeper.get_data(node_path) | |
| # If not found, try the normal path | |
| if node_data is None: | |
| node_path = self.build_path(path, tenant_specific=False) | |
| for target in ("personal", "tenant", "global"): | |
| node_path = self.build_path(path, target=target) | |
| node_data = await self.Zookeeper.get_data(node_path) | |
| if node_data is not None: | |
| return io.BytesIO(initial_bytes=node_data) | |
| else: | |
| return None | |
| if node_data is not None: | |
| return io.BytesIO(initial_bytes=node_data) | |
| return None | |
| except kazoo.exceptions.ConnectionClosedError: | |
| L.warning("Zookeeper library provider is not ready") | |
| raise RuntimeError("Zookeeper library provider is not ready") | |
| async def read(self, path: str) -> Optional[BytesIO]: | |
| """ | |
| Read a node with precedence personal → tenant → global. | |
| Args: | |
| path: Logical library path starting with '/'. | |
| Returns: | |
| BytesIO or None if not found in any scope. | |
| Raises: | |
| RuntimeError: If ZooKeeper is not ready. | |
| """ | |
| if self.Zookeeper is None: | |
| L.warning("Zookeeper Client has not been established (yet). Cannot read {}".format(path)) | |
| raise RuntimeError("Zookeeper Client has not been established (yet). Not ready.") | |
| try: | |
| global_node_path = self.build_path(path, target="global") | |
| candidates = [] | |
| # Include personal only if a CredentialsId is available | |
| try: | |
| authz = Authz.get() | |
| cred_id = getattr(authz, "CredentialsId", None) | |
| except LookupError: | |
| cred_id = None | |
| if cred_id not in (None, ""): | |
| candidates.append(self.build_path(path, target="personal")) | |
| # Include tenant only if a tenant is available | |
| try: | |
| tenant = Tenant.get() | |
| except LookupError: | |
| tenant = None | |
| if tenant: | |
| candidates.append(self.build_path(path, target="tenant")) | |
| # Always include global last | |
| candidates.append(global_node_path) | |
| # De-duplicate while preserving order | |
| seen = set() | |
| for node_path in candidates: | |
| if node_path in seen: | |
| continue | |
| seen.add(node_path) | |
| node_data = await self.Zookeeper.get_data(node_path) | |
| if node_data is not None: | |
| return io.BytesIO(initial_bytes=node_data) | |
| return None | |
| except kazoo.exceptions.ConnectionClosedError: | |
| L.warning("Zookeeper library provider is not ready") | |
| raise RuntimeError("Zookeeper library provider is not ready") from None |
🧰 Tools
🪛 Ruff (0.13.1)
284-284: Avoid specifying long messages outside the exception class
(TRY003)
292-292: Consider moving this statement to an else block
(TRY300)
296-296: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
296-296: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In asab/library/providers/zookeeper.py around lines 269 to 297, the loop that
checks ("personal","tenant","global") can return the same global path early when
Authz/Tenant are unset because build_path("personal"/"tenant") resolves to the
global path; change the loop to track already-seen node_path values and skip
duplicates so precedence is honored (only return when a previously-unchecked
path yields data), and in the except block capture the original exception (e)
and re-raise the RuntimeError using "raise ... from e" so the traceback is
preserved.
asab/library/providers/zookeeper.py
Outdated
| if '.' in node and not node.endswith(('.io', '.d')): | ||
| fname = "{}/{}".format(base_path.rstrip("/"), node) | ||
| ftype = "item" | ||
|
|
||
| # Retrieve node size for items only | ||
| try: | ||
| node_path = self.build_path(fname, tenant_specific=(target == "tenant")) | ||
| node_path = self.build_path(fname, target=target) | ||
| zstat = self.Zookeeper.Client.exists(node_path) | ||
| size = zstat.dataLength if zstat else 0 | ||
| except kazoo.exceptions.NoNodeError: | ||
| size = None | ||
| except Exception as e: | ||
| L.warning("Failed to retrieve size for node {}: {}".format(node_path, e)) | ||
| size = None | ||
| else: # Directory check | ||
| else: |
There was a problem hiding this comment.
Avoid blocking the event loop; use async exists and fix size semantics.
Calling self.Zookeeper.Client.exists() here blocks the event loop. Use the async wrapper and return None when size is unknown.
Apply this diff:
- node_path = self.build_path(fname, target=target)
- zstat = self.Zookeeper.Client.exists(node_path)
- size = zstat.dataLength if zstat else 0
- except kazoo.exceptions.NoNodeError:
- size = None
- except Exception as e:
- L.warning("Failed to retrieve size for node {}: {}".format(node_path, e))
- size = None
+ node_path = self.build_path(fname, target=target)
+ zstat = await self.Zookeeper.exists(node_path)
+ size = zstat.dataLength if zstat else None
+ except kazoo.exceptions.KazooException as e:
+ L.warning("Failed to retrieve size for node {}: {}".format(fname, e))
+ size = None📝 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.
| if '.' in node and not node.endswith(('.io', '.d')): | |
| fname = "{}/{}".format(base_path.rstrip("/"), node) | |
| ftype = "item" | |
| # Retrieve node size for items only | |
| try: | |
| node_path = self.build_path(fname, tenant_specific=(target == "tenant")) | |
| node_path = self.build_path(fname, target=target) | |
| zstat = self.Zookeeper.Client.exists(node_path) | |
| size = zstat.dataLength if zstat else 0 | |
| except kazoo.exceptions.NoNodeError: | |
| size = None | |
| except Exception as e: | |
| L.warning("Failed to retrieve size for node {}: {}".format(node_path, e)) | |
| size = None | |
| else: # Directory check | |
| else: | |
| if '.' in node and not node.endswith(('.io', '.d')): | |
| fname = "{}/{}".format(base_path.rstrip("/"), node) | |
| ftype = "item" | |
| try: | |
| node_path = self.build_path(fname, target=target) | |
| zstat = await self.Zookeeper.exists(node_path) | |
| size = zstat.dataLength if zstat else None | |
| except kazoo.exceptions.KazooException as e: | |
| L.warning("Failed to retrieve size for node {}: {}".format(fname, e)) | |
| size = None | |
| else: |
🧰 Tools
🪛 Ruff (0.13.1)
360-360: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In asab/library/providers/zookeeper.py around lines 351 to 363, the synchronous
call to self.Zookeeper.Client.exists(...) blocks the event loop and the code
treats a missing znode as size 0; replace the blocking call with the async
exists wrapper and await it, handle kazoo.exceptions.NoNodeError by setting size
= None, and also ensure that when exists returns falsy you set size = None (not
0); update the surrounding function signature to be async if required and keep
the existing exception handling to log unexpected errors and set size = None.
| if self.Layer == 0: | ||
| if target == "global": | ||
| layer_label = "0:global" | ||
| elif target == "tenant": | ||
| layer_label = "0:tenant" | ||
| elif target == "personal": | ||
| layer_label = "0:personal" | ||
| else: | ||
| layer_label = self.Layer # Keep normal numbering for other layers | ||
| layer_label = self.Layer | ||
|
|
There was a problem hiding this comment.
Do not store string labels in LibraryItem.layers (expects List[int]).
Using "0:global"/"0:tenant"/"0:personal" breaks type guarantees and downstream consumers.
Apply this diff:
- if self.Layer == 0:
- if target == "global":
- layer_label = "0:global"
- elif target == "tenant":
- layer_label = "0:tenant"
- elif target == "personal":
- layer_label = "0:personal"
- else:
- layer_label = self.Layer
+ layer_label = self.LayerIf you need to expose scope, consider adding a separate metadata field in LibraryItem rather than overloading layers. Based on learnings
📝 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.
| if self.Layer == 0: | |
| if target == "global": | |
| layer_label = "0:global" | |
| elif target == "tenant": | |
| layer_label = "0:tenant" | |
| elif target == "personal": | |
| layer_label = "0:personal" | |
| else: | |
| layer_label = self.Layer # Keep normal numbering for other layers | |
| layer_label = self.Layer | |
| layer_label = self.Layer |
🤖 Prompt for AI Agents
In asab/library/providers/zookeeper.py around lines 368–377, the code currently
writes string labels like "0:global" into LibraryItem.layers which is typed as
List[int>; instead, keep layers as integers (e.g., 0) and do not concatenate
scope into the layer value. Set layer_label to the integer self.Layer in all
branches and, if scope needs to be recorded, add or populate a separate metadata
field on the LibraryItem (e.g., item.metadata["scope"] =
"global"|"tenant"|"personal") rather than overloading the layers list.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
asab/library/providers/zookeeper.py (6)
322-331: Remove blind except and simplify personal candidate buildingNo need to catch a broad Exception;
_current_credentials_id()already safely returns None. Use the new helper and explicit None check.Apply this diff:
- cred_id = None - try: - cred_id = self._current_credentials_id() - except Exception: - cred_id = None - personal_candidate = self._personal_node_path(path, cred_id) - if personal_candidate: + cred_id = self._current_credentials_id() + personal_candidate = self._personal_node_path(path, cred_id) + if personal_candidate is not None: candidate_paths.append(personal_candidate)
332-336: Skip tenant candidate when no tenant is set (micro-optimization, clearer intent)Avoid appending a tenant path that collapses to global when no tenant is present.
Apply this diff:
- tenant_candidate = self.build_path(path, tenant_specific=True) - if tenant_candidate not in candidate_paths: - candidate_paths.append(tenant_candidate) + if self._current_tenant_id(): + tenant_candidate = self.build_path(path, tenant_specific=True) + if tenant_candidate not in candidate_paths: + candidate_paths.append(tenant_candidate)
349-351: Preserve/clear traceback when re-raising (B904)Chain the RuntimeError with “from None” to make the source clearer.
Apply this diff:
- raise RuntimeError("Zookeeper library provider is not ready") + raise RuntimeError("Zookeeper library provider is not ready") from None
371-389: Use helper for personal path and drop redundant NoNodeError catch
await self.Zookeeper.get_childrenalready returns None when the node doesn’t exist; no exception is raised. Also avoid manualreplace("//", "/").Apply this diff:
- cred_id = None - try: - cred_id = self._current_credentials_id() - except Exception: - cred_id = None + cred_id = self._current_credentials_id() - if cred_id: - personal_node_path = "{}/.personal/{}{}".format(self.BasePath, cred_id, path) - personal_node_path = personal_node_path.replace("//", "/") - try: - personal_nodes = await self.Zookeeper.get_children(personal_node_path) or [] - except kazoo.exceptions.NoNodeError: - personal_nodes = [] + if cred_id: + personal_node_path = self._personal_node_path(path, cred_id) + personal_nodes = await self.Zookeeper.get_children(personal_node_path) or [] personal_items = await self.process_nodes(personal_nodes, path, target="personal") else: personal_items = []
267-281: Narrow exception handling in contextvar helpersCatch LookupError explicitly; current implementation is fine functionally, but reflect the actual failure modes.
Apply this diff:
- def _current_tenant_id(self): - try: - return Tenant.get() - except LookupError: - return None + def _current_tenant_id(self): + try: + return Tenant.get() + except LookupError: + return NoneAnd similarly
_current_credentials_id()already follows this; no further changes needed.
578-592: Avoid blind excepts in change watcherLimit to LookupError when fetching credentials; keep the outer try/except for digest computations.
Apply this diff:
- elif target == "personal": - cred_id = None - try: - cred_id = self._current_credentials_id() - except Exception: - cred_id = None + elif target == "personal": + cred_id = self._current_credentials_id() if cred_id: try: await do_check_path(actual_path="/.personal/{}{}".format(cred_id, path)) except Exception as e: L.exception( "Failed to process library changes: '{}'".format(e), struct_data={"path": path, "credentials_id": cred_id}, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
asab/library/providers/zookeeper.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
asab/library/providers/zookeeper.py (4)
asab/zookeeper/wrapper.py (4)
get(59-77)get_data(94-105)get_children(80-91)exists(49-56)asab/library/service.py (1)
read(203-239)asab/library/providers/filesystem.py (1)
read(67-85)asab/zookeeper/container.py (2)
get_data(252-256)get_children(249-250)
🪛 Ruff (0.13.1)
asab/library/providers/zookeeper.py
299-299: Avoid specifying long messages outside the exception class
(TRY003)
317-317: Avoid specifying long messages outside the exception class
(TRY003)
326-326: Do not catch blind exception: Exception
(BLE001)
347-347: Consider moving this statement to an else block
(TRY300)
375-375: Do not catch blind exception: Exception
(BLE001)
427-427: Do not catch blind exception: Exception
(BLE001)
582-582: Do not catch blind exception: Exception
(BLE001)
587-587: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
asab/library/providers/zookeeper.py (2)
417-429: Don’t block the event loop; use async exists and correct size semantics
self.Zookeeper.Client.existsis synchronous; use the async wrapper and return None when size is unknown. Also,existsnever raises NoNodeError.Apply this diff:
- # Retrieve node size for items only - try: - if target == "personal": - node_path = self._personal_node_path(fname, self._current_credentials_id()) - else: - node_path = self.build_path(fname, tenant_specific=(target == "tenant")) - zstat = self.Zookeeper.Client.exists(node_path) - size = zstat.dataLength if zstat else 0 - except kazoo.exceptions.NoNodeError: - size = None - except Exception as e: - L.warning("Failed to retrieve size for node {}: {}".format(fname, e)) - size = None + # Retrieve node size for items only + try: + if target == "personal": + node_path = self._personal_node_path(fname, self._current_credentials_id()) + else: + node_path = self.build_path(fname, tenant_specific=(target == "tenant")) + if node_path is None: + size = None + else: + zstat = await self.Zookeeper.exists(node_path) + size = zstat.dataLength if zstat else None + except kazoo.exceptions.KazooException as e: + L.warning("Failed to retrieve size for node {}: {}".format(fname, e)) + size = None
435-447: Keep LibraryItem.layers as List[int], not stringsWriting "0:global"/"0:tenant"/"0:personal" breaks type guarantees. Use the integer layer only.
Apply this diff:
- # Assign correct layer label for top layer 0 (presentational only) - if self.Layer == 0: - if target == "global": - layer_label = "0:global" - elif target == "tenant": - layer_label = "0:tenant" - elif target == "personal": - layer_label = "0:personal" - else: - layer_label = "0:{}".format(target) - else: - layer_label = self.Layer + # Keep layers as integers + layer_label = self.LayerIf you need to expose scope, prefer a separate metadata field instead of overloading layers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
asab/library/providers/zookeeper.py (5)
353-356: Preserve exception context (B904): raise from NoneImprove error signaling and satisfy tooling.
Apply:
except kazoo.exceptions.ConnectionClosedError: L.warning("Zookeeper library provider is not ready") - raise RuntimeError("Zookeeper library provider is not ready") + raise RuntimeError("Zookeeper library provider is not ready") from None
327-335: Remove redundant broad try/except around credentials lookup_current_credentials_id() already handles LookupError and returns None; catching Exception is noisy (BLE001).
Apply:
- cred_id = None - try: - cred_id = self._current_credentials_id() - except Exception: - cred_id = None + cred_id = self._current_credentials_id() personal_candidate = self._personal_node_path(path, cred_id) if personal_candidate: candidate_paths.append(personal_candidate)Based on learnings
375-392: Reuse helper and drop unnecessary exception handling for personal nodesget_children wrapper returns None on NoNodeError; no need for try/except. Also avoid manual path building.
Apply:
- cred_id = None - try: - cred_id = self._current_credentials_id() - except Exception: - cred_id = None - - if cred_id: - personal_node_path = "{}/.personal/{}{}".format(self.BasePath, cred_id, path) - personal_node_path = personal_node_path.replace("//", "/") - try: - personal_nodes = await self.Zookeeper.get_children(personal_node_path) or [] - except kazoo.exceptions.NoNodeError: - personal_nodes = [] - personal_items = await self.process_nodes(personal_nodes, path, target="personal") - else: - personal_items = [] + cred_id = self._current_credentials_id() + personal_node_path = self._personal_node_path(path, cred_id) + if personal_node_path: + personal_nodes = await self.Zookeeper.get_children(personal_node_path) or [] + personal_items = await self.process_nodes(personal_nodes, path, target="personal") + else: + personal_items = []Based on learnings
295-302: Avoid blind exception and simplify lookupThe extra try/except is redundant; _current_credentials_id() already returns None when unavailable.
Apply:
- if cred_id is None: - try: - cred_id = self._current_credentials_id() - except Exception: - cred_id = None + if cred_id is None: + cred_id = self._current_credentials_id()Based on learnings
412-413: Simplify dot-component detectionUse any(...) for readability.
Apply:
- startswithdot = functools.reduce(lambda x, y: x or y.startswith('.'), node.split(os.path.sep), False) + startswithdot = any(part.startswith('.') for part in node.split(os.path.sep))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
asab/library/providers/zookeeper.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
asab/library/providers/zookeeper.py (2)
asab/zookeeper/wrapper.py (4)
get(59-77)get_data(94-105)get_children(80-91)exists(49-56)asab/zookeeper/container.py (2)
get_data(252-256)get_children(249-250)
🪛 Ruff (0.13.1)
asab/library/providers/zookeeper.py
298-298: Do not catch blind exception: Exception
(BLE001)
321-321: Avoid specifying long messages outside the exception class
(TRY003)
330-330: Do not catch blind exception: Exception
(BLE001)
351-351: Consider moving this statement to an else block
(TRY300)
379-379: Do not catch blind exception: Exception
(BLE001)
431-431: Do not catch blind exception: Exception
(BLE001)
586-586: Do not catch blind exception: Exception
(BLE001)
591-591: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
asab/library/providers/zookeeper.py (3)
265-266: Remove placeholder commentThis looks like a leftover note inside the class.
Apply:
- # inside class ZooKeeperLibraryProvider -
439-451: Fix type violation: layers must be List[int], not stringsLibraryItem.layers is typed as List[int]; writing "0:global"/"0:tenant"/"0:personal" will break consumers.
Apply:
- # Assign correct layer label for top layer 0 (presentational only) - if self.Layer == 0: - if target == "global": - layer_label = "0:global" - elif target == "tenant": - layer_label = "0:tenant" - elif target == "personal": - layer_label = "0:personal" - else: - layer_label = "0:{}".format(target) - else: - layer_label = self.Layer + # Preserve integer layer semantics; do not encode scope into layers + layer_label = self.LayerIf you need to expose scope, add a separate metadata field rather than overloading layers. Based on learnings
421-433: Don’t block the event loop; use async exists and guard None for personal pathThis calls the synchronous Kazoo client on the event loop and may pass None when cred_id is absent.
Apply:
- # Retrieve node size for items only - try: - if target == "personal": - node_path = self._personal_node_path(fname, self._current_credentials_id()) - else: - node_path = self.build_path(fname, tenant_specific=(target == "tenant")) - zstat = self.Zookeeper.Client.exists(node_path) - size = zstat.dataLength if zstat else 0 - except kazoo.exceptions.NoNodeError: - size = None - except Exception as e: - L.warning("Failed to retrieve size for node {}: {}".format(fname, e)) - size = None + # Retrieve node size for items only + try: + if target == "personal": + node_path = self._personal_node_path(fname, self._current_credentials_id()) + else: + node_path = self.build_path(fname, tenant_specific=(target == "tenant")) + if not node_path: + size = None + else: + zstat = await self.Zookeeper.exists(node_path) + size = zstat.dataLength if zstat else None + except kazoo.exceptions.KazooException as e: + L.warning("Failed to retrieve size for node {}: {}".format(fname, e)) + size = NoneUse the async wrapper (see asab/zookeeper/wrapper.py: exists). Based on learnings
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
asab/library/providers/zookeeper.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
asab/library/providers/zookeeper.py (3)
asab/zookeeper/wrapper.py (4)
get(59-77)get_data(94-105)get_children(80-91)exists(49-56)asab/library/service.py (1)
read(203-239)asab/zookeeper/container.py (2)
get_data(252-256)get_children(249-250)
🪛 Ruff (0.13.2)
asab/library/providers/zookeeper.py
316-316: Avoid specifying long messages outside the exception class
(TRY003)
325-325: Do not catch blind exception: Exception
(BLE001)
346-346: Consider moving this statement to an else block
(TRY300)
422-422: Do not catch blind exception: Exception
(BLE001)
578-578: Do not catch blind exception: Exception
(BLE001)
| if target == "personal": | ||
| node_path = self._personal_node_path(fname, self._current_credentials_id()) | ||
| else: | ||
| node_path = self.build_path(fname, tenant_specific=(target == "tenant")) | ||
| zstat = self.Zookeeper.Client.exists(node_path) | ||
| size = zstat.dataLength if zstat else 0 | ||
| except kazoo.exceptions.NoNodeError: | ||
| size = None | ||
| except Exception as e: | ||
| L.warning("Failed to retrieve size for node {}: {}".format(node_path, e)) | ||
| L.warning("Failed to retrieve size for node {}: {}".format(fname, e)) | ||
| size = None |
There was a problem hiding this comment.
Avoid blocking the loop when computing item size
self.Zookeeper.Client.exists(...) runs synchronously, so every node blocks the event loop while hitting ZooKeeper. On top of that, a missing node is reported as size 0, which is indistinguishable from empty content. Switch to the async wrapper and surface “missing” as None.
- try:
- if target == "personal":
- node_path = self._personal_node_path(fname, self._current_credentials_id())
- else:
- node_path = self.build_path(fname, tenant_specific=(target == "tenant"))
- zstat = self.Zookeeper.Client.exists(node_path)
- size = zstat.dataLength if zstat else 0
- except kazoo.exceptions.NoNodeError:
- size = None
- except Exception as e:
- L.warning("Failed to retrieve size for node {}: {}".format(fname, e))
- size = None
+ try:
+ if target == "personal":
+ node_path = self._personal_node_path(fname, self._current_credentials_id())
+ else:
+ node_path = self.build_path(fname, tenant_specific=(target == "tenant"))
+ if node_path is None:
+ size = None
+ else:
+ zstat = await self.Zookeeper.exists(node_path)
+ size = zstat.dataLength if zstat else None
+ except kazoo.exceptions.NoNodeError:
+ size = None
+ except kazoo.exceptions.KazooException as e:
+ L.warning("Failed to retrieve size for node {}: {}".format(fname, e))
+ size = None📝 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.
| if target == "personal": | |
| node_path = self._personal_node_path(fname, self._current_credentials_id()) | |
| else: | |
| node_path = self.build_path(fname, tenant_specific=(target == "tenant")) | |
| zstat = self.Zookeeper.Client.exists(node_path) | |
| size = zstat.dataLength if zstat else 0 | |
| except kazoo.exceptions.NoNodeError: | |
| size = None | |
| except Exception as e: | |
| L.warning("Failed to retrieve size for node {}: {}".format(node_path, e)) | |
| L.warning("Failed to retrieve size for node {}: {}".format(fname, e)) | |
| size = None | |
| try: | |
| if target == "personal": | |
| node_path = self._personal_node_path(fname, self._current_credentials_id()) | |
| else: | |
| node_path = self.build_path(fname, tenant_specific=(target == "tenant")) | |
| if node_path is None: | |
| size = None | |
| else: | |
| zstat = await self.Zookeeper.exists(node_path) | |
| size = zstat.dataLength if zstat else None | |
| except kazoo.exceptions.NoNodeError: | |
| size = None | |
| except kazoo.exceptions.KazooException as e: | |
| L.warning("Failed to retrieve size for node {}: {}".format(fname, e)) | |
| size = None |
🧰 Tools
🪛 Ruff (0.13.2)
422-422: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In asab/library/providers/zookeeper.py around lines 414 to 424, the code uses
the synchronous self.Zookeeper.Client.exists(...) which blocks the event loop
and treats a missing node the same as an empty node (size 0); replace the sync
call with the async exists wrapper (await the async exists method on the
Zookeeper client), set size = None if the async exists returns no zstat or
raises kazoo.exceptions.NoNodeError, otherwise set size = zstat.dataLength, and
keep the existing generic Exception handling but ensure it does not call the
synchronous API so the loop is non-blocking.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
asab/library/providers/zookeeper.py (2)
407-418: Use asyncexistswrapper to avoid blocking the event loop.Line 412 calls the synchronous
self.Zookeeper.Client.exists(...), which blocks the event loop. This was flagged in a past review comment.Apply this diff to use the async wrapper:
try: if target == "personal": node_path = self._personal_node_path(fname, self._current_credentials_id()) else: node_path = self.build_path(fname, tenant_specific=(target == "tenant")) - zstat = self.Zookeeper.Client.exists(node_path) - size = zstat.dataLength if zstat else 0 + if node_path is None: + size = None + else: + zstat = await self.Zookeeper.exists(node_path) + size = zstat.dataLength if zstat else None except kazoo.exceptions.NoNodeError: size = None - except Exception as e: + except kazoo.exceptions.KazooException as e: L.warning("Failed to retrieve size for node {}: {}".format(fname, e)) size = NoneBased on learnings
424-435: Remove string layer labels; use integer only.Lines 427-431 assign string values like
"0:global","0:tenant","0:personal"tolayer_label, which is then added toLibraryItem.layers(aList[int]). This breaks the type contract and was flagged as a critical issue in a past review.Apply this diff:
- # Assign correct layer label for top layer 0 (presentational only) - if self.Layer == 0: - if target == "global": - layer_label = "0:global" - elif target == "tenant": - layer_label = "0:tenant" - elif target == "personal": - layer_label = "0:personal" - else: - layer_label = "0:{}".format(target) - else: - layer_label = self.Layer + layer_label = self.LayerIf you need to track scope, add a separate metadata field on
LibraryItemrather than overloadinglayers.Based on learnings
🧹 Nitpick comments (1)
asab/library/providers/zookeeper.py (1)
370-379: Consider deduplicating personal path to prevent edge-case duplicates.While the code checks
tenant_node_path != global_node_path(line 364), there's no similar guard forpersonal_node_path. Ifpersonal_node_pathsomehow resolves to matchtenant_node_pathorglobal_node_path(unlikely but possible in edge cases), duplicate items will be returned.Apply this diff to add defensive checks:
# Process personal nodes for current credentials (if any) cred_id = self._current_credentials_id() personal_node_path = self._personal_node_path(path, cred_id) - if personal_node_path: + if personal_node_path and personal_node_path not in {global_node_path, tenant_node_path}: personal_nodes = await self.Zookeeper.get_children(personal_node_path) or [] personal_items = await self.process_nodes(personal_nodes, path, target="personal") else: personal_items = []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
asab/library/providers/zookeeper.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
asab/library/providers/zookeeper.py (3)
asab/zookeeper/wrapper.py (4)
get(59-77)get_data(94-105)get_children(80-91)exists(49-56)asab/library/service.py (1)
read(203-239)asab/zookeeper/container.py (2)
get_data(252-256)get_children(249-250)
🪛 Ruff (0.13.2)
asab/library/providers/zookeeper.py
316-316: Avoid specifying long messages outside the exception class
(TRY003)
325-325: Do not catch blind exception: Exception
(BLE001)
346-346: Consider moving this statement to an else block
(TRY300)
416-416: Do not catch blind exception: Exception
(BLE001)
572-572: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (4)
asab/library/providers/zookeeper.py (4)
302-350: LGTM! Precedence logic correctly implemented.The updated
readmethod properly implements personal → tenant → global precedence by:
- Building candidates in the correct order
- Deduplicating paths to avoid re-processing the same znode
- Returning on the first match
This addresses the past critical review comment about precedence bugs.
Note: The broad
Exceptioncatch on line 325 is acceptable here as a defensive fallback when accessing context variables.
491-496: LGTM! Personal subscription logic is correct.The implementation properly:
- Checks for credentials before subscribing
- Follows the same pattern as tenant subscriptions
- Only establishes the watch when
cred_idis available
567-576: LGTM! Personal change detection follows established patterns.The implementation:
- Correctly checks for
cred_idbefore processing- Uses the same exception handling pattern as tenant/global branches
- Properly structures the actual_path for personal targets
The broad
Exceptioncatch is acceptable here as defensive error handling to prevent one subscription failure from breaking others.
581-592: LGTM! Clean implementation mirroring_get_tenants.The
_get_personalshelper:
- Properly enumerates credentials with custom content
- Filters hidden entries (starting with ".")
- Gracefully handles missing
/.personaldirectory- Follows the same pattern as the existing
_get_tenantsmethod
| def _personal_node_path(self, path: str, cred_id: typing.Optional[str] = None) -> typing.Optional[str]: | ||
| """ | ||
| Resolve the absolute znode path for the personal target. | ||
| Args: | ||
| path (str): Logical library path (must start with '/'). | ||
| cred_id (str | None): CredentialsId; if None, it is looked up. | ||
| Returns: | ||
| str | None: '/<base>/.personal/<CredentialsId><path>' or None if not available. | ||
| """ | ||
| assert path[:1] == '/' | ||
| cred_id = self._current_credentials_id() | ||
| if not cred_id: | ||
| return None | ||
|
|
||
| base = "{}/.personal/{}{}".format(self.BasePath, cred_id, path) | ||
| return base.rstrip("/") |
There was a problem hiding this comment.
Fix parameter shadowing bug in _personal_node_path.
Line 295 overwrites the cred_id parameter by calling self._current_credentials_id(), making the parameter completely ignored. Callers passing cred_id will have their value discarded.
Apply this diff:
- def _personal_node_path(self, path: str, cred_id: typing.Optional[str] = None) -> typing.Optional[str]:
- """
- Resolve the absolute znode path for the personal target.
-
- Args:
- path (str): Logical library path (must start with '/').
- cred_id (str | None): CredentialsId; if None, it is looked up.
-
- Returns:
- str | None: '/<base>/.personal/<CredentialsId><path>' or None if not available.
- """
- assert path[:1] == '/'
- cred_id = self._current_credentials_id()
- if not cred_id:
- return None
-
- base = "{}/.personal/{}{}".format(self.BasePath, cred_id, path)
- return base.rstrip("/")
+ def _personal_node_path(self, path: str, cred_id: typing.Optional[str] = None) -> typing.Optional[str]:
+ """
+ Resolve the absolute znode path for the personal target.
+
+ Args:
+ path (str): Logical library path (must start with '/').
+ cred_id (str | None): CredentialsId; if None, it is looked up.
+
+ Returns:
+ str | None: '/<base>/.personal/<CredentialsId><path>' or None if not available.
+ """
+ assert path[:1] == '/'
+ if cred_id is None:
+ cred_id = self._current_credentials_id()
+ if not cred_id:
+ return None
+
+ base = "{}/.personal/{}{}".format(self.BasePath, cred_id, path)
+ return base.rstrip("/")📝 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.
| def _personal_node_path(self, path: str, cred_id: typing.Optional[str] = None) -> typing.Optional[str]: | |
| """ | |
| Resolve the absolute znode path for the personal target. | |
| Args: | |
| path (str): Logical library path (must start with '/'). | |
| cred_id (str | None): CredentialsId; if None, it is looked up. | |
| Returns: | |
| str | None: '/<base>/.personal/<CredentialsId><path>' or None if not available. | |
| """ | |
| assert path[:1] == '/' | |
| cred_id = self._current_credentials_id() | |
| if not cred_id: | |
| return None | |
| base = "{}/.personal/{}{}".format(self.BasePath, cred_id, path) | |
| return base.rstrip("/") | |
| def _personal_node_path(self, path: str, cred_id: typing.Optional[str] = None) -> typing.Optional[str]: | |
| """ | |
| Resolve the absolute znode path for the personal target. | |
| Args: | |
| path (str): Logical library path (must start with '/'). | |
| cred_id (str | None): CredentialsId; if None, it is looked up. | |
| Returns: | |
| str | None: '/<base>/.personal/<CredentialsId><path>' or None if not available. | |
| """ | |
| assert path[:1] == '/' | |
| if cred_id is None: | |
| cred_id = self._current_credentials_id() | |
| if not cred_id: | |
| return None | |
| base = "{}/.personal/{}{}".format(self.BasePath, cred_id, path) | |
| return base.rstrip("/") |
🤖 Prompt for AI Agents
In asab/library/providers/zookeeper.py around lines 283 to 300, the method
_personal_node_path currently overwrites the cred_id parameter by
unconditionally calling self._current_credentials_id(), which discards any
caller-provided cred_id; change the logic to only call
self._current_credentials_id() when the incoming cred_id is None (i.e., use the
passed-in cred_id if truthy, otherwise set cred_id =
self._current_credentials_id()); keep the existing early return if cred_id is
still falsy, and retain building base =
"{}/.personal/{}{}".format(self.BasePath, cred_id, path) and returning
base.rstrip("/") .
Summary by CodeRabbit
New Features
Bug Fixes
Documentation