Skip to content

Identified improvements for consent delegation and integration tests. #84

Description

@hasithakn

Follow-ups from PR #52 (Consent Delegation + group-ids/sort + Postgres support) review

Tracking issue for cleanup work identified while reviewing #52 before merge. #52 itself is not blocked on this — merge it, then work through this list. Each item is independent and can be fixed/checked off separately.

Code style / consistency

  • Non-standard transaction-abort pattern. authresource/service.go (CreateAuthResource/UpdateAuthResource) uses a new sentinel error (errAuthWriteAborted) to cancel a transaction mid-way and surface a specific error. No other ExecuteTransaction caller in the codebase does this — they validate before starting the transaction. Either align with the existing pattern or document why this case needed something different.
  • Silent config default breaks existing convention. config.go's applyDefaults() fills in RecordedState with "RECORDED" when a deployment's YAML omits it. Every other status-mapping field fails config validation instead of defaulting silently. Decide: should recorded_state be required like the others, or is the silent default intentional (e.g. for backward compat with old config files)? If intentional, say so in the comment.
  • Inconsistent case-insensitive comparisons. The same kind of status/type comparison is done 3 different ways across files touched by Add Consent Delegation Feature #52: strings.EqualFold, strings.ToUpper(a) == strings.ToUpper(b), and plain ==. Pick one idiom and use it consistently in these code paths.
  • Validator function takes pre-merged service state instead of a request DTO. ValidateAuthResourceTypeConstraints takes DB-model state the service assembles from a transactional read, unlike every other validator in the repo. Consider moving the merge logic into the service, or document this as an intentional new category of validator.
  • Sort direction is case-sensitive while a sibling filter isn't. parseConsentSorts only accepts lowercase asc/desc, but consentStatuses on the same endpoint is case-insensitive. Normalize sort direction too, or document the inconsistency in the API spec.
  • New query-param multiplicity rule inconsistent with existing filters. The new group-ids/sort params reject repeated params; existing filters silently accept repeats. Pick one behavior and apply it consistently.
  • Three different subprocess/error-handling idioms in the integration test harness. test_utils.go has one style for MySQL, a different helper for Postgres, a third raw style for SQLite. Extract one shared helper.

Correctness to double-check (may be a real bug, not just a missing test)

  • Custom auth-type search may be case-sensitive or not depending on the database. The authTypes filter does a plain SQL IN (...) with no explicit case handling, so the same query could return different results on MySQL vs. Postgres vs. SQLite. Decide intended behavior and enforce it explicitly if consistency across backends is required.

Test coverage gaps — delegation

  • No test proves the new row-locking actually prevents a race. Most important gap — nothing fires two concurrent authorization writes to prove GetByIDForUpdate works as intended.
  • ValidateAuthResourceTypeConstraints has no direct unit test — only exercised indirectly via integration tests.
  • Removing the last delegate_subject while its delegate remains is untested (only the reverse direction is tested).
  • Transitioning a consent from non-delegated to delegated (or back) via a single UpdateAuthResource call is untested.
  • Only a symmetric 2-delegate/2-subject ratio is tested — add an asymmetric case.
  • No end-to-end test sends a mixed-case type (e.g. "Delegate") through the real HTTP API.
  • No unit test exists for the recorded_state config default — no config_test.go at all in that package.
  • No test proves a RECORDED authorization gets properly cascaded on consent revoke/expiry.

Test coverage gaps — group-ids / sort

  • None of the sort parameter's error paths are tested (bad field, bad direction, malformed item, duplicate field, >3 fields, empty item, repeated sort param).
  • validityTime, updatedTime, consentType sort fields are never exercised by any test.
  • No test combines sort with delegation/authTypes filters and pagination together — the exact combination the query rewrite was meant to support.

Test coverage gaps — PostgreSQL support

  • No CI job runs the integration suite against any database — Postgres support is only manually verifiable.
  • No test would catch a Postgres-only SQL bug (e.g. pin buildExpiredConsentsQuery's placeholder conversion with a unit test — this PR already fixed one real bug here with no guarding test).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions