Skip to content

Fix dynamic_object for key-based classes#177

Merged
amc-corey-cox merged 3 commits intomainfrom
fix-key-based-classes-169
Mar 31, 2026
Merged

Fix dynamic_object for key-based classes#177
amc-corey-cox merged 3 commits intomainfrom
fix-key-based-classes-169

Conversation

@amc-corey-cox
Copy link
Copy Markdown
Contributor

Summary

Test plan

  • New test test_dynamic_object_key_based_class with inline schema using key: true
  • Existing test_dynamic_object still passes (identifier-based)

🤖 Generated with Claude Code

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes dynamic_object() so it can populate identifier fields for inlined dicts when the target class uses key: true (not only identifier: true), avoiding crashes when neither is present. This unblocks transformations involving LinkML-Map’s own key-based datamodel classes.

Changes:

  • Update dynamic_object() to fall back to SchemaView.get_identifier_slot(..., use_key=True) and guard when no id/key slot exists.
  • Add a regression test using an inline schema with a key-based class (key: true).

Reviewed changes

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

File Description
src/linkml_map/utils/dynamic_object.py Adds key-based identifier fallback and avoids None dereference when no id/key slot exists.
tests/test_utils/test_dynamic_object.py Adds coverage for dynamic objects built from dict-keyed collections using key: true.
Comments suppressed due to low confidence (1)

tests/test_utils/test_dynamic_object.py:56

  • The test asserts value for alpha but not for beta, so a regression affecting only subsequent entries could slip through. Consider also asserting dynobj.items["beta"].value == "two" for completeness.
    data = {"items": {"alpha": {"value": "one"}, "beta": {"value": "two"}}}
    dynobj = dynamic_object(data, sv, "Container")
    assert type(dynobj).__name__ == "Container"
    assert isinstance(dynobj.items, dict)
    assert type(dynobj.items["alpha"]).__name__ == "Item"
    assert dynobj.items["alpha"].name == "alpha"
    assert dynobj.items["alpha"].value == "one"
    assert dynobj.items["beta"].name == "beta"

Comment thread src/linkml_map/utils/dynamic_object.py Outdated
…asses

Fixes #169. `get_identifier_slot()` returns None for classes that use
`key: true` instead of `identifier: true`. Now falls back to
`use_key=True` lookup, and guards against None when neither is present.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@amc-corey-cox amc-corey-cox force-pushed the fix-key-based-classes-169 branch from fd4f05c to c7a24fd Compare March 30, 2026 19:57
Add test for classes with neither identifier nor key to cover the
id_slot None guard.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 30, 2026 21:33
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread src/linkml_map/utils/dynamic_object.py Outdated
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@csiege csiege left a comment

Choose a reason for hiding this comment

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

Review: Approve

Clean, well-scoped fix. The production code change is 4 lines and does exactly what it should:

  1. use_key=True — the SchemaView.get_identifier_slot() API already supports falling back to key: true slots; the old call just wasn't using it.
  2. if id_slot is not None: guard — correct behavior when a class has neither identifier nor key (the new NO_ID_SCHEMA test covers exactly this case). The old code would have crashed with AttributeError: 'NoneType' object has no attribute 'name'.

Both new tests are meaningful: one exercises key: true, one exercises the no-id/no-key path. The existing test_dynamic_object continues to cover the identifier: true path.

Our production bdchm schema uses identifier: true on one class, so this bug was not triggered by our pipeline today — but the guard is the right defensive behavior regardless. 7/7 CI checks pass.

@amc-corey-cox amc-corey-cox merged commit e64da19 into main Mar 31, 2026
9 checks passed
@amc-corey-cox amc-corey-cox deleted the fix-key-based-classes-169 branch March 31, 2026 18:11
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.

dynamic_object.py does not handle key-based (vs identifier-based) classes

3 participants