Skip to content

fix: give CompiledSearchTerm a None related_terms default - #354

Open
Kevin Turcios (KRRT7) wants to merge 1 commit into
microsoft:mainfrom
KRRT7:fix-compiled-search-term-default
Open

Kevin Turcios (KRRT7) wants to merge 1 commit into
microsoft:mainfrom
KRRT7:fix-compiled-search-term-default

Conversation

@KRRT7

Copy link
Copy Markdown
Contributor

Summary

  • CompiledSearchTerm was a stdlib @dataclass subclassing the pydantic dataclass SearchTerm. A stdlib dataclass takes the base class's class-level value as the field default, and for related_terms that value is a pydantic FieldInfo object, not None.
    • CompiledSearchTerm(Term("x")).related_terms was therefore a truthy FieldInfo, and dedupe_related_terms crashed on it with AttributeError: 'FieldInfo' object has no attribute 'text'.
    • Production code only creates CompiledSearchTerm via cast, so it isn't hit there today; tests work around it by always passing related_terms=None.
  • Declare CompiledSearchTerm with typeagent's pydantic dataclass decorator, like SearchTerm (same import as universal_message.py).
  • A scan of all 103 dataclasses in the package found no other non-pydantic dataclass inheriting a FieldInfo default.

Relation to #348: #348 changes this default to Field(default=None, ...), which is still a FieldInfo, so the bug remains there. This PR doesn't touch any file #348 changes; it applies cleanly on top of #348 and the affected tests pass there too.

Validation

  • New tests: a default-constructed CompiledSearchTerm has related_terms is None, and dedupe_related_terms accepts such terms. Both fail without the fix.
  • make format check test

CompiledSearchTerm was a stdlib dataclass subclassing the pydantic
dataclass SearchTerm, so it took SearchTerm's pydantic Field object as the
default of related_terms instead of None. CompiledSearchTerm(Term("x"))
therefore had a truthy, non-list related_terms, which crashed
dedupe_related_terms. Declare it with the same pydantic dataclass
decorator as SearchTerm.

Co-authored-by: wtfyd-agent[bot] <328989958+wtfyd-agent[bot]@users.noreply.github.com>

@bmerkle Bernhard Merkle (bmerkle) 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.

LGTM — verified locally.

  • Confirmed the root cause: a stdlib @dataclass subclass picks up SearchTerm's class-level FieldInfo as the related_terms default; both new tests fail against main's query.py and pass with the fix. Full make is green (813 passed).
  • Checked the pydantic-dataclass semantics this switches to: term keeps its identity (important, since search mutates terms in place), the cast-based to_required_search_term path is unaffected, bad input now raises ValidationError, and post-construction assignment stays unvalidated, all the same as SearchTerm.
  • FYI: a related_terms list passed to the constructor is now copied (elements keep identity), matching SearchTerm; nothing in src//tools/ constructs CompiledSearchTerm directly, so no impact.

(An equally small alternative would have been keeping the stdlib dataclass and redeclaring related_terms: list[Term] | None = None in the subclass, but matching SearchTerm's decorator is the more consistent choice.)

This branch has not been deployed

No deployments
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.

2 participants