镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Allow fact updates to fields whose current value is falsy - #3426

Open
bacescuandrei wants to merge 1 commit into
apache:masterfrom
bacescuandrei:fix/update-fact-falsy-fields
Open

bacescuandrei wants to merge 1 commit into
apache:masterfrom
bacescuandrei:fix/update-fact-falsy-fields

Conversation

@bacescuandrei

Copy link
Copy Markdown

Description

BaseKnowledgeService._update_fact() only applies an update when the current attribute value is truthy:

if getattr(match, k, False):
    setattr(match, k, v)

So knowledge_svc.update_fact() and PATCH /api/v2/facts silently ignore updates to fields whose current value is 0, None, '' or []. Examples are a fact with score 0, technique_id None, or an empty source. Operation.all_facts() only returns facts with score > 0, so a fact whose score dropped to 0 can never be given a positive score again.

This change checks that the attribute exists (hasattr), as _update_relationship() already does.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Added test_update_fact_with_falsy_current_value to tests/services/test_knowledge_svc.py. It updates score from 0 and technique_id from None, and checks that an unknown key is not added.
  • The test fails on master (assert 0 == 2) and passes with the change.
  • Full suite on Python 3.12: 562 passed, 1 xfailed.
  • flake8 and bandit are clean on the changed files.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works

_update_fact() only applied an update when the current attribute value
was truthy, so a fact with score 0, technique_id None or an empty
source could not be updated through the knowledge service or
PATCH /api/v2/facts. A fact whose score dropped to 0 is excluded from
Operation.all_facts() and could not be given a positive score again.

Check that the attribute exists instead, as _update_relationship()
already does.
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.

1 participant