Skip to content

BED-6631: RODC Default Selector Broken Due To Incorrect Cypher #1981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

BED-6631: RODC Default Selector Broken Due To Incorrect Cypher #1981

wants to merge 2 commits into from
+44 −0

Conversation

ALCooper12
Copy link
Contributor

@ALCooper12 ALCooper12 commented Oct 17, 2025 •
edited
Loading

Description

  • Altered v8.3.0 migration file to use the correct attribute name n.isreadonlydc = true for the cypher query of the RO-DC default selector

Motivation and Context

This PR addresses:

Why is this change required? What problem does it solve?

  • At the moment, the default selector for Read-Only DCs does not return objects because the cypher query references the attribute incorrectly as isReadOnlyDC. The attribute in BloodHound is case-sensitive and is actually stored as isreadonlydc. This fix will allow results to be returned when running the cypher query

How Has This Been Tested?

  • Within my local env and the test env (still working on this)*

Screenshots (optional):

Types of changes

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

Checklist:

Summary by CodeRabbit

  • New Features

    • Added a default "Read‑Only DCs" selector so read‑only data centers are automatically identified and available for grouping and filtering.

  • Chores

    • Bootstrapped selector and seed data added to ensure the new selector is present on system setup without altering existing selectors.

@ALCooper12 ALCooper12 self-assigned this Oct 17, 2025
@ALCooper12 ALCooper12 added bug Something isn't working api A pull request containing changes affecting the API code. labels Oct 17, 2025
@coderabbitai coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 17, 2025 •
edited
Loading

Walkthrough

A SQL migration is introduced that adds a new "Read-Only DCs" asset group tag selector to Tier Zero. The migration creates a selector entry with a cypher targeting the isreadonlydc attribute and conditionally seeds it if it doesn't already exist.

Changes

Cohort / File(s) Summary
Database Migration
cmd/api/src/database/migration/migrations/v8.3.0.sql		 Adds data-driven CTE-based insert flow to create a new asset_group_tag_selectors entry for "Read-Only DCs" with cypher targeting isreadonlydc attribute, and seeds a corresponding asset_group_tag_selector_seeds record conditioned on non-existence.

Sequence Diagram

sequenceDiagram
    participant Migration as v8.3.0 Migration
    participant DB as Database
    
    Migration->>DB: Check if "Read-Only DCs" selector exists
    alt Selector does not exist
        Migration->>DB: Insert into asset_group_tag_selectors<br/>(name, cypher, description)
        DB-->>Migration: Return inserted id & name
        Migration->>DB: Insert into asset_group_tag_selector_seeds<br/>(using returned id)
        DB-->>Migration: Seed created
    else Selector already exists
        DB-->>Migration: Skip insertion
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Reasoning: Single migration file following established selector-upsert patterns with straightforward CTE logic. Primary focus should be verifying correct SQL syntax, accurate isreadonlydc attribute targeting, and proper non-existence condition handling.

Possibly related PRs

Suggested reviewers

  • urangel
  • mistahj67

Poem

🐰✨ A selector springs forth, neat and true,
Read-Only DCs now tagged in view,
With cypher's dance through the database floor,
Tier Zero's wisdom grows ever more! 🎯

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description Check ❓ Inconclusive The pull request description includes all major sections required by the template: Description, Motivation and Context (with BED-6631 ticket reference), How Has This Been Tested, Types of changes, and a completed Checklist. The Motivation and Context section provides clear reasoning for the fix, explaining that BloodHound stores the attribute case-sensitively as isreadonlydc rather than isReadOnlyDC. However, there is a significant inconsistency: the testing section states "still working on this", suggesting testing is incomplete, yet all items in the Checklist—including "All new and existing tests passed"—are marked as complete. This contradiction raises questions about the accuracy of the PR description and whether it accurately reflects the current state of the changes. To move forward, the author should clarify the testing status by either completing the testing and updating the testing section to remove the "(still working on this)" note, or unchecking the Checklist items related to testing if it is genuinely incomplete. The description content itself is appropriate for the change, but this discrepancy must be resolved to ensure the PR accurately reflects its readiness for review.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "BED-6631: RODC Default Selector Broken Due To Incorrect Cypher" directly and accurately summarizes the primary change in the changeset. It clearly identifies the issue (RODC default selector), the root cause (incorrect Cypher query), and references the associated ticket (BED-6631). The title is specific, concise, and immediately communicates the nature of the fix to someone scanning the repository history. It avoids vague language and directly aligns with the actual modification made to the v8.3.0 migration file.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch BED-6631

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

sirisjo
sirisjo reviewed Oct 20, 2025
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sirisjo sirisjo sirisjo left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@ALCooper12 ALCooper12

Labels

api A pull request containing changes affecting the API code. bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ALCooper12 @sirisjo