Skip to content

fix: don't lose aliases when deserializing duplicate or malformed data - #356

Open
Kevin Turcios (KRRT7) wants to merge 1 commit into
microsoft:mainfrom
KRRT7:fix-sqlite-alias-deserialize
Open

Kevin Turcios (KRRT7) wants to merge 1 commit into
microsoft:mainfrom
KRRT7:fix-sqlite-alias-deserialize

Conversation

@KRRT7

Copy link
Copy Markdown
Contributor

Summary

  • SqliteRelatedTermsAliases.deserialize and set_related_terms used a plain INSERT, so an alias listed more than once (for example the same termText in two items) raised sqlite3.IntegrityError: UNIQUE constraint failed on the (term, alias) primary key. add_related_term already uses INSERT OR IGNORE, and the memory alias index keeps the first copy. Both now use INSERT OR IGNORE.
  • deserialize deleted the existing aliases before parsing its input, in both the SQLite and memory backends. Duplicate (SQLite) or malformed (both) data therefore raised after the existing aliases were already gone. Both backends now parse the whole input first and only then replace the stored aliases, so a failed deserialize leaves them intact.

Validation

  • New tests run against both backends: deserializing duplicate aliases succeeds with one copy, and malformed data raises KeyError without losing existing aliases. Plus a SQLite test for set_related_terms with duplicates. All fail without the fix (except the duplicate case on memory, which already worked).
  • make format check test

SqliteRelatedTermsAliases.deserialize and set_related_terms used a plain
INSERT, so an alias listed twice raised IntegrityError on the (term, alias)
primary key; add_related_term already used INSERT OR IGNORE, and the memory
index keeps the first copy. deserialize also deleted the existing aliases
before parsing its input, in both backends, so duplicate or malformed data
left the index empty. Parse first, then replace, and ignore duplicates.

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.

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