Issue 7602 - CI - lib389 user compare fails due to parentid mismatch#7603
Issue 7602 - CI - lib389 user compare fails due to parentid mismatch#7603droideck wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_format_compare_mismatch, consider passing in the pre-fetchedget_compare_attrs()results (or computing them once in the caller) instead of callingget_compare_attrs()twice on each object to avoid redundant LDAP reads. - In
test_user_compare_m2Repl, you might also assert that other instance-local attributes likeentryidandnsuniqueidare excluded fromget_compare_attrs()to prevent regressions on those fields as well.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_format_compare_mismatch`, consider passing in the pre-fetched `get_compare_attrs()` results (or computing them once in the caller) instead of calling `get_compare_attrs()` twice on each object to avoid redundant LDAP reads.
- In `test_user_compare_m2Repl`, you might also assert that other instance-local attributes like `entryid` and `nsuniqueid` are excluded from `get_compare_attrs()` to prevent regressions on those fields as well.
## Individual Comments
### Comment 1
<location path="src/lib389/lib389/_mapped_object.py" line_range="654-657" />
<code_context>
It will just check if the attributes are same.
- 'nsUniqueId' attribute is not checked intentionally because we want to compare arbitrary objects
- i.e they may have different 'nsUniqueId' but same attributes.
+ Instance-local operational attributes like 'nsUniqueId', 'entryid',
+ and 'parentid' are not checked intentionally because we want to
+ compare arbitrary objects, i.e they may have different internal
+ database identity but same attributes.
</code_context>
<issue_to_address>
**suggestion:** Docstring excludes `modifytimestamp` but it is also in `_compare_exclude`.
Since `_compare_exclude` also skips `modifytimestamp`, the current wording could mislead readers into thinking only the listed attributes are excluded. Please either add `modifytimestamp` to the description or rephrase to make it clear the list is illustrative rather than exhaustive.
```suggestion
Instance-local operational attributes like 'nsUniqueId', 'entryid',
'modifytimestamp', and 'parentid' are not checked intentionally
because we want to compare arbitrary objects, i.e they may have
different internal database identity but same attributes.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Instance-local operational attributes like 'nsUniqueId', 'entryid', | ||
| and 'parentid' are not checked intentionally because we want to | ||
| compare arbitrary objects, i.e they may have different internal | ||
| database identity but same attributes. |
There was a problem hiding this comment.
suggestion: Docstring excludes modifytimestamp but it is also in _compare_exclude.
Since _compare_exclude also skips modifytimestamp, the current wording could mislead readers into thinking only the listed attributes are excluded. Please either add modifytimestamp to the description or rephrase to make it clear the list is illustrative rather than exhaustive.
| Instance-local operational attributes like 'nsUniqueId', 'entryid', | |
| and 'parentid' are not checked intentionally because we want to | |
| compare arbitrary objects, i.e they may have different internal | |
| database identity but same attributes. | |
| Instance-local operational attributes like 'nsUniqueId', 'entryid', | |
| 'modifytimestamp', and 'parentid' are not checked intentionally | |
| because we want to compare arbitrary objects, i.e they may have | |
| different internal database identity but same attributes. |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
| # attributes, we don't want to compare | ||
| self._compare_exclude = ['entryid', 'modifytimestamp', 'nsuniqueid'] | ||
| # Backend-generated attributes that are local to an instance. | ||
| self._compare_exclude = ['entryid', 'modifytimestamp', 'nsuniqueid', 'parentid'] |
There was a problem hiding this comment.
I wonder if we should not also exclude some other operational attributes
( like the numsubordonates related one )
Description: Fix UserAccount.compare() failures between replicated entries where backend-local parentid values differ across suppliers. Treat parentid like entryid by excluding it from generic DSLdapObject compare attributes, since it is an internal database operational attribute generated per instance. Add regression coverage to ensure replicated user comparison does not include parentid. Fixes: 389ds#7602 Reviewed by: ?
58faf07 to
b2402cd
Compare
Description: Fix UserAccount.compare() failures between replicated entries where backend-local parentid values differ across suppliers. Treat parentid like entryid by excluding it from generic DSLdapObject compare attributes, since it is an internal database operational attribute generated per instance. Add regression coverage to ensure replicated user comparison does not include parentid.
Fixes: #7602
Reviewed by: ?
Summary by Sourcery
Exclude backend-local parentid from lib389 object comparison and add regression coverage for replicated user comparisons.
Bug Fixes:
Enhancements:
Tests: