Conversation
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
|
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. |
|
I agree to the Weaviate Contributor License Agreement. |
chrikrah
left a comment
There was a problem hiding this comment.
@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?
|
Thanks for the detailed check. I reproduced both the two-block collection payload and 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 For activation, the existing 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 |
Fixes #2170.
Reconfigure.ObjectTTL.delete_by_date_property()coerces an omittedttl_offsetto zero. Even a call that only suppliesfilter_expired_objectstherefore includes an explicitdefaultTtl: 0in the generated TTL update block. KeepNoneunchanged 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:
objectTTLConfig, while creation and reading useobjectTtlConfig. Locally, merging into a schema with the latter spelling produces two blocks. The old TTL block is untouched; the new one omitsdefaultTtlafter this fix.delete_by_*reconfiguration factories setenabled=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
test/collection/test_config_update.py.test/collection/test_config_update.pyandtest/collection/test_config.py.AI assistance was used to investigate, implement, and test this patch and review follow-up.