Skip to content

fix: is_ordered_sublist matching substrings across list items - #2286

Merged
degenaro merged 5 commits into
oscal-compass:developfrom
arpitjain099:fix/ordered-sublist-substring
Jul 27, 2026
Merged

fix: is_ordered_sublist matching substrings across list items#2286
degenaro merged 5 commits into
oscal-compass:developfrom
arpitjain099:fix/ordered-sublist-substring

Conversation

@arpitjain099

Copy link
Copy Markdown
Contributor

Types of changes

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

Summary

While reading through trestle/common/list_utils.py I noticed is_ordered_sublist joins both lists with spaces and then does a plain substring test. That lets a needle item match part of a haystack item rather than a whole item, so is_ordered_sublist(['b'], ['abc']) returns True and is_ordered_sublist(['a', 'b'], ['xa', 'by']) does too. The docstring and its examples describe an item-wise contiguous match, so these are false positives. This function is used in the xlsx component-definition task (xlsx_helper.py) to detect the NIST mappings column from header tokens, where a partial-token match could pick the wrong column.

The fix compares the items directly as a contiguous run instead of using string containment. I added a few cases to the existing test covering the substring situations and a needle longer than the haystack. The updated list_utils tests and the xlsx_to_oscal_cd task tests both pass locally.

Key links:

is_ordered_sublist joined both lists with spaces and did a substring
test, so a needle item could match part of a haystack item (for
example ['b'] was reported as contained in ['abc']). Compare the items
directly as a contiguous run instead, which matches the documented
behavior and the existing examples.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@arpitjain099
arpitjain099 requested a review from a team as a code owner July 8, 2026 13:02
@degenaro

Copy link
Copy Markdown
Collaborator

review from Claude:

Review: #2286 — Fix is_ordered_sublist matching substrings across list items

What it does

Fixes is_ordered_sublist in trestle/common/list_utils.py. The old implementation:

return ' '.join(needle) in ' '.join(haystack)

joined both lists into space-separated strings and did substring matching — so it could match parts of items, not just whole items. For example, is_ordered_sublist(['b'], ['abc']) returned True, even though 'b' isn't a haystack item at all, just a character inside 'abc'.

The new implementation:

n_items = len(needle)
if n_items == 0:
    return True
for start in range(len(haystack) - n_items + 1):
    if haystack[start:start + n_items] == needle:
        return True
return False

This slides a window of length len(needle) over haystack and checks for an exact contiguous match — a correct, direct translation of "is needle a contiguous sublist of haystack."

Correctness ✅

  • is_ordered_sublist(['b'], ['abc']) → now correctly False (was True)
  • is_ordered_sublist(['a', 'b'], ['xa', 'by']) → now correctly False (was True, since "a b" is a substring of "xa by")
  • is_ordered_sublist(['a', 'b'], ['a']) → correctly False. range(len(haystack) - n_items + 1) naturally produces an empty range when the needle is longer than the haystack, so no extra guard is needed there.
  • Empty needle → True (vacuously contained), a reasonable convention.

Complexity

O(n·m) worst case instead of a single string concatenation + substring search — but these lists are short header/token lists (used for xlsx column detection), so this is a non-issue.

Tests ✅

The added cases directly target the bug (partial substring matches), plus an edge case (needle longer than haystack) and a sanity check (exact match still works). Focused and sufficient.

Nits

  • The comment # an empty needle is trivially contained; a needle longer than the haystack cannot be appears truncated — it looks like it was meant to span two separate comments (one for the if n_items == 0 branch, one for the loop) but got cut off mid-sentence. Worth tidying before merge.
  • No docstring update needed — the new behavior now actually matches the existing docstring examples, which was the whole point of the fix.

Verdict

Clean, minimal, well-tested bug fix with a clear rationale (traced to a real downstream issue in xlsx_helper.py column detection). Approve pending the comment cleanup.

@degenaro degenaro changed the title Fix is_ordered_sublist matching substrings across list items fix: is_ordered_sublist matching substrings across list items Jul 13, 2026
@degenaro

Copy link
Copy Markdown
Collaborator

make code-lint is failing.

degenaro and others added 2 commits July 13, 2026 08:48
Replaces the explicit scan loop with any(), which is what
'hatch fmt --linter --check' was flagging (SIM110). Behavior is
unchanged, including the empty-needle and needle-longer-than-haystack
cases.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@arpitjain099

Copy link
Copy Markdown
Contributor Author

Thanks @degenaro, fixed in f7b9849.

The failure was ruff SIM110 on the scan loop in is_ordered_sublist. It's now an any() over the same window comparison:

return any(haystack[start:start + n_items] == needle for start in range(len(haystack) - n_items + 1))

hatch fmt --linter --check passes on both changed files now.

Behavior is unchanged. I checked the old and new versions against each other over the edge cases (empty needle, needle longer than haystack, repeated elements, both docstring examples, and the ['b','c'] vs ['ab','cd'] case this PR is actually about) and they agree on every one.

One note in case it comes up separately: ruff format --check also wants to reformat this file, but it does that on the unmodified file too, so it's pre-existing and not something this PR introduced. I left it alone rather than mixing a reformat into this change.

Whitespace only. Makes 'ruff format --check' clean for this file as
well as the linter.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@arpitjain099

Copy link
Copy Markdown
Contributor Author

Follow-up on my note about the formatter: I was wrong that it was worth leaving alone.

ruff format only wanted one line in this file, and it was the line I'd just written, the slice spacing in haystack[start : start + n_items]. It's whitespace only, so I've applied it in 0960dfa. Both ruff check and ruff format --check are clean on this file now, not just the linter.

@degenaro degenaro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@degenaro

Copy link
Copy Markdown
Collaborator

@arpitjain099

Copy link
Copy Markdown
Contributor Author

@arpitjain099 are you on our slack channel? [#oscal-compass-trestle-agileauthoring-c2p](https://cloud-native.slack.com/archives/C06F3PEPNBW)

@degenaro
No, I am not. I tried signing up via google account but I see this message. Would love to join the community!
image

@arpitjain099

Copy link
Copy Markdown
Contributor Author

@arpitjain099 are you on our slack channel? [#oscal-compass-trestle-agileauthoring-c2p](https://cloud-native.slack.com/archives/C06F3PEPNBW)

@degenaro can you help me get an invite to slack? I can share my email (rather not here, publicly)

@degenaro

Copy link
Copy Markdown
Collaborator

@arpitjain099 Have you tried https://cncf-slack.netlify.app/ ?

@arpitjain099

Copy link
Copy Markdown
Contributor Author

@arpitjain099 Have you tried https://cncf-slack.netlify.app/ ?

@degenaro Joined :)
Thank you

@arpitjain099

Copy link
Copy Markdown
Contributor Author

@degenaro this PR is ready to merge.

@degenaro
degenaro merged commit a5ea1c5 into oscal-compass:develop Jul 27, 2026
16 checks passed
degenaro added a commit that referenced this pull request Aug 7, 2026
* Fix is_ordered_sublist matching substrings across list items

is_ordered_sublist joined both lists with spaces and did a substring
test, so a needle item could match part of a haystack item (for
example ['b'] was reported as contained in ['abc']). Compare the items
directly as a contiguous run instead, which matches the documented
behavior and the existing examples.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>

* fix: satisfy ruff SIM110 in is_ordered_sublist

Replaces the explicit scan loop with any(), which is what
'hatch fmt --linter --check' was flagging (SIM110). Behavior is
unchanged, including the empty-needle and needle-longer-than-haystack
cases.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>

* style: apply ruff format to the slice expression

Whitespace only. Makes 'ruff format --check' clean for this file as
well as the linter.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>

---------

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Co-authored-by: Lou DeGenaro <lou.degenaro@gmail.com>
Signed-off-by: Lou DeGenaro <lou.degenaro@gmail.com>
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.

2 participants