Skip to content

Fix HERD.get_object_entities(attribute=...) to mirror add_ref(attribute=...)#1504

Merged
rly merged 3 commits into
devfrom
fix/herd-get-object-entities-attribute
Jun 24, 2026
Merged

Fix HERD.get_object_entities(attribute=...) to mirror add_ref(attribute=...)#1504
rly merged 3 commits into
devfrom
fix/herd-get-object-entities-attribute

Conversation

@rly

@rly rly commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Motivation

Fixes #1501.

HERD.add_ref(container=..., attribute=...) and HERD.get_object_entities(container=..., attribute=...) resolved the attribute argument differently, so a reference added with an attribute could not be read back with the same attribute:

  • add_ref routes through _validate_object, which for a non-DataType attribute (e.g. DynamicTable.description) stores the parent container under a computed relative_path.
  • get_object_entities instead did container[attribute] with the default relative_path='', which raises KeyError (or TypeError for containers without __getitem__) for non-DataType attributes.

Solution

_validate_object now takes a create flag and threads it to its _check_object_field calls. get_object_entities reuses _validate_object(..., create=False) for the attribute case, so both methods resolve the attribute identically.

The attribute=None path in get_object_entities is unchanged, so directly passing relative_path still works.

How to test the behavior

Added test_get_obj_entities_non_datatype_attribute: adds a ref with attribute='description' and retrieves it with the same attribute. It fails on dev (KeyError: 'description') and passes with this change. The existing test_get_obj_entities_attribute (DataType-attribute / column case) continues to pass.

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This will be checked during CI.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number?

🤖 Generated with Claude Code

get_object_entities resolved the attribute argument with container[attribute]
and a default relative_path, while add_ref routes through _validate_object,
which stores non-DataType attributes under a computed relative_path. As a
result a reference added with an attribute could not be retrieved with the
same attribute (KeyError/TypeError for non-DataType attributes).

_validate_object now takes a create flag, and get_object_entities reuses it
with create=False so both methods resolve the attribute identically.

Add a regression test for the non-DataType attribute case. Fixes #1501.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.24%. Comparing base (54dbf42) to head (2f2a319).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #1504   +/-   ##
=======================================
  Coverage   93.24%   93.24%           
=======================================
  Files          41       41           
  Lines       10217    10217           
  Branches     2107     2107           
=======================================
  Hits         9527     9527           
  Misses        415      415           
  Partials      275      275           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rly rly requested a review from oruebel June 23, 2026 08:08
Comment thread src/hdmf/common/resources.py Outdated
Comment thread src/hdmf/common/resources.py Outdated
Comment thread src/hdmf/common/resources.py
Comment thread src/hdmf/common/resources.py
Comment thread src/hdmf/common/resources.py Outdated

@oruebel oruebel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I added some suggestions to help clarify the usage of _check_object_field and fix what I believe is a bug in get_key

rly and others added 2 commits June 24, 2026 10:59
…tities-attribute

# Conflicts:
#	CHANGELOG.md
…_key

Apply oruebel's review feedback on #1504:
- Flip the `create` default to False on `_check_object_field` and
  `_validate_object`; only `add_ref` passes `create=True`. This fixes
  `get_key`, which previously relied on the implicit create and would
  fail on the missing `idx` of a not-yet-added object; it now raises a
  clear "Object not in Object Table." ValueError.
- Add docstrings to `_validate_object` and a `doc` to the `create`
  docval argument, and rewrite the `_check_object_field` docstring to
  describe the actual return/raise behavior.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rly rly merged commit c5d62f1 into dev Jun 24, 2026
22 of 28 checks passed
@rly rly deleted the fix/herd-get-object-entities-attribute branch June 24, 2026 19:39
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.

HERD.get_object_entities(attribute=...) is asymmetric with add_ref(attribute=...) and fails for non-DataType attributes

2 participants