Skip to content

fix: omit unspecified offsets in date-property TTL updates - #2171

Open
HuaTNA wants to merge 2 commits into
weaviate:mainfrom
HuaTNA:fix/preserve-omitted-ttl-offset
Open

HuaTNA wants to merge 2 commits into
weaviate:mainfrom
HuaTNA:fix/preserve-omitted-ttl-offset

Conversation

@HuaTNA

@HuaTNA HuaTNA commented Sep 23, 2026 •

Copy link
Copy Markdown

Fixes #2170.

Reconfigure.ObjectTTL.delete_by_date_property() coerces an omitted ttl_offset to zero. Even a call that only supplies filter_expired_objects therefore includes an explicit defaultTtl: 0 in the generated TTL update block. Keep None unchanged so the offset is omitted instead. Explicit zero and timedelta offsets still work, and collection creation still defaults to zero.

The regression tests check the factory result, omission when merging into an empty TTL block, and non-overwriting of positive/negative offsets when merging directly into an existing TTL block. They do not establish end-to-end preservation through the collection update path.

Related review findings

The review identified two existing behaviors that still need a maintainer decision before merge:

  • The collection update path uses objectTTLConfig, while creation and reading use objectTtlConfig. Locally, merging into a schema with the latter spelling produces two blocks. The old TTL block is untouched; the new one omits defaultTtl after this fix.
  • The delete_by_* reconfiguration factories set enabled=True, including when only a filtering option is supplied. Changing that activation contract needs consideration separately from offset omission.

These behaviors have been reproduced locally. No live-server test was run, so this PR makes no claim about how the server reconciles the two differently spelled blocks or preserves an existing TTL value.

Validation

  • With the revised regression tests retained and the old coercion restored: 2 failed, 24 passed in test/collection/test_config_update.py.
  • With the fix: 233 passed across test/collection/test_config_update.py and test/collection/test_config.py.
  • Ruff lint/format, Flake8, and whitespace checks passed.
  • Python 3.13.13, macOS arm64; no live-server integration test was run.

AI assistance was used to investigate, implement, and test this patch and review follow-up.

@orca-security-eu orca-security-eu Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Infrastructure as Code high 0   medium 0   low 0   info 0 View in Orca
Passed Passed SAST high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Vulnerabilities high 0   medium 0   low 0   info 0 View in Orca

@weaviate-git-bot

Copy link
Copy Markdown

To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge.

beep boop - the Weaviate bot 👋🤖

PS:
Are you already a member of the Weaviate Forum?

@HuaTNA

HuaTNA commented Sep 23, 2026

Copy link
Copy Markdown
Author

I agree to the Weaviate Contributor License Agreement.

@chrikrah chrikrah left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@HuaTNA the deletion is right and I would hold it for one answer. I ran it at 1a55a55 and falsified it: put
the ttl_offset is None coercion back with your tests kept, and both parametrisations of
test_object_ttl_update_preserves_omitted_offset fail, against 26 passed on your branch.

What the removed line actually buys is narrower than it looks, and worth getting right in the description.
_CollectionConfigUpdate reads and writes objectTTLConfig, while the server and this client's own read path
at config_methods.py:464 both spell it objectTtlConfig. Merging into a fetched schema with the server's
spelling leaves that block untouched and appends a second one, so the client preserves nothing: what your
change does is stop an explicit defaultTtl: 0 being sent, and the server is what keeps the old value.

blocking, and not caused by you: enabled is the only non-Optional field on ObjectTTLConfigUpdate, and
every factory hardcodes enabled=True. Merging a filter-only update into a disabled config returns
enabled: True, so a caller who wanted one flag changed restarts object deletion, and after your change the
offset then in force is the preserved one. Worth deciding before this lands.

Evidence, Python 3.13, editable install plus pytest, at 1a55a55:

$ python -m pytest test/collection/test_config_update.py -q
26 passed, 4 warnings in 0.04s
# coercion restored in the Reconfigure path, your tests kept
2 failed, 24 passed

$ python probe_ttl_case.py
keys after merge: ['objectTTLConfig', 'objectTtlConfig']
server block untouched: {'enabled': True, 'deleteOn': 'expiresAt', 'defaultTtl': 3600, 'filterExpiredObjects': False}
appended block: {'enabled': True, 'filterExpiredObjects': True}
disabled config after a filter-only update: {'enabled': True, ..., 'filterExpiredObjects': True}

Every claim above came from those runs. I did not run the rest of the suite, and nothing here talks to a
Weaviate instance, so the two-block payload is the client's output and I have not seen how the server
reconciles it.

@dirkkul, you merged 7 of the changes touching this file and .github/CODEOWNERS matches nothing here: is
the key casing worth its own fix before this one, or after?

@HuaTNA HuaTNA changed the title fix: preserve omitted offsets in date-property TTL updates fix: omit unspecified offsets in date-property TTL updates Sep 29, 2026
@HuaTNA

HuaTNA commented Sep 29, 2026

Copy link
Copy Markdown
Author

Thanks for the detailed check. I reproduced both the two-block collection payload and enabled=True when merging a filter-only update into a disabled TTL block at 1a55a55.

I've corrected the PR description, issue, and parameter documentation to describe offset omission, without claiming end-to-end preservation by the client or server. In 0be94ae4, the tests now inspect the factory result and the TTL-block merge directly, including an empty block where defaultTtl must be absent. They no longer use the collection-level fixture with the mismatched spelling. Restoring the coercion still gives 2 failed, 24 passed; the fixed configuration suites give 233 passed. Ruff and Flake8 also pass.

For activation, the existing test_object_ttl_update integration test uses a delete_by_* factory to enable TTL on a collection without a TTL configuration. A change to preserve enabled=False needs to account for that enabling use case as well. The casing mismatch and activation behavior are still present in current main (eb5546a8); this update leaves their resolution pending the maintainer decision you requested.

I haven't run a live-server test and cannot establish how the server reconciles the two blocks. I also checked the two failing CI jobs on the original PR head: one fails during collection creation in test_text_from_collection_config with class already exists; the other fails the backup-cancellation status assertion in test_cancel_backup_create[False]. Those logs do not show a TTL-test failure, but I have not independently reproduced either failure.

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.

Partial date-property TTL updates reset an omitted offset to zero

3 participants