Skip to content

Normalize ATProto CID links in GraphQL responses#40

Merged
Kzoeps merged 3 commits into
mainfrom
karma/hyper-358-image-ref-returns-as-maplinkbaf
Apr 30, 2026
Merged

Normalize ATProto CID links in GraphQL responses#40
Kzoeps merged 3 commits into
mainfrom
karma/hyper-358-image-ref-returns-as-maplinkbaf

Conversation

@Kzoeps
Copy link
Copy Markdown
Collaborator

@Kzoeps Kzoeps commented Apr 30, 2026

Summary

  • Normalize typed GraphQL Blob.ref values from ATProto { "$link": "..." } objects into plain CID strings.
  • Apply the same type-aware normalization to generated cid-link scalar and array fields without changing raw record JSON output.
  • Add schema-level regression coverage and a user-facing Changie fragment.

Tests

  • go test -v ./internal/graphql/types
  • go test -v ./internal/graphql/schema
  • go build -v ./...
  • make lint
  • DATABASE_URL=sqlite::memory: go test -v -race ./...
  • make test

Summary by CodeRabbit

Bug Fixes

  • Fixed GraphQL serialization of CID link fields. Blob reference and CID-link fields now correctly serialize as plain CID strings instead of improperly formatted values, ensuring proper GraphQL API responses.

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 30, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperindex-atproto-client Ready Ready Preview, Comment Apr 30, 2026 6:26am

Request Review

@railway-app
Copy link
Copy Markdown

railway-app Bot commented Apr 30, 2026

🚅 Deployed to the hyperindex-pr-40 environment in hyperindex-v2-setup

Service Status Web Updated (UTC)
hyperindex ✅ Success (View Logs) Web Apr 30, 2026 at 6:35 am
hyperindex-client ✅ Success (View Logs) Web Apr 30, 2026 at 6:28 am

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 30, 2026

📝 Walkthrough

Walkthrough

This PR fixes GraphQL serialization of ATProto CID links by adding runtime resolvers that normalize CID references. The resolvers support both plain CID strings and {"$link": string} object formats, extracting the underlying CID value for proper serialization in GraphQL responses.

Changes

Cohort / File(s) Summary
Changelog Entry
.changes/unreleased/fix-graphql-cid-link-serialization.yaml
Changelog entry documenting the bug fix for CID-link serialization in typed GraphQL responses, marking it as user-affecting.
Test Infrastructure
internal/graphql/schema/builder_test.go
Refactored test DB setup with setupSchemaRecordTestDB helper; added comprehensive regression test for CIDLink serialization across multiple query patterns (by collection, by URI, and via generic records resolver).
CID Link Extraction Logic
internal/graphql/types/mapper.go
Added extractCIDLinkString helper function that normalizes CID references from multiple formats (string, {"$link": string}, or map[string]string); modified Blob.ref field resolver to use this helper with explicit error handling.
Runtime Resolvers
internal/graphql/types/object.go
Added runtime resolvers for CID-link lexicon properties in GraphQL field construction; supports both singular TypeCIDLink fields and arrays of CID-link items, extracting CID strings while preserving array structure.
Unit Tests
internal/graphql/types/types_test.go
Comprehensive test suite validating Blob.ref and generated CID-link field resolvers; includes positive cases for string and $link object inputs, negative cases preventing map-formatted string serialization, and test helpers for schema execution.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Hop along, dear code, now CID links dance true,
No more {$link: whispers in GraphQL's view!
From maps we extract, normalize with care,
Plain strings in responses—plain magic in there!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Normalize ATProto CID links in GraphQL responses' accurately and concisely summarizes the main objective of the pull request, which is to normalize CID link representations across GraphQL responses.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch karma/hyper-358-image-ref-returns-as-maplinkbaf

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
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.

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

@railway-app railway-app Bot temporarily deployed to hyperindex-v2-setup / hyperindex-pr-40 April 30, 2026 06:26 Destroyed
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
internal/graphql/schema/builder_test.go (1)

898-921: ⚡ Quick win

Prefer the shared DB test helper for setup consistency.

Line 901–Line 920 manually bootstrap SQLite + migrations; please route this through the shared helper (internal/testutil/db.go) to keep DB-backed tests consistent and reduce setup drift.

As per coding guidelines: **/*_test.go: Prefer shared test helpers such as internal/testutil/db.go when adding DB-backed tests.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/graphql/schema/builder_test.go` around lines 898 - 921, Replace the
manual SQLite/migrations bootstrap in setupSchemaRecordTestDB with the shared
test DB helper from internal/testutil/db.go: remove sqlite.NewExecutor,
migrations.Run and the explicit exec.Close cleanup, call the shared helper
(e.g., SetupTestDB/NewTestDB from internal/testutil/db.go) to obtain the test DB
executor/ctx and register its cleanup, then use that executor to construct
repositories.NewRecordsRepository, BatchInsert the provided rec, build
resolver.Repositories and return resolver.WithRepositories(ctx, repos).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/graphql/schema/builder_test.go`:
- Around line 898-921: Replace the manual SQLite/migrations bootstrap in
setupSchemaRecordTestDB with the shared test DB helper from
internal/testutil/db.go: remove sqlite.NewExecutor, migrations.Run and the
explicit exec.Close cleanup, call the shared helper (e.g., SetupTestDB/NewTestDB
from internal/testutil/db.go) to obtain the test DB executor/ctx and register
its cleanup, then use that executor to construct
repositories.NewRecordsRepository, BatchInsert the provided rec, build
resolver.Repositories and return resolver.WithRepositories(ctx, repos).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 33d77fe3-be0f-4ff8-90f8-c1e493d35de0

📥 Commits

Reviewing files that changed from the base of the PR and between 7ac8235 and 72e4a2c.

📒 Files selected for processing (5)
  • .changes/unreleased/fix-graphql-cid-link-serialization.yaml
  • internal/graphql/schema/builder_test.go
  • internal/graphql/types/mapper.go
  • internal/graphql/types/object.go
  • internal/graphql/types/types_test.go

@Kzoeps Kzoeps merged commit 665e2a7 into main Apr 30, 2026
11 checks passed
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.

1 participant