Issue 7627 - When it exists configured matching rule for an indexed a…

[tbordaz]

…ttribute it should be used if the schema does not define one

Bug description:
An attribute is defined in the schema. The matching rule is optional in the schema
so the definition falls back to the default matching rule that is lexicographic order.

An attribute can be configured to be indexed and the configuration can define a
matching rule 'nsMatchingRule".

If the schema does not define a matching rule, the server should fall back to the
configured one ('nsMatchingRule') prior to default lexicographic one

Fix description:
At startup when binding the attribute their schema definitions
if no matching rule is define and it existed a configured matching rule
the default matching rule should be the configured one

fixes: #7627
fixes: #7518

Reviewed by:

Summary by Sourcery

Honor configured ordering matching rules when schema definitions lack compatible rules and improve detection and coverage of index ordering behavior.

Bug Fixes:

• Use an indexed attribute’s configured ordering matching rule when the schema does not provide a compatible ordering rule, while preserving schema-compatible behavior.

Enhancements:

• Add index validation for detecting indexes configured with integer ordering whose on-disk ordering remains lexicographic.
• Expand regression coverage for configured matching-rule precedence, integer ordering, collation indexing, and range searches.

Tests:

• Add regression tests covering matching-rule fallback, index ordering mismatch detection, collation-based indexing, and lexicographic versus integer range behavior.

[sourcery-ai]

Hey - I've left some high level feedback:

• The conditional and the accompanying comment/log message seem inconsistent: the code executes when !slapi_matchingrule_is_ordering(...) && slapi_matchingrule_can_use_compare_fn(...), but the log says the matching rule is not compatible with the attribute syntax; revisit the condition and log text to ensure they accurately reflect when the configured matching rule is considered compatible/usable.
• You always select official_rules[0] as the fallback ordering matching rule when multiple rules may be present; if multiple matching rules can be configured, consider clarifying or constraining this choice (e.g., explicitly selecting the ordering rule) so the behavior is deterministic and intentional.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The conditional and the accompanying comment/log message seem inconsistent: the code executes when `!slapi_matchingrule_is_ordering(...) && slapi_matchingrule_can_use_compare_fn(...)`, but the log says the matching rule is not compatible with the attribute syntax; revisit the condition and log text to ensure they accurately reflect when the configured matching rule is considered compatible/usable.
- You always select `official_rules[0]` as the fallback ordering matching rule when multiple rules may be present; if multiple matching rules can be configured, consider clarifying or constraining this choice (e.g., explicitly selecting the ordering rule) so the behavior is deterministic and intentional.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[packit-as-a-service]

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

• sudo dnf install -y 'dnf*-command(copr)'
• dnf copr enable packit/389ds-389-ds-base-7628
• And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

[droideck]

I think we're missing a return 0; after the loop. If the name/OID isn't registered we fall off the end and the caller gets an undefined value. The old inline loop was protected by is_compat, but this one is called directly, so a typo in nsMatchingRule (or a collation language tag like fr) would reach it. Since it's exported in slapi-plugin.h, maybe a NULL guard on the argument as well?

[tbordaz]

Agree

[droideck]

I believe this also catches collation rules. They're registered as caseIgnoreOrderingMatch-<lang>, so the "ordering" test passes, plugin_mr_find() returns NULL (the collation plugin has no plg_mr_names), and do_continue = 1 skips the RULE path. If I'm reading it right, nsMatchingRule: 2.16.840.1.113730.3.3.2.11.1 on cn/sn/uid would stop creating the index after upgrade. What do you think about only taking this branch when plugin_mr_find() returns a plugin with plg_mr_compare, and leaving do_continue at 0 otherwise so the RULE path still runs?

[tbordaz]

Good catch. collation rule can not be used for comparison. It it is a valid index rule. The patch prevents to use it as an index. I need to rework the patch.
I agree the problem here is that do_continue is set to 1. Now the collation plugin is not a valid comparison and the patch should prevent to use it via mr_ordering_plugin.

[droideck]

Small one: mr_ordering_plugin is overwritten on every value, including with NULL when the lookup fails. With [integerOrderingMatch, <collation OID>] on one index we'd end up with neither a comparator nor a RULE index. Maybe assign only on success?

[tbordaz]

I think the an attribute should have only one comparison (ordering) nsMatchingRule. If multiple ordering are set, it looks to me an invalid config and taking the last value is acceptable.

[droideck]

Two thoughts here.
This block doesn't check need_compare_fn, so with a compatible plus an incompatible ordering rule on the same index the incompatible one wins (uidNumber with integerOrderingMatch and caseIgnoreOrderingMatch would get a string comparator). Skipping the block when we already have a comparator would probably be enough.

Also, this is the upgrade case from my earlier comment and IMO it deserves a WARNING with a reindex hint rather than INFO. An existing lexicographic index gets opened with int_compare and equality lookups miss keys until db2index.
One more thing I noticed: for string syntax the keys aren't integer-normalized, so =007 and =7 compare equal and merge into one key, and (uid=7) would return the 007 entry because indexed equality skips the filter test. Would it make sense to keep the can_use_compare_fn gate and limit this to what parentid needs?
But I don't have a strong opinion here and maybe it's something we do intentionally.

[tbordaz]

Right with multiple nsMatchingRule values we can have need_compare_fn and mr_ordering_plugin both set. I need to rework the patch for multiple ordering nsMatchingRule.

The upgrade I was thinking of heathcheck being responsible to report incoherent index ordering versus ordering config.

A string syntax attribute should not normalize '007' into '7'. So if the search filter is '(uid=007)' the equality index should not return the key '=7'. How did you get this behavior ?

[droideck]

Suggested change

	log.warning(" %s - MISMATCH: config has integerOrderingMatch but disk is lexicographic (suggest redindex)", index_name)
	log.warning(" %s - MISMATCH: config has integerOrderingMatch but disk is lexicographic (suggest reindex)", index_name)

[droideck]

Should we also set all_ok = False here when reporting the MISMATCH?

[droideck]

_check_disk_ordering() only looks at the first 100 dbscan lines. For attributes with a sub index those are all */+ keys, so it returns UNKNOWN and we silently skip cn/uid on real deployments, IIUC.
Not new code, but now that it runs on user indexes it probably needs to count = keys instead of raw lines...

[droideck]

LGTM! Thank you!