Library: Make layers consistently string-typed.#726
Library: Make layers consistently string-typed.#726mithunbharadwaj wants to merge 15 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughReplaces single-root recursion in the ZooKeeper provider with per-root (personal, tenant, global) searches; Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
🤖 Fix all issues with AI agents
In `@asab/library/providers/zookeeper.py`:
- Around line 646-654: The recursive directory check in _recursive_find uses the
simplistic condition '"." not in child' which is inconsistent with process_nodes
(around line 386) that treats names with .io and .d suffixes as directories;
update _recursive_find to use the same directory-detection logic as
process_nodes (either call the same helper function or replicate the suffix
check for '.io' and '.d') so entries like 'templates.d' are recursed into;
reference the _recursive_find method, the process_nodes logic, and the child
variable when making the change.
🧹 Nitpick comments (1)
asab/library/providers/zookeeper.py (1)
630-633: Consider filtering all dotfiles, not just scope directories.Unlike
process_nodes(lines 382-384),_recursive_findonly skips.tenantsand.personal. Other hidden entries (e.g.,.version.yaml) could be matched if the search filename happens to be a suffix. For consistency, consider filtering all dot-prefixed children.Proposed fix
for child in children: # ---- hard stop: never cross scopes ---- - if child in {".tenants", ".personal"}: + if child.startswith("."): continue
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@asab/library/providers/zookeeper.py`:
- Around line 625-630: The loop assumes children is iterable but
self.Zookeeper.get_children may return None (it swallows NoNodeError); update
the code around the call to self.Zookeeper.get_children to handle a None return
(e.g., assign children = await self.Zookeeper.get_children(path) and if children
is None: return or set children = [] ) before executing the for child in
children loop so you don't attempt to iterate over None.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@asab/library/providers/zookeeper.py`:
- Around line 594-600: The find method lacks the readiness guard present in
read/list, so add the same check at the start of async def find(self, filename:
str) to raise a RuntimeError if the ZooKeeper client isn't connected;
specifically replicate the readiness guard used in the list/read methods
(call/check the same readiness attribute or helper and raise RuntimeError with
the same message) before using attributes that assume a connected client (so
tenant/cred logic and subsequent operations won't hit AttributeError).
asab/library/providers/zookeeper.py
Outdated
| async def find(self, filename: str) -> list: | ||
| """ | ||
| Recursively search for files ending with a specific name in ZooKeeper nodes, starting from the base path. | ||
|
|
||
| :param filename: The filename to search for (e.g., '.setup.yaml') | ||
| :return: A list of LibraryItem objects for files ending with the specified name, | ||
| or an empty list if no matching files were found. | ||
| """ | ||
| results = [] | ||
| await self._recursive_find(self.BasePath, filename, results) | ||
|
|
||
| tenant_id = self._current_tenant_id() | ||
| cred_id = self._current_credentials_id() | ||
|
|
||
| search_roots = [] |
There was a problem hiding this comment.
Add a readiness guard in find (match read/list).
Line 594: find can be called before ZooKeeper is connected; it will fail with an AttributeError instead of the clearer RuntimeError used by read/list. Consider adding the same guard for consistency.
💡 Suggested fix
async def find(self, filename: str) -> list:
+ if self.Zookeeper is None:
+ L.warning("Zookeeper Client has not been established (yet). Cannot find {}".format(filename))
+ raise RuntimeError("Zookeeper Client has not been established (yet). Not ready.")
results = []🤖 Prompt for AI Agents
In `@asab/library/providers/zookeeper.py` around lines 594 - 600, The find method
lacks the readiness guard present in read/list, so add the same check at the
start of async def find(self, filename: str) to raise a RuntimeError if the
ZooKeeper client isn't connected; specifically replicate the readiness guard
used in the list/read methods (call/check the same readiness attribute or helper
and raise RuntimeError with the same message) before using attributes that
assume a connected client (so tenant/cred logic and subsequent operations won't
hit AttributeError).
mejroslav
left a comment
mejroslav
left a comment
There was a problem hiding this comment.
Hello @mithunbharadwaj , I have questions before you merge this one.
| """ | ||
| Recursively search for files ending with a specific name in ZooKeeper nodes, starting from the base path. | ||
|
|
||
| :param filename: The filename to search for (e.g., '.setup.yaml') | ||
| :return: A list of LibraryItem objects for files ending with the specified name, | ||
| or an empty list if no matching files were found. | ||
| """ |
There was a problem hiding this comment.
@mithunbharadwaj Did you remove the docstring? Please provide a new one then.
asab/library/providers/zookeeper.py
Outdated
| ) | ||
| else: | ||
| # recurse only into directories | ||
| if "." not in child: |
There was a problem hiding this comment.
This will also skip directories like /Site/LogMan.io/ - don't we have exceptions somewhere in asab.library?
asab/library/providers/zookeeper.py
Outdated
| type="item", # or "dir" if applicable | ||
| layers=[self.Layer], | ||
| type="item", | ||
| layers=[self.Layer if self.Layer != 0 else "0:{}".format(target)], |
There was a problem hiding this comment.
Are you mixing ints and strings here?
layers=[str(self.Layer) if self.Layer != 0 else "0:{}".format(target)]There was a problem hiding this comment.
This is intentional for the new target-aware layer model.
Layer 0 now carries target information ("0:tenant", "0:personal"), and layers are treated as opaque identifiers throughout the library stack.
There was a problem hiding this comment.
I know, but the library layers should be consistent in types.
I propose:
- Double check that
item.Layerattribute is not used as an integer somewhere inasab.libraryand micro-services which use asab. - Change it's type to a string for all layers, not only the zeroth.
- Clearly state that this as a breaking change in changelog.
There was a problem hiding this comment.
@mejroslav So this change does not break internal behavior.
However, I agree this is a public type contract change for consumers, and I’ll mark it as a breaking change and normalize all layers to strings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@asab/library/providers/zookeeper.py`:
- Around line 639-657: The returned LibraryItem.name currently slices using
name=full_path[len(self.BasePath):], which leaves internal scope prefixes (e.g.
/.tenants/... or /.personal/...) in tenant/personal results; change the logic
that computes the public name for LibraryItem (where name=...) to strip the
per-root scope prefix (remove the specific per-root segment like
"/.tenants/<id>" or "/.personal/<id>") rather than only trimming self.BasePath,
so the resulting name is a library-relative path (e.g. "/foo/bar.yaml")
compatible with read/list; update the code that builds name in the LibraryItem
construction (same spot using full_path and self.BasePath) to detect and remove
the per-root prefix when present.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
asab/library/item.py (1)
13-21: Align docstring with actual fields (override semantics, target, size).The Attributes section documents
target(which doesn't exist onLibraryItem), describesoverridewith boolean-like language ("IfTrue") despite it being anintfield, and omits thesizefield entirely. This mismatch confuses users of this public dataclass API.✍️ Proposed docstring fix
- override (int): If `True`, this item is marked as an override for the providers with the same Item name. - target (str): Specifies the target context, e.g., "tenant" or "global". Defaults to "global". + override (int): Non-zero indicates this item overrides providers with the same Item name. + size (Optional[int]): Size in bytes. `None` for directories or unknown size.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@asab/library/providers/zookeeper.py`:
- Around line 642-651: The continuation line supplying the name argument to
LibraryItem in the results.append block is over-indented causing flake8 E126;
locate the results.append(...) that constructs LibraryItem (look for
LibraryItem, name= and self.Layer/target) and re-indent the continuation so the
string concatenation ("/" + full_path[len(root):].lstrip("/")) aligns with the
opening parenthesis or the hanging indent style used in the file (i.e., reduce
its indent to match the other continued arguments), ensuring the block passes
flake8 E126.
- Around line 653-661: The recursive call to _recursive_find is missing the
required keyword-only root argument; update the call in the else branch to pass
root (the same root used in the current invocation) as a keyword, i.e. call
self._recursive_find(full_path, filename, results, target=target, root=root) so
_recursive_find receives its required root parameter and recursion into
subdirectories works correctly.
asab/library/item.py
Outdated
| layer (int): The number of highest layer in which this Item is found. The higher the number, the lower the layer is. | ||
| layers (list[int | str]): Identifiers of layers in which this item was found. | ||
| Values are provider-defined and treated as opaque identifiers. | ||
| Examples: 0, 1, "0:global", "0:tenant", "0:personal". |
There was a problem hiding this comment.
I suggest to provide a list of strings only to avoid additional complexity.
Besides, the example should be like:
["0:personal", "0:tenant", "0:global", 2, 3]
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
asab/library/item.py (1)
13-24:⚠️ Potential issue | 🟠 MajorAlign
layerstype hint with documented/actual values.The docstring and existing providers treat layer identifiers as int or str, but the type annotation now restricts to
List[str]. This is a public API mismatch and will break typed consumers (and conflicts with current provider behavior). Prefer making the typeList[Union[int, str]]or, if you want strings only, convert all providers tostrand update the docstring/examples accordingly.🐛 Proposed fix (align type hint with docstring/examples)
- layers: typing.List[str] + layers: typing.List[typing.Union[int, str]]
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@asab/library/providers/filesystem.py`:
- Around line 154-159: The _list method emits layers as List[int] while
_recursive_find returns List[str], causing mixed types against the
LibraryItem.layers: List[str] contract; update the _list implementation to cast
or format its layers to strings (e.g., using str(self.Layer) or mapping ints to
str) so that LibraryItem(..., layers=[...]) always receives List[str]; touch the
_list function and ensure it uses the same conversion logic as _recursive_find
and references LibraryItem, layers, and self.Layer.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@asab/library/item.py`:
- Line 18: The docstring for the Item field "override" describes boolean
semantics but the code declares override: int = 0; update the declaration to a
boolean and default to False (e.g., override: bool = False) in the Item class,
and adjust any code that relies on numeric values of override to use True/False;
alternatively, if numeric semantics are intended, update the docstring to
explain what the integer represents (e.g., count or priority) and keep override:
int = 0—refer to the Item class and the override field to implement the chosen
change consistently.
🧹 Nitpick comments (1)
asab/library/providers/zookeeper.py (1)
641-651:endswith(filename)can produce false-positive matches.
full_path.endswith(filename)will match any path whose suffix equalsfilename. For example, searching forsetup.yamlwould match both/foo/setup.yamland/foo/my-setup.yaml. Consider checking that the match is preceded by/to ensure an exact filename match:- if full_path.endswith(filename): + if full_path.endswith("/" + filename) or full_path == filename:This may be pre-existing behavior, but worth tightening now that
findis being reworked.
| providers (list): List of `LibraryProvider` objects containing this Item. | ||
| disabled (bool): `True` if the Item is disabled, `False` otherwise. If the Item is disabled, `LibraryService.read(...)` will return `None`. | ||
| favorite (bool): True if the Item is marked as a favorite. | ||
| override (int): If `True`, this item is marked as an override for the providers with the same Item name. |
There was a problem hiding this comment.
Docstring says "If True" but override is typed as int.
The docstring describes boolean semantics ("If True") while the field is override: int = 0. Either change the type to bool or update the doc to describe what the integer value represents (e.g., override count or priority).
🤖 Prompt for AI Agents
In `@asab/library/item.py` at line 18, The docstring for the Item field "override"
describes boolean semantics but the code declares override: int = 0; update the
declaration to a boolean and default to False (e.g., override: bool = False) in
the Item class, and adjust any code that relies on numeric values of override to
use True/False; alternatively, if numeric semantics are intended, update the
docstring to explain what the integer represents (e.g., count or priority) and
keep override: int = 0—refer to the Item class and the override field to
implement the chosen change consistently.
mithunbharadwaj
changed the title
3 participants
Summary by CodeRabbit
New Features
Bug Fixes