Skip to content

fix: correct metadata/version check in version command#2111

Open
rishi-jat wants to merge 9 commits into
oscal-compass:developfrom
rishi-jat:fix/version-metadata-check
Open

fix: correct metadata/version check in version command#2111
rishi-jat wants to merge 9 commits into
oscal-compass:developfrom
rishi-jat:fix/version-metadata-check

Conversation

@rishi-jat

Copy link
Copy Markdown
Contributor

Issue

While reviewing the code, I noticed the boolean condition in version.py:

if not (oscal_object.metadata or oscal_object.metadata.version)

This can raise an AttributeError when metadata is None and does not correctly validate missing version values.

Changes

  • Updated the condition to explicitly check metadata and version separately.
  • Added a unit test to ensure the command raises TrestleError when metadata is None.

Prevents a potential crash and improves validation logic.

Signed-off-by: Rishi Jat <rishijat098@gmail.com>
Copilot AI review requested due to automatic review settings March 3, 2026 17:16
@rishi-jat rishi-jat requested a review from a team as a code owner March 3, 2026 17:16
@rishi-jat

rishi-jat commented Mar 3, 2026

Copy link
Copy Markdown
Contributor Author

/cc @degenaro
/cc @butler54
/cc @jpower432
/cc @vikas-agarwal76

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug in the version command where a metadata/version check could raise AttributeError and fail to properly validate missing version values.

Changes:

  • Corrected the boolean condition in VersionCmd._get_version to safely handle metadata is None.
  • Added a unit test to ensure TrestleError is raised when metadata is None.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
trestle/core/commands/version.py Fixes the metadata/version guard to avoid AttributeError and improve validation behavior.
tests/trestle/core/commands/version_test.py Adds regression coverage for the metadata is None case by asserting TrestleError.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +51 to 52
if not oscal_object.metadata or not oscal_object.metadata.version: # type: ignore
raise TrestleError(f'Unable to determine the version. Metadata version is missing in model: {obj_path}.')

Copilot AI Mar 3, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated condition has two distinct failure modes (metadata is None vs metadata.version missing/falsey). The new test covers metadata is None, but there’s no test covering the case where metadata exists and metadata.version is missing/None/empty. Add a unit test that sets mock_oscal_object.metadata to an object with version = None (and optionally '') and assert TrestleError is raised to fully cover the corrected logic.

Copilot uses AI. Check for mistakes.

@butler54 butler54 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to do some more refactoring here:

TL;DR - i'm not convinced you can hit the code properly anyway. Why?
The top level OSCAL models have metadata as mandatory
e.g.

metadata: common.Metadata

so Metadata, with pydandic enforcement, cannot be none.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants