`/deployments` only returns top-level deployments — subdeployments not discoverable without parent ID

[Sam-Bolling]

Finding

DeploymentRepository.applyFilters() explicitly excludes subdeployments from top-level GET /deployments responses with WHERE parent_deployment_id IS NULL OR parent_deployment_id = '', making subdeployments undiscoverable without prior knowledge of the parent deployment ID.

Review Source: Live integration testing against the Go CSAPI server during OSHConnect-Python publisher fleet migration to the Go server.
Severity: P2-Important
Category: API Design
Ownership: Upstream (SomethingCreativeStudios/connected-systems-go)

---

Problem Statement

When a client requests GET /deployments, the repository layer explicitly filters to only top-level deployments (parent_deployment_id IS NULL). Subdeployments created via POST /deployments/{parentId}/subdeployments are completely absent from the top-level listing. The only way to discover them is to already know the parent deployment ID and query /deployments/{parentId}/subdeployments.

This makes it impossible for a client to enumerate all deployments in a collection without first fetching every top-level deployment and recursively querying each for subdeployments.

Affected code — internal/repository/deployment_repository.go — applyFilters():

func (r *DeploymentRepository) applyFilters(query *gorm.DB, params *queryparams.DeploymentsQueryParams, parentId *string) *gorm.DB {
    // ... other filters ...

    if parentId != nil {
        if params.Recursive {
            // Recursive traversal from parent to include all descendants.
            query = query.Where(`id IN (
                WITH RECURSIVE deployment_descendants AS (
                    SELECT id FROM deployments WHERE parent_deployment_id = ?
                    UNION ALL
                    SELECT d.id FROM deployments d
                    JOIN deployment_descendants dd ON d.parent_deployment_id = dd.id
                )
                SELECT id FROM deployment_descendants
            )`, *parentId)
        } else {
            // Direct children only
            query = query.Where("parent_deployment_id = ?", *parentId)
        }
    } else {
        if params.Recursive {
            // Canonical recursive search should include top-level deployments and all descendants.
            // ← Currently a no-op — does NOT actually include descendants
        } else {
            // ← THIS LINE: explicitly excludes all subdeployments from top-level listing
            query = query.Where("parent_deployment_id IS NULL OR parent_deployment_id = ''")
        }
    }

    return query
}

Affected code — internal/api/deployment_handler.go — ListDeployments():

func (h *DeploymentHandler) ListDeployments(w http.ResponseWriter, r *http.Request) {
    params := queryparams.DeploymentsQueryParams{}.BuildFromRequest(r)
    deployments, total, err := h.repo.List(params, nil)  // parentId = nil → top-level only
    // ...
}

Scenario:

# Create a top-level deployment
POST /deployments
{ "type": "Feature", "properties": { "uid": "urn:test:dep:parent", "name": "Parent" } }
# → 201, Location: /deployments/aaa-111

# Create a subdeployment
POST /deployments/aaa-111/subdeployments
{ "type": "Feature", "properties": { "uid": "urn:test:dep:child", "name": "Child" } }
# → 201, Location: /deployments/bbb-222

# List all deployments
GET /deployments
# Expected: both "Parent" and "Child" returned
# Actual: only "Parent" returned — "Child" is invisible

# Also note: the `recursive=true` path for parentId==nil is a no-op:
GET /deployments?recursive=true
# Expected: all deployments (top-level + descendants)
# Actual: same as without recursive — the `if params.Recursive` branch does nothing

Impact: The OSHConnect-Python bootstrap helpers use find_by_uid() against /deployments to check if a subdeployment already exists before creating it. Because subdeployments are not in the top-level listing, the lookup always returned None, causing duplicate creation attempts that failed on the unique UID constraint. The workaround adds special-case logic to search under the parent:

# Workaround in bootstrap_helpers.py
def ensure_deployment(session, base_url, collection_id, deployment, parent_id=None):
    existing = find_by_uid(session, deps_url, deployment["properties"]["uid"])
    if existing:
        return existing["id"]
    # Subdeployment not in top-level listing — try under parent
    if parent_id:
        sub_url = f"{deps_url}/{parent_id}/subdeployments"
        existing = find_by_uid(session, sub_url, deployment["properties"]["uid"])
        if existing:
            return existing["id"]
    # ... create it

Additionally, the recursive=true code path for top-level listings is a no-op comment with no actual implementation:

if params.Recursive {
    // Canonical recursive search should include top-level deployments and all descendants.
    // ← This is just a comment — no filter is applied, so it returns same as non-recursive
}

---

Ownership Verification

This code is pre-existing in the upstream SomethingCreativeStudios/connected-systems-go repository. The OS4CSAPI fork is currently synced with upstream (main branch is up to date).

Conclusion: This code is upstream.

Files to Modify

File	Action	Est. Lines	Purpose
internal/repository/deployment_repository.go	Modify	~15	Change default listing to include subdeployments (flat view), or implement recursive=true at top level
e2e/	Modify	~25	Add E2E test verifying subdeployments appear in top-level listing

Proposed Solutions

Option A: Flat listing — include all deployments at top level (Recommended)

Remove the parent_deployment_id IS NULL filter from the default (non-recursive, no-parent) path. All deployments appear in GET /deployments regardless of nesting depth. Each subdeployment carries its parent_deployment_id so clients can reconstruct the hierarchy if needed.

} else {
    if params.Recursive {
        // No filter needed — return everything
    } else {
        // REMOVED: query = query.Where("parent_deployment_id IS NULL OR parent_deployment_id = ''")
        // Now returns all deployments, matching SensorHub behavior
    }
}

Pros: Matches SensorHub behavior; simplest fix; clients can discover all deployments with a single request; consistent with how /systems returns all systems (including subsystems)
Cons: Changes existing behavior — clients relying on top-level-only might receive more results
Effort: Small | Risk: Low

Option B: Implement recursive=true at top level

Keep the default behavior (top-level only) but implement the currently-no-op recursive=true path:

} else {
    if params.Recursive {
        // Return all deployments (top-level + all descendants)
        // No WHERE clause needed — just omit the parent filter
    } else {
        query = query.Where("parent_deployment_id IS NULL OR parent_deployment_id = ''")
    }
}

Pros: Preserves existing default behavior; adds opt-in flat listing; the code comment already suggests this was intended
Cons: Clients must know to pass ?recursive=true; still breaks discovery for clients that don't know about the parameter
Effort: Small | Risk: None

Scope — What NOT to Touch

• ❌ Do NOT modify files outside the "Files to Modify" table above
• ❌ Do NOT refactor adjacent code that isn't part of this finding
• ❌ Do NOT change the ListSubdeployments() handler behavior — it correctly lists children of a specific parent
• ❌ Do NOT change the AddSubdeployment() creation flow
• ❌ Do NOT modify the closure table logic in findAllChildren()

Acceptance Criteria

• GET /deployments returns both top-level deployments and subdeployments (Option A), or GET /deployments?recursive=true returns all deployments (Option B)
• Subdeployments include their parent_deployment_id in the response so clients can reconstruct hierarchy
• GET /deployments/{id}/subdeployments still works correctly (direct children of a parent)
• Pagination (limit, offset) works correctly with the expanded result set
• Existing tests still pass (make test)
• E2E test added verifying subdeployment visibility

Dependencies

Blocked by: Nothing
Blocks: Nothing
Related: #7 — ?uid= filter ignored (compounds this problem — can't filter subdeployments by UID even if listed); #9 — Default limit of 10 (expanded listing increases the importance of reasonable pagination defaults)

---

Operational Constraints

⚠️ MANDATORY: Before starting work on this issue, review docs/governance/AI_OPERATIONAL_CONSTRAINTS.md if available.

Key constraints:

• Precedence: OGC specifications → AI Collaboration Agreement → This issue description → Existing code → Conversational context
• No scope expansion: Fix the finding, nothing more
• Minimal diffs: Prefer the smallest change that satisfies the acceptance criteria
• Ask when unclear: If intent is ambiguous, stop and ask for clarification
• Respect ownership: This code is upstream — coordinate with the maintainer if contributing back

Ownership-Specific Constraints

If Upstream:

• Track the issue for potential future contribution or discussion with the maintainer
• If the fix is trivial and clearly beneficial, note in the issue that it could be offered as a separate upstream PR

References

#	Document	What It Provides
1	internal/repository/deployment_repository.go — applyFilters()	Root cause — WHERE parent_deployment_id IS NULL excludes subdeployments
2	internal/api/deployment_handler.go — ListDeployments()	Handler passes nil parentId → triggers top-level-only filter
3	OGC 23-001 §8.5 — Deployment resources	Spec defines deployment listing endpoint
4	OSHConnect-Python publishers/bootstrap_helpers.py — ensure_deployment()	Real-world workaround demonstrating impact
5	SensorHub API behavior	Comparison — SensorHub returns all deployments (flat) at /deployments

[Sam-Bolling]

Evaluation result — VALIDATED with corrections. Defect is real; two specific claims need correction; two additional defects discovered under the same root cause.

Full write-up: docs/research/issue-evaluations/issue-008.md. Spec extracts and live transcripts under docs/research/evidence/issue-008/.

The core finding holds

/deployments default does exclude subdeployments — confirmed live (T1 = numberMatched 1 with parent only; T4 = subdeployment retrievable at /{id}/subdeployments; T5 = subdeployment retrievable directly by URL). Root cause is the unconditional WHERE parent_deployment_id IS NULL OR parent_deployment_id = '' clause in applyFilters at the top-level branch.

The spec is on this issue's side

Direct fetch of canonical Part 1 OAS at opengeospatial/ogcapi-connected-systems/master/api/part1/openapi:

	paths/systems.yaml	paths/deployments.yaml
GET description	"By default, only top level systems are included (i.e., subsystems are ommitted) unless the parent query parameter is set."	"List or search all Deployment resources available from this server endpoint."
recursive parameter	$ref: ../parameters/recursive.yaml	absent
parent parameter	present	present

And from paths/subdeployments.yaml:

"individual members can also be retrieved by ID directly at the canonical Deployment resources endpoint."

The OAS asymmetry between /systems and /deployments is deliberate: /systems declares top-level-only as the default and provides recursive to override; /deployments does neither. The spec contract for /deployments is flat by default, with ?parent=X for filtering.

Two factual claims in the issue body are wrong

(1) "consistent with how /systems returns all systems (including subsystems)" — false. internal/repository/system_repository.go:386-389:

if !params.Recursive {
    query = query.Where("systems.parent_system_id IS NULL")
}

/systems defaults to top-level only too. The difference is that for /systems it's spec-mandated, and for /deployments it's a non-spec restriction. The framing should be reversed: "the spec deliberately treats /deployments differently from /systems."

(2) "the recursive=true path for parentId==nil is a no-op" — false. Live test T2:

GET /deployments?recursive=true → numberMatched=2 (parent + child)

The empty branch isn't missing implementation; it omits the IS NULL clause (which lives in the sibling else), so the query falls through unfiltered. The misleading element is the comment, which reads like a TODO. Net behavior is already correct for that flag, though recursive is not even a spec parameter at /deployments.

Two additional defects under the same root cause (not in the issue body)

D1 — ?parent=X returns 0 at top level (T3):

GET /deployments?parent=<parentId> → numberMatched=0

applyFilters adds WHERE parent_deployment_id IN (parentId) AND the default-branch's WHERE parent_deployment_id IS NULL. AND'd → impossible. The spec parameter parent is broken at the canonical endpoint.

D2 — ?id=<URI> of a subdeployment returns 0 (T6):

GET /deployments?id=urn:test:issue8:dep:child → numberMatched=0

Same conflict (id IN ? OR unique_identifier IN ? AND'd with parent_deployment_id IS NULL). This directly violates the spec note in paths/subdeployments.yaml that "individual members can also be retrieved by ID directly at the canonical Deployment resources endpoint." It also explains the OSHConnect find_by_uid failure described in the issue: subdeployments are simply unaddressable from the canonical endpoint by their UID.

D2 is arguably more severe than the headline finding because it violates an explicit spec sentence (not just structural inference) and breaks the canonical URI-lookup workflow.

Both D1 and D2 collapse into the same single fix as Option A: removing the unconditional parent_deployment_id IS NULL clause.

Recommended fix (refined Option A)

 } else {
-    if params.Recursive {
-        // Canonical recursive search should include top-level deployments and all descendants.
-    } else {
-        query = query.Where("parent_deployment_id IS NULL OR parent_deployment_id = ''")
-    }
+    // Spec: GET /deployments returns ALL Deployment resources. paths/deployments.yaml
+    // does not include the `recursive` parameter or any top-level-only language,
+    // unlike paths/systems.yaml. Subdeployments must be retrievable from this
+    // canonical endpoint per paths/subdeployments.yaml ("individual members can
+    // also be retrieved by ID directly at the canonical Deployment resources
+    // endpoint"). No parent filter applied here.
+    _ = params.Recursive // accepted as a no-op alias; not a spec parameter
 }

Single one-block change. Closes #8 + fixes D1 + fixes D2 simultaneously.

Option B (preserve top-level default + implement recursive=true) is not recommended — it perpetuates the non-spec default, doesn't fix D1/D2 without extra logic, and embeds a non-spec parameter as the only correct way to use the canonical endpoint.

Revised acceptance criteria

Original AC list is fine; please add:

• GET /deployments?parent={parentId} returns matching subdeployments (currently 0 — D1)
• GET /deployments?id=<subdeployment-URI> returns the subdeployment (currently 0 — D2; spec-mandated)
• New code comment cites paths/deployments.yaml + paths/subdeployments.yaml so the rationale survives future "be consistent with /systems" refactors

Verdict

• Keep open. Defect is real and well-scoped; recommended Option A is correct.
• Severity: P2 as filed is reasonable. There is a defensible case for P1 because D2 violates an explicit spec sentence and breaks canonical URI lookup of subdeployments. I would not push for P1 unless a consumer hits D2 specifically — the fix is identical at either severity.
• Ownership: Upstream (SomethingCreativeStudios/connected-systems-go). Strong upstream PR candidate.
• No follow-up issues filed — D1 and D2 share the same one-line fix and belong in the /deployments only returns top-level deployments — subdeployments not discoverable without parent ID #8 PR.

Methodology note

First issue in the run where the spec sided with the report on its main thrust. Two specific claims still failed scrutiny (the comparative-server framing and the "no-op" implementation diagnosis), and two additional spec-violating defects surfaced from the same code path during empirical testing. Two diligence rules from this evaluation:

1. Never accept a comparative-server claim without checking the canonical OAS for both endpoints by name. Here, the OAS is the primary evidence — the spec author wrote different descriptions and different parameter sets for the two endpoints, and that asymmetry is the spec's actual position.
2. When a code branch is documented as a no-op via comment, run it before believing the comment. T2 took fifteen seconds and overturned a stated factual claim.

[Sam-Bolling]

Reopening — the previous closure was unintentional.

GitHub auto-closed this issue when commit d77e7ed landed on main because my commit body contained the phrase "One-block change closes #8 + D1 + D2", which GitHub parsed as a closing keyword. The intended meaning was descriptive ("a single fix would close #8 along with the two new defects D1/D2 found during evaluation"), not a directive — the commit only added research/evaluation docs, not a code fix.

The evaluation verdict (see docs/research/issue-evaluations/issue-008.md) stands: VALIDATED with corrections, plus two additional defects (D1: ?parent= returns 0; D2: ?id=<sub-URI> returns 0, which violates the explicit canonical-endpoint sentence in paths/subdeployments.yaml). The actual code fix has not been made yet; this issue should remain open until the fix lands.

Apologies for the noise.

[Sam-Bolling]

Closing as not planned, in agreement with the maintainer's call.

Resolution

@SomethingCreativeStudios on the upstream triage pass:

"Per Part 1 Req 21, deployments actually do match systems... redoc/openAPI is missing the recursive param, not fixing. I also don't have it on hand but the GitHub issue I brought forth on nested deleted I believes double confirms this."

This overrules the evaluation's spec reading, and on review the maintainer's call has the better authority. Posting a candid summary so the closure is auditable and so this disagreement is preserved for anyone who revisits the area later.

Where the evaluation went wrong on spec authority

The evaluation in issue-008.md §2 leaned on a comparative read of two OpenAPI YAML files — paths/systems.yaml (which contains "By default, only top level systems are included… unless the parent query parameter is set" and a recursive parameter ref) versus paths/deployments.yaml (which contains neither phrase nor parameter). The eval treated that asymmetry as a deliberate spec position: top-level-only-by-default for /systems, flat-listing-for-all for /deployments.

The maintainer is pointing to a higher-authority source the eval did not consult: OGC 23-001 (Part 1) Requirement 21, which is the normative prose-requirement counterpart to the OpenAPI annexes. His read is that Req 21 makes the /deployments behaviour match /systems (top-level-only-by-default with recursive to expand), and that the absence of a recursive parameter in paths/deployments.yaml is a documentation gap in the OpenAPI redoc rather than a deliberate semantic asymmetry. He also references a separate upstream "nested delete" issue he filed that he believes corroborates the same interpretation.

I did not verify Req 21 directly during the original evaluation (the methodology stopped at the OpenAPI bundle because that's the machine-readable surface clients validate against). Given the maintainer's spec authority and the consistency of Req 21 with the existing /systems implementation, his call stands. The eval's "the asymmetry is deliberate" framing should be treated as superseded.

This is a useful diligence-rule update: when an OpenAPI gap looks meaningful, cross-check the normative Part 1 / Part 2 prose requirements before treating the OAS asymmetry as definitive.

What live testing actually showed (preserved for the record)

Not to push back on the close, but to keep the empirical findings auditable in case anyone hits them downstream:

#	Request	numberMatched	Note
T1	GET /deployments	1	parent only, child invisible (consistent with Req 21 read)
T2	GET /deployments?recursive=true&limit=20	2	parent + child — works as a de-facto extension
T3	GET /deployments?parent={parentId}	0	spec-defined parent parameter returns nothing at top level
T4	GET /deployments/{parent}/subdeployments	1	child via subdeployments path
T5	GET /deployments/{childId}	200	child retrievable by direct ID path
T6	GET /deployments?id=urn:test:issue8:dep:child	0	canonical URI lookup of a subdeployment fails at /deployments

T3 and T6 are the live findings the eval labelled "D1" and "D2". They share the same root cause: applyFilters at internal/repository/deployment_repository.go:198 AND-conjoins the user-supplied filter (parent IN ? or id IN ? OR unique_identifier IN ?) with the top-level-only clause (parent_deployment_id IS NULL).

Under the maintainer's spec read (top-level-only is correct), T3 and T6 are still empirically odd — the spec's own ?parent= and ?id= filters returning 0 is at least a usability surface — but they are not spec-violations once the top-level-only default is accepted as canonical. They're the same behaviour /systems exhibits today, so any downstream client relying on URI lookup for subdeployments needs to use either ?recursive=true&id=... or hit /deployments/{parent}/subdeployments directly. That's consistent with how subsystems work, and is a known consequence of the Req 21 model.

The OpenAPI redoc gap (no recursive parameter documented at /deployments) is the surface that misled the eval; the maintainer noted he is not fixing that either. So users who consult only the redoc may continue to see the same asymmetry the eval saw — that's a caveat to flag for downstream tooling but not in scope for cs-go.

Mapping rationale → original claims

Claim from issue body	Outcome
"Default /deployments listing excludes subdeployments"	Factually true — but per maintainer's Req 21 read, this is the spec-mandated default, not a defect. Same as /systems.
"Be consistent with /systems returning all systems including subsystems"	Factually inverted — /systems is top-level-only too (verified at system_repository.go:386-389). Maintainer's read makes the two endpoints consistent in the opposite direction (both top-level-only).
"recursive=true for parentId==nil is a no-op"	Factually wrong — T2 confirms recursive=true produces the flat listing the issue wanted. The empty if branch is implementation-by-omission, not a missing implementation.
Severity P2 / P1	Moot — closed as not planned.
Option A: remove parent_deployment_id IS NULL from default branch	Rejected. Would diverge from /systems and from Req 21.
Option B: preserve top-level default + properly implement recursive=true	This is closer to the maintainer's actual implementation today, even if not formally documented in the OAS redoc. T2 shows it already works.

Cross-references

• Original evaluation: docs/research/issue-evaluations/issue-008.md (T1–T6 transcripts, source attestation, OpenAPI comparative read — now superseded by the maintainer's Part 1 Req 21 reading).
• Adjacent context: maintainer's separate upstream issue on nested deletes (referenced but not linked in his triage note) reportedly corroborates the same interpretation.
• Related theme — non-spec extensions vs canonical filters: closures on Research: Strict schema validation — requiring ALL declared fields in observation results #5, Research: Cross-resource references — @link objects only vs. flat @id strings for interoperability #6, ?uid= query parameter is silently ignored — server returns all resources unfiltered #7, all reflecting the same diligence rule (consult the canonical normative source before treating cross-server asymmetry as a defect).

Closing as not planned (won't fix), in agreement with the maintainer's spec-authority reading.