Just catch missing property exception everywhere in code that calculates values to display#1273
Conversation
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
3 times, most recently
from
aee9945 to
dff6f27
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConsolidates unknown-value rendering (removes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
🧹 Nitpick comments (2)
src/stratis_cli/_actions/_list_filesystem.py (1)
281-288: Consider guardingMergeScheduled()independently.If
Originis present butMergeScheduledis independently missing, the exception will be caught by the outer try/except, but the entire snapshot origin section will display as unknown. IfMergeScheduledcan be missing whileOriginis present, consider guarding it separately for a more granular fallback.♻️ Optional: Guard MergeScheduled independently
try: origin = get_property(fs.Origin(), self.uuid_formatter, None) print(f"Snapshot origin: {origin}") if origin is not None: - scheduled = "Yes" if fs.MergeScheduled() else "No" + try: + scheduled = "Yes" if fs.MergeScheduled() else "No" + except DbusClientMissingPropertyError: # pragma: no cover + scheduled = TABLE_UNKNOWN_STRING print(f" Revert scheduled: {scheduled}") except DbusClientMissingPropertyError: # pragma: no cover print(f"Snapshot origin: {TABLE_UNKNOWN_STRING}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stratis_cli/_actions/_list_filesystem.py` around lines 281 - 288, The current try/except around retrieving Origin and calling fs.MergeScheduled() causes a missing MergeScheduled property to hide a present Origin; change the logic in the snapshot printing block to fetch Origin first (using get_property(fs.Origin(), self.uuid_formatter, None) as currently) and print it, then call fs.MergeScheduled() inside its own small try/except that catches DbusClientMissingPropertyError and sets scheduled to TABLE_UNKNOWN_STRING (or "No"/"Yes" when available); reference get_property, fs.Origin(), fs.MergeScheduled(), DbusClientMissingPropertyError, and TABLE_UNKNOWN_STRING when making this change so Origin is always shown even if MergeScheduled is missing.src/stratis_cli/_actions/_list_pool.py (1)
316-327: Consider adding exception handling for consistency.The
uuid_strandname_strhelpers inDefaultdon't guard againstDbusClientMissingPropertyError, unlike the analogous helpers in_physical.pyand_list_filesystem.py. If the PR's goal is to "catch missing property exception everywhere," these should follow the same defensive pattern.♻️ Proposed fix to add exception handling
+# isort: FIRSTPARTY +from dbus_client_gen import DbusClientMissingPropertyError + +from ._formatting import ( + TABLE_UNKNOWN_STRING, ... def uuid_str(self, mopool: Any) -> str: """ Return a string representation of UUID, correctly formatted. """ - return self.uuid_formatter(mopool.Uuid()) + try: + return self.uuid_formatter(mopool.Uuid()) + except DbusClientMissingPropertyError: # pragma: no cover + return TABLE_UNKNOWN_STRING `@staticmethod` def name_str(mopool: Any) -> str: """ Return a string representation of the pool name. """ - return mopool.Name() + try: + return mopool.Name() + except DbusClientMissingPropertyError: # pragma: no cover + return TABLE_UNKNOWN_STRING🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stratis_cli/_actions/_list_pool.py` around lines 316 - 327, The uuid_str and name_str methods (in Default) need the same defensive try/except used elsewhere: wrap the mopool.Uuid() and mopool.Name() calls in a try block and catch DbusClientMissingPropertyError, returning a safe fallback (e.g. empty string) when the property is missing; ensure you import or reference DbusClientMissingPropertyError the same way as in _physical.py and _list_filesystem.py so behavior is consistent across helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stratis_cli/_actions/_utils.py`:
- Around line 298-306: In the free() method the check uses self.total (the
method/property reference) instead of the backing attribute, causing the None
check to always fail; change the conditional to test self._total (and/or call
self.total() only if total is a method) so the guard reads "if self._used is
None or self._total is None" and then return self._total - self._used; update
only the free() function's conditional to reference the backing attribute _total
to avoid TypeError when _total is None.
---
Nitpick comments:
In `@src/stratis_cli/_actions/_list_filesystem.py`:
- Around line 281-288: The current try/except around retrieving Origin and
calling fs.MergeScheduled() causes a missing MergeScheduled property to hide a
present Origin; change the logic in the snapshot printing block to fetch Origin
first (using get_property(fs.Origin(), self.uuid_formatter, None) as currently)
and print it, then call fs.MergeScheduled() inside its own small try/except that
catches DbusClientMissingPropertyError and sets scheduled to
TABLE_UNKNOWN_STRING (or "No"/"Yes" when available); reference get_property,
fs.Origin(), fs.MergeScheduled(), DbusClientMissingPropertyError, and
TABLE_UNKNOWN_STRING when making this change so Origin is always shown even if
MergeScheduled is missing.
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 316-327: The uuid_str and name_str methods (in Default) need the
same defensive try/except used elsewhere: wrap the mopool.Uuid() and
mopool.Name() calls in a try block and catch DbusClientMissingPropertyError,
returning a safe fallback (e.g. empty string) when the property is missing;
ensure you import or reference DbusClientMissingPropertyError the same way as in
_physical.py and _list_filesystem.py so behavior is consistent across helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42da799e-4a8f-4bc7-9300-26fddd50cef8
📒 Files selected for processing (5)
src/stratis_cli/_actions/_formatting.pysrc/stratis_cli/_actions/_list_filesystem.pysrc/stratis_cli/_actions/_list_pool.pysrc/stratis_cli/_actions/_physical.pysrc/stratis_cli/_actions/_utils.py
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
2 times, most recently
from
b9034cb to
8ae6b1c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/stratis_cli/_actions/_list_pool.py (1)
444-450: Consider extractingsize_stras a shared utility.The
size_strhelper function is duplicated in bothDefaultDetail._print_detail_viewandDetail.display(in_list_filesystem.py). This could be extracted to a shared location, but given the small scope, this is a minor optional improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stratis_cli/_actions/_list_pool.py` around lines 444 - 450, The helper size_str used to format Range | None is duplicated between DefaultDetail._print_detail_view and Detail.display; extract it into a shared utility (e.g., add a small function size_str(range: Range | None) -> str in a common module or a new util module) and replace both local definitions to import and call that single function, ensuring the signature and TABLE_UNKNOWN_STRING reference are preserved and updating imports in _list_pool.py and _list_filesystem.py to use the shared size_str.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stratis_cli/_actions/_list_filesystem.py`:
- Around line 142-150: The return type annotation for limit_str is wrong: it
never returns None, so update the signature of limit_str to return str (replace
"def limit_str(mofs: Any) -> str | None:" with "def limit_str(mofs: Any) ->
str:") and keep the body as-is (which returns str(get_property(...)) or
TABLE_UNKNOWN_STRING); verify references to get_property, SizeLimit(),
TABLE_UNKNOWN_STRING, and DbusClientMissingPropertyError remain unchanged.
---
Nitpick comments:
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 444-450: The helper size_str used to format Range | None is
duplicated between DefaultDetail._print_detail_view and Detail.display; extract
it into a shared utility (e.g., add a small function size_str(range: Range |
None) -> str in a common module or a new util module) and replace both local
definitions to import and call that single function, ensuring the signature and
TABLE_UNKNOWN_STRING reference are preserved and updating imports in
_list_pool.py and _list_filesystem.py to use the shared size_str.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3f25a1e-eba5-4a15-82e0-f587e888381b
📒 Files selected for processing (4)
src/stratis_cli/_actions/_list_filesystem.pysrc/stratis_cli/_actions/_list_pool.pysrc/stratis_cli/_actions/_physical.pysrc/stratis_cli/_actions/_utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/stratis_cli/_actions/_physical.py
- src/stratis_cli/_actions/_utils.py
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
from
8ae6b1c to
94a24fc
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stratis_cli/_actions/_list_pool.py (1)
272-308:⚠️ Potential issue | 🟠 MajorGuard alert generation against missing D-Bus properties.
The
alert_codes()method calls multiple property accessors without error handling:AvailableActions()(line 282),NoAllocSpace()(line 285),Encrypted()(lines 296, 301), and_volume_key_loaded()which internally callsVolumeKeyLoaded(). If any of these raiseDbusClientMissingPropertyError, the exception propagates and breaks pool detail and pool table rendering. This inconsistency is problematic because the same file'spool_properties(),size_triple(),uuid_str(), andname_str()methods all wrap their property accesses in try-except blocks forDbusClientMissingPropertyErrorwith sensible fallbacks.Wrap each property access in
alert_codes()with try-except handling forDbusClientMissingPropertyError, using appropriate defaults (empty lists for alerts, False for boolean states). Also add a regression test that forces these getters to raiseDbusClientMissingPropertyErrorto verify the fallback behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stratis_cli/_actions/_list_pool.py` around lines 272 - 308, The alert_codes function calls mopool.AvailableActions(), mopool.NoAllocSpace(), mopool.Encrypted(), and Default._volume_key_loaded() (which calls VolumeKeyLoaded()) without guarding for DbusClientMissingPropertyError; wrap each of these calls in try/except DbusClientMissingPropertyError and use safe defaults (treat missing AvailableActions as no availability alerts, missing NoAllocSpace as False, missing Encrypted as False, and missing VolumeKeyLoaded as False) so availability_alerts, no_alloc_space_alerts and pool_encryption_alerts are built from those defaults; update the PoolEncryptionAlert construction to operate correctly when Encrypted or volume_key_loaded are the fallback False values; and add a regression test that mocks/patches mopool property getters to raise DbusClientMissingPropertyError and asserts alert_codes() returns the expected empty/default alert lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stratis_cli/_actions/_list_filesystem.py`:
- Around line 181-190: The dict comprehension that builds
pool_object_path_to_pool_name in list_filesystems() can abort if
MOPool(info).Name() raises DbusClientMissingPropertyError; wrap the call to
MOPool(info).Name() in a try/except that catches DbusClientMissingPropertyError
and uses TABLE_UNKNOWN_STRING as the value so the cache is always populated,
keeping pool_name_str()'s fallback consistent; update the cache-building code in
list_filesystems() (referencing MOPool.Name(), TABLE_UNKNOWN_STRING and
pool_object_path_to_pool_name) and add a regression test that mocks
MOPool.Name() to raise DbusClientMissingPropertyError and verifies the
unknown-string entry and end-to-end behavior.
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 422-423: The print call when encrypted is None is using a literal
placeholder instead of interpolating TABLE_UNKNOWN_STRING; update the branch in
_list_pool.py (the block checking "if encrypted is None") to interpolate the
value (e.g., make the string an f-string or use .format) so the printed line
shows the actual TABLE_UNKNOWN_STRING value rather than
"{TABLE_UNKNOWN_STRING}"—change the print in that branch where encrypted is None
to print the interpolated value.
---
Outside diff comments:
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 272-308: The alert_codes function calls mopool.AvailableActions(),
mopool.NoAllocSpace(), mopool.Encrypted(), and Default._volume_key_loaded()
(which calls VolumeKeyLoaded()) without guarding for
DbusClientMissingPropertyError; wrap each of these calls in try/except
DbusClientMissingPropertyError and use safe defaults (treat missing
AvailableActions as no availability alerts, missing NoAllocSpace as False,
missing Encrypted as False, and missing VolumeKeyLoaded as False) so
availability_alerts, no_alloc_space_alerts and pool_encryption_alerts are built
from those defaults; update the PoolEncryptionAlert construction to operate
correctly when Encrypted or volume_key_loaded are the fallback False values; and
add a regression test that mocks/patches mopool property getters to raise
DbusClientMissingPropertyError and asserts alert_codes() returns the expected
empty/default alert lists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3520d4d7-2180-41d4-a0b2-958d5c0e2f11
📒 Files selected for processing (2)
src/stratis_cli/_actions/_list_filesystem.pysrc/stratis_cli/_actions/_list_pool.py
| def pool_name_str(self, mofs: Any) -> str: | ||
| """ | ||
| Return the name of this filesystem's pool. | ||
| """ | ||
| try: | ||
| return self.pool_object_path_to_pool_name.get( | ||
| mofs.Pool(), TABLE_UNKNOWN_STRING | ||
| ) | ||
| except DbusClientMissingPropertyError: # pragma: no cover | ||
| return TABLE_UNKNOWN_STRING |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and understand its structure
find . -name "_list_filesystem.py" -type fRepository: stratis-storage/stratis-cli
Length of output: 117
🏁 Script executed:
# Let's check the content around the mentioned lines and find the list_filesystems function
cat -n src/stratis_cli/_actions/_list_filesystem.py | head -200Repository: stratis-storage/stratis-cli
Length of output: 7300
🏁 Script executed:
# Search for list_filesystems function definition
rg "def list_filesystems" src/stratis_cli/_actions/_list_filesystem.py -A 50Repository: stratis-storage/stratis-cli
Length of output: 1641
🏁 Script executed:
# Search for pool_object_path_to_pool_name initialization
rg "pool_object_path_to_pool_name" src/stratis_cli/_actions/_list_filesystem.py -B 5 -A 5Repository: stratis-storage/stratis-cli
Length of output: 1818
🏁 Script executed:
# Check what exception MOPool.Name() can raise
rg "class MOPool" . -A 20Repository: stratis-storage/stratis-cli
Length of output: 53
🏁 Script executed:
# Look for DbusClientMissingPropertyError to understand where it's used
rg "DbusClientMissingPropertyError" src/stratis_cli/_actions/_list_filesystem.py -B 2 -A 2Repository: stratis-storage/stratis-cli
Length of output: 1952
Cache population in list_filesystems() lacks error handling for missing pool names.
The pool_name_str() method provides a fallback when the pool object path is unavailable, but this fallback never executes if MOPool(info).Name() raises DbusClientMissingPropertyError during cache initialization (lines 70-73). The dict comprehension aborts the entire command before the method is invoked.
This is inconsistent with the class's established error-handling pattern—all other property accessors (name_str(), limit_str(), devnode_str(), size_triple()) wrap their calls in try-except blocks to gracefully handle missing properties.
Wrap MOPool(info).Name() in a try-except block (as suggested) to populate missing pool names with TABLE_UNKNOWN_STRING. Additionally, add a regression test that makes MOPool.Name() raise DbusClientMissingPropertyError to ensure the fallback works end-to-end.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stratis_cli/_actions/_list_filesystem.py` around lines 181 - 190, The
dict comprehension that builds pool_object_path_to_pool_name in
list_filesystems() can abort if MOPool(info).Name() raises
DbusClientMissingPropertyError; wrap the call to MOPool(info).Name() in a
try/except that catches DbusClientMissingPropertyError and uses
TABLE_UNKNOWN_STRING as the value so the cache is always populated, keeping
pool_name_str()'s fallback consistent; update the cache-building code in
list_filesystems() (referencing MOPool.Name(), TABLE_UNKNOWN_STRING and
pool_object_path_to_pool_name) and add a regression test that mocks
MOPool.Name() to raise DbusClientMissingPropertyError and verifies the
unknown-string entry and end-to-end behavior.
There was a problem hiding this comment.
@coderabbitai How would you suggest I construct that dict, then?
There was a problem hiding this comment.
@mulkieran Since you can't use try/except directly inside a generator expression or dict comprehension, the two cleanest options are:
Option 1 — plain for loop (most readable, consistent with the PR's pattern):
pool_object_path_to_pool_name = {}
for path, info in pools(props=props).search(managed_objects):
try:
pool_object_path_to_pool_name[path] = MOPool(info).Name()
except DbusClientMissingPropertyError: # pragma: no cover
pool_object_path_to_pool_name[path] = TABLE_UNKNOWN_STRINGOption 2 — local helper function (keeps a dict-literal style):
def _pool_name(info):
try:
return MOPool(info).Name()
except DbusClientMissingPropertyError: # pragma: no cover
return TABLE_UNKNOWN_STRING
pool_object_path_to_pool_name = {
path: _pool_name(info)
for path, info in pools(props=props).search(managed_objects)
}I'd lean toward Option 1 — it's the most straightforward and mirrors the try/except style already used throughout size_triple(), limit_str(), etc. in this same PR. Option 2 is fine too if you prefer keeping the dict-comprehension form.
There was a problem hiding this comment.
@coderabbitai Thanks, I'll pass on that one. This PR is just trying to harden the actual listing code, and the probability that a pool object path doesn't have its own Name property is very low.
There was a problem hiding this comment.
@mulkieran, that's a fair call — the PR's focus on hardening the display/listing helpers is clear, and a pool always having its Name property present at the cache-building stage is a very reasonable assumption. Thanks for the explanation!
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
from
94a24fc to
f762752
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/stratis_cli/_actions/_list_pool.py (2)
489-494: Minor: preferif not encryption_infosoverif encryption_infos == [].A more Pythonic empty-list check:
- if encryption_infos == []: + if not encryption_infos:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stratis_cli/_actions/_list_pool.py` around lines 489 - 494, Replace the explicit empty-list equality check "if encryption_infos == []:" with a truthiness check "if not encryption_infos:" in the block handling encryption_infos (the branch that prints "No token slot information available" vs iterating and printing str(info)). Update the conditional around the encryption_infos variable to use Pythonic truthiness rather than equality.
402-426: Consider extracting a helper for repeated try/except pattern.The file has many nearly identical try/except blocks following this pattern:
try: value = some_expression except DbusClientMissingPropertyError: # pragma: no cover value = fallbackWhile the current explicit approach is readable, a helper like
def get_or_unknown(func, default=TABLE_UNKNOWN_STRING)could reduce boilerplate. This is optional given the clarity benefits of the current approach.♻️ Optional helper pattern
def _get_property_or_default(getter: Callable[[], T], default: T) -> T: """Safely get a D-Bus property, returning default if missing.""" try: return getter() except DbusClientMissingPropertyError: # pragma: no cover return defaultUsage:
cache_str = _get_property_or_default( lambda: "Yes" if mopool.HasCache() else "No", TABLE_UNKNOWN_STRING )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stratis_cli/_actions/_list_pool.py` around lines 402 - 426, There are many repeated try/except blocks that catch DbusClientMissingPropertyError and return TABLE_UNKNOWN_STRING; extract a small helper like _get_property_or_default(getter, default=TABLE_UNKNOWN_STRING) and replace each repeated pattern (e.g., the calls that compute actions_allowed_str, cache_str, fs_limit_str, allow_overprovisioning_str using mopool.AvailableActions(), mopool.HasCache(), mopool.FsLimit(), mopool.Overprovisioning()) with calls to that helper passing a lambda for the getter and TABLE_UNKNOWN_STRING as the default to remove boilerplate and centralize the exception handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 626-631: The props_list entry for metadata currently uses
(metadata_version in (MetadataVersion.V1, None)) which treats None as known and
prints "Le"; update that expression in the _list_pool logic so it only evaluates
True for an explicit MetadataVersion.V1 (e.g. metadata_version ==
MetadataVersion.V1) so that metadata_version == None is treated as unknown and
will render "?" like the other properties; adjust the tuple entry in props_list
(the one referencing MetadataVersion.V1) accordingly and add a brief inline
comment if you prefer to document the decision.
---
Nitpick comments:
In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 489-494: Replace the explicit empty-list equality check "if
encryption_infos == []:" with a truthiness check "if not encryption_infos:" in
the block handling encryption_infos (the branch that prints "No token slot
information available" vs iterating and printing str(info)). Update the
conditional around the encryption_infos variable to use Pythonic truthiness
rather than equality.
- Around line 402-426: There are many repeated try/except blocks that catch
DbusClientMissingPropertyError and return TABLE_UNKNOWN_STRING; extract a small
helper like _get_property_or_default(getter, default=TABLE_UNKNOWN_STRING) and
replace each repeated pattern (e.g., the calls that compute actions_allowed_str,
cache_str, fs_limit_str, allow_overprovisioning_str using
mopool.AvailableActions(), mopool.HasCache(), mopool.FsLimit(),
mopool.Overprovisioning()) with calls to that helper passing a lambda for the
getter and TABLE_UNKNOWN_STRING as the default to remove boilerplate and
centralize the exception handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c256becf-4d3a-4c2c-8df4-8a2e3bd21db1
📒 Files selected for processing (1)
src/stratis_cli/_actions/_list_pool.py
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
4 times, most recently
from
aed4123 to
e51eb59
Compare
|
@coderabbitai review |
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
6 times, most recently
from
39b7eb4 to
253a63c
Compare
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
It's improved over the legacy List which should be dropped anyway. In particular, it seems to be able to handle variance in the types of the elements in ths list a bit better. Signed-off-by: mulhern <amulhern@redhat.com>
Systematically test by simulating each one raising an exception. Signed-off-by: mulhern <amulhern@redhat.com>
mulkieran
force-pushed
the
just-catch-missing-property-exception-everywhere
branch
from
253a63c to
7c389cd
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/logical/test_list.py`:
- Around line 269-290: Split the property-mocking loop into two runs: one for
pool-scoped table cases (options lists that include self._POOLNAME) and one for
the all-pools table case (options without a pool argument). For the pool-scoped
run, exclude only "Pool" from the spec.findall(...) list so "Name" and "Uuid"
fallback render paths get exercised; for the all-pools run, exclude "Name" and
"Uuid" but include "Pool" so the Pool() path is covered. Locate the loop using
spec.findall("./property"), the patched target
stratis_cli._actions._data.MOFilesystem, and the options lists
([self._POOLNAME], [self._POOLNAME, f"--name=..."], and the no-pool case) and
apply the appropriate exclusion set for each run.
In `@tests/integration/physical/test_list.py`:
- Around line 138-157: The test currently skips exercising the "Pool" property
and thus never runs the all-pools code path; modify the loop around mocking
MODev properties (MODev and Db...DbusClientMissingPropertyError usage) so that
when property_name == "Pool" you also run a companion subtest that calls
TEST_RUNNER(self._MENU) (i.e., invoke blockdev list with no positional pool
argument) in addition to the existing TEST_RUNNER(self._MENU +
[self._POOLNAME]); keep the same patch context and subTest wrapper so the mock
and side_effect remain applied for the Pool case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 403341e9-1dbe-4f62-acdd-45955d1f2e7d
📒 Files selected for processing (6)
src/stratis_cli/_actions/_list_filesystem.pysrc/stratis_cli/_actions/_list_pool.pysrc/stratis_cli/_actions/_physical.pytests/integration/logical/test_list.pytests/integration/physical/test_list.pytests/integration/pool/test_list.py
✅ Files skipped from review due to trivial changes (1)
- src/stratis_cli/_actions/_physical.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/stratis_cli/_actions/_list_filesystem.py
- src/stratis_cli/_actions/_list_pool.py
| for property_name in [ | ||
| prop.attrib["name"] | ||
| for prop in spec.findall("./property") | ||
| if not (prop.attrib["name"] in ("Pool", "Name", "Uuid")) | ||
| ]: | ||
| with patch.object( | ||
| # pylint: disable=protected-access | ||
| stratis_cli._actions._data.MOFilesystem, # pyright: ignore | ||
| property_name, | ||
| autospec=True, | ||
| side_effect=DbusClientMissingPropertyError( | ||
| "oops", | ||
| stratis_cli._actions._constants.FILESYSTEM_INTERFACE, # pyright: ignore | ||
| property_name, | ||
| ), | ||
| ): | ||
| for options in [ | ||
| [self._POOLNAME], | ||
| [self._POOLNAME, f"--name={self._VOLUMES[0]}"], | ||
| [self._POOLNAME, f"--name={self._VOLUMES[1]}"], | ||
| [self._POOLNAME, f"--name={self._VOLUMES[2]}"], | ||
| ]: |
There was a problem hiding this comment.
The exclusion list skips some of the fallback paths this PR is adding.
Name and Uuid do not need to be excluded for the [self._POOLNAME] table case, so those missing-property render paths never run here. Pool is also left uncovered because there is no companion filesystem list case without a pool argument. Splitting the option sets would let you keep the lookup-sensitive detail cases while still testing Name/Uuid in the pool-scoped table view and Pool() in the all-pools view.
🧰 Tools
🪛 Ruff (0.15.7)
[error] 272-272: Test for membership should be not in
Convert to not in
(E713)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/logical/test_list.py` around lines 269 - 290, Split the
property-mocking loop into two runs: one for pool-scoped table cases (options
lists that include self._POOLNAME) and one for the all-pools table case (options
without a pool argument). For the pool-scoped run, exclude only "Pool" from the
spec.findall(...) list so "Name" and "Uuid" fallback render paths get exercised;
for the all-pools run, exclude "Name" and "Uuid" but include "Pool" so the
Pool() path is covered. Locate the loop using spec.findall("./property"), the
patched target stratis_cli._actions._data.MOFilesystem, and the options lists
([self._POOLNAME], [self._POOLNAME, f"--name=..."], and the no-pool case) and
apply the appropriate exclusion set for each run.
There was a problem hiding this comment.
That will trigger failures on a code path that precedes the actual display, so I'm not doing that for this PR.
There was a problem hiding this comment.
@mulkieran, understood — Pool, Name, and Uuid are used by the lookup/routing logic before the display code is ever reached, so patching them to raise DbusClientMissingPropertyError would produce failures in that pre-display path rather than exercising the display fallbacks this PR is adding. The current exclusion list is the right call.
✏️ Learnings added
Learnt from: mulkieran
Repo: stratis-storage/stratis-cli PR: 1273
File: tests/integration/logical/test_list.py:269-290
Timestamp: 2026-03-30T16:08:17.821Z
Learning: In `tests/integration/logical/test_list.py`, the `Pool`, `Name`, and `Uuid` properties of `MOFilesystem` must be excluded from the `DbusClientMissingPropertyError`-patching loop in `List6TestCase.test_dropping_properties`. These properties are used by the lookup/routing code path that runs *before* the display logic, so patching them to raise `DbusClientMissingPropertyError` triggers failures in the pre-display path rather than exercising the missing-property display fallbacks that the test is designed to cover.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mulkieran
Repo: stratis-storage/stratis-cli PR: 1273
File: tests/integration/pool/test_list.py:362-383
Timestamp: 2026-03-27T20:54:28.782Z
Learning: In `tests/integration/pool/test_list.py`, patching `MOPool.MetadataVersion` to return a controlled value (e.g. `str(MetadataVersion.V1)` or `str(MetadataVersion.V2)`) while simultaneously patching other `MOPool` properties to raise `DbusClientMissingPropertyError` causes false-positive test failures. The author (mulkieran) confirmed this is due to patch interaction side effects. The approach was deliberately dropped; V2 coverage relies on the pool's natural metadata version instead.
Learnt from: mulkieran
Repo: stratis-storage/stratis-cli PR: 1251
File: src/stratis_cli/_error_reporting.py:288-290
Timestamp: 2026-01-07T17:36:05.159Z
Learning: In src/stratis_cli/_error_reporting.py, the zbus.Error handling path for DPClientSetPropertyContext (around line 288-290) is now covered by tests because stratisd changed its behavior to return org.freedesktop.zbus.Error for property setting operations.
| for property_name in [ | ||
| prop.attrib["name"] | ||
| for prop in spec.findall("./property") | ||
| if prop.attrib["name"] != "Pool" | ||
| ]: | ||
| with patch.object( | ||
| # pylint: disable=protected-access | ||
| stratis_cli._actions._data.MODev, # pyright: ignore | ||
| property_name, | ||
| autospec=True, | ||
| side_effect=DbusClientMissingPropertyError( | ||
| "oops", | ||
| stratis_cli._actions._constants.BLOCKDEV_INTERFACE, # pyright: ignore | ||
| property_name, | ||
| ), | ||
| ): | ||
| with self.subTest( | ||
| property_name=property_name, | ||
| ): | ||
| TEST_RUNNER(self._MENU + [self._POOLNAME]) |
There was a problem hiding this comment.
Pool() never gets exercised by this test.
Every subtest uses blockdev list <pool>, so skipping "Pool" leaves the all-pools table path untested when MODev.Pool() is missing. Add one companion blockdev list case without the positional pool argument for that property.
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 157-157: Consider [*self._MENU, self._POOLNAME] instead of concatenation
Replace with [*self._MENU, self._POOLNAME]
(RUF005)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/physical/test_list.py` around lines 138 - 157, The test
currently skips exercising the "Pool" property and thus never runs the all-pools
code path; modify the loop around mocking MODev properties (MODev and
Db...DbusClientMissingPropertyError usage) so that when property_name == "Pool"
you also run a companion subtest that calls TEST_RUNNER(self._MENU) (i.e.,
invoke blockdev list with no positional pool argument) in addition to the
existing TEST_RUNNER(self._MENU + [self._POOLNAME]); keep the same patch context
and subTest wrapper so the mock and side_effect remain applied for the Pool
case.
There was a problem hiding this comment.
@mulkieran, understood — the same reasoning applies here: MODev.Pool() is consumed by the pool-lookup routing code that runs before the display logic, so patching it to raise DbusClientMissingPropertyError would trigger a failure in the wrong path rather than exercising the missing-property display fallbacks the test is designed to cover. My suggestion was incorrect. Thanks for the clarification!
✏️ Learnings added
Learnt from: mulkieran
Repo: stratis-storage/stratis-cli PR: 1273
File: tests/integration/physical/test_list.py:138-157
Timestamp: 2026-03-30T16:12:29.167Z
Learning: In `tests/integration/physical/test_list.py`, the `Pool` property of `MODev` must be excluded from the `DbusClientMissingPropertyError`-patching loop in `List3TestCase.test_dropping_properties`. `MODev.Pool()` is used by the pool-lookup/routing code path that runs *before* the display logic, so patching it to raise `DbusClientMissingPropertyError` causes failures in the pre-display path rather than exercising the missing-property display fallbacks that the test is designed to cover.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mulkieran
Repo: stratis-storage/stratis-cli PR: 1273
File: tests/integration/logical/test_list.py:269-290
Timestamp: 2026-03-30T16:08:17.821Z
Learning: In `tests/integration/logical/test_list.py`, the `Pool`, `Name`, and `Uuid` properties of `MOFilesystem` must be excluded from the `DbusClientMissingPropertyError`-patching loop in `List6TestCase.test_dropping_properties`. These properties are used by the lookup/routing code path that runs *before* the display logic, so patching them to raise `DbusClientMissingPropertyError` triggers failures in the pre-display path rather than exercising the missing-property display fallbacks that the test is designed to cover.
Learnt from: mulkieran
Repo: stratis-storage/stratis-cli PR: 1251
File: src/stratis_cli/_error_reporting.py:288-290
Timestamp: 2026-01-07T17:36:05.159Z
Learning: In src/stratis_cli/_error_reporting.py, the zbus.Error handling path for DPClientSetPropertyContext (around line 288-290) is now covered by tests because stratisd changed its behavior to return org.freedesktop.zbus.Error for property setting operations.
Learnt from: mulkieran
Repo: stratis-storage/stratis-cli PR: 1273
File: tests/integration/pool/test_list.py:362-383
Timestamp: 2026-03-27T20:54:28.782Z
Learning: In `tests/integration/pool/test_list.py`, patching `MOPool.MetadataVersion` to return a controlled value (e.g. `str(MetadataVersion.V1)` or `str(MetadataVersion.V2)`) while simultaneously patching other `MOPool` properties to raise `DbusClientMissingPropertyError` causes false-positive test failures. The author (mulkieran) confirmed this is due to patch interaction side effects. The approach was deliberately dropped; V2 coverage relies on the pool's natural metadata version instead.
|
Without the change: With the change: with Created prop arbitrarily set to return an error. |
2 participants
Catch any DbusClientMissingPropertyError exceptions that might be generated while calculating values to display for any of the listing functionality.
Systematically test all paths that are followed when one property goes missing from the D-Bus in the display code, except for, in some cases, Pool, Uuid, and Name properties which are used not just for display but also for locating specific object paths for filesystems and blockdevs when the listing is restricted to a subset of the available filesystems or blockdevs. Do not address the possible failure of the object path lookup code in this PR. The three properties, Pool, Uuid, and Name are fundamental ones, and will not be missing unless there is a bug in stratisd. Other properties, like size-related ones especially, may be absent because it is not possible to obtain them for system reasons. This PR prevents the absence of these properties from causing a stack trace.
Do not add in any extra method that allow a default value to be specified on DbusClientMissingPropertyError. These method only increase the complexity, because it becomes necessary in some cases to specify a default that then becomes a value that is used in a branch. Might as well just use the branch already available in the try-except block; this approach should have slightly better performance because it avoids extra function call overhead.