Add public REST search API for document discovery - #1136
Conversation
Enhance SEO and AI discoverability: - Expand JSON-LD in index.html with @graph containing WebApplication, Organization, SoftwareSourceCode schemas plus SearchAction - Add GET /api/search/ endpoint for crawlers and lightweight integrations that prefer simple HTTP over GraphQL/MCP (supports global and corpus-scoped search with vector search fallback to text matching) - Document the search API in llms.txt and llms-full.txt - Fix manifest.json branding from "Create React App Sample" to OpenContracts - Add MAX_SEARCH_RESULTS constant to discovery constants - Add comprehensive tests for the search endpoint (15 test cases) https://claude.ai/code/session_015NyrhCvpdCxtju7jhWwYmP
Uncomment googlebot, bingbot, and duckduckbot in the social media crawler list so the Cloudflare Worker serves pre-rendered OG meta tags to search engine crawlers (improving SEO for deep-linked pages). Add corresponding test cases for the three newly enabled bots. https://claude.ai/code/session_015NyrhCvpdCxtju7jhWwYmP
Code ReviewOverall this is a well-structured addition — clean endpoint design, good documentation, and a solid test suite foundation. A few issues worth addressing before merge: Bug: Document visibility in tests and production logicTest setup missing
cls.public_doc = Document.objects.create(
title="Sample Indemnity Agreement",
creator=cls.owner,
# is_public not set → defaults to False
)So Deeper design concern: In For corpus-scoped search, it would be more correct to skip the document-level visibility check (since the corpus access was already verified) and query documents directly via # Instead of:
Document.objects.visible_to_user(user).filter(id__in=corpus_doc_ids)
# Consider:
Document.objects.filter(id__in=corpus_doc_ids)For the global search, filtering via Performance:
|
…e.legal Update the canonical URL in index.html OG tags and JSON-LD, and in the llms.txt / llms-full.txt project documentation links. https://claude.ai/code/session_015NyrhCvpdCxtju7jhWwYmP
Code Review: Add public REST search API for document discoveryOverall this is well-structured and the feature is a useful addition. A few issues worth addressing before merge, including one that will cause test failures. Bug: Test documents not visible to anonymous usersFile:
cls.public_doc = Document.objects.create(
title="Sample Indemnity Agreement",
description="Contains standard indemnification language",
creator=cls.owner,
# missing: is_public=True
)This will cause cls.public_doc = Document.objects.create(
title="Sample Indemnity Agreement",
description="Contains standard indemnification language",
creator=cls.owner,
is_public=True,
)Performance:
|
| Issue | Severity |
|---|---|
Test documents not is_public=True → 2 tests will fail |
High |
select_related() without arguments |
Medium |
AttributeError silently swallowed |
Medium |
| Authenticated user always treated as anonymous | Low / design question |
| No rate limiting | Low |
| Missing CHANGELOG entry | Low |
| Document corpus-first ordering | Nit |
…urce.legal Rename all remaining references to the old domain across production config, Auth0 defaults, Traefik routing rules, CI workflows, documentation, sample env files, and tests. Add new migration 0005 to update the Django Site domain (preserving migration 0003 untouched for existing databases). https://claude.ai/code/session_015NyrhCvpdCxtju7jhWwYmP
|
PR Review: Add public REST search API for document discovery Overall this is a well-structured PR with good test coverage and a clear purpose. The domain rename is clean and consistent. A few issues worth addressing before merge. Critical: Traefik routing gap The web-secure-router in traefik.yml only forwards /graphql, /admin, /mcp, /sse, /robots.txt, /llms.txt, /llms-full.txt, /sitemap.xml, /.well-known to Django. /api/ is not listed, so the frontend-secure-router (priority 1) matches /api/search/ first and sends it to the frontend static file server. The endpoint will 404 in production. The web-secure-router rule needs PathPrefix for /api/ added alongside /graphql, /admin, etc. Worth also verifying api/health/ works in production. Performance: select_related() without arguments in _search_global select_related() with no arguments JOINs every FK/OneToOne on Document. None of those related fields appear in the result dict. Remove it or replace with .only() scoped to the needed fields. Code quality: Over-broad exception swallowing in _search_within_corpus Catching AttributeError silently swallows programming errors like renamed methods or missing attributes. Narrow to only the expected exceptions or add a logger.debug() call. The RuntimeError branch already logs properly; the bare pass is inconsistent. Design: Global search exhausts limit with corpuses before documents If the corpus query returns the full limit, remaining = 0 and the document query is skipped entirely. A search matching many corpuses will silently suppress all document results. Consider a fixed split (e.g. limit // 2 per type) or interleaving by a common ranking signal. Test gaps test_limit_capped_at_max only asserts status_code == 200. It should also verify the result count is bounded by MAX_SEARCH_RESULTS. test_result_structure checks type/slug/title but not description or similarity_score, both documented in llms-full.txt as part of the API contract. Minor observations
What is good
|
- Add discovery_endpoint_served PostHog event to all SEO/crawler endpoints (robots.txt, llms.txt, llms-full.txt, sitemap.xml, .well-known/mcp.json, /api/search/) with user_agent tracking - Fix Traefik routing gap: add PathPrefix(/api) to web-secure-router and frontend-secure-router exclusion so /api/* routes to Django - Replace select_related() (no args) with .only() scoped to needed fields in _search_global to avoid unnecessary JOINs - Narrow exception handling in _search_within_corpus: remove AttributeError catch, add debug logging for ValueError/TypeError - Fix global search limit split: reserve half slots for each type so corpus results can't suppress all document results - Strengthen test_limit_capped_at_max to verify result count - Add description and similarity_score assertions to test_result_structure - Document anonymous-only evaluation and similarity_score null contract https://claude.ai/code/session_015NyrhCvpdCxtju7jhWwYmP
The DocumentPath model requires version_number (non-nullable integer). The SearchApiTest.setUpTestData was creating DocumentPath instances without it, causing IntegrityError in pytest. https://claude.ai/code/session_015NyrhCvpdCxtju7jhWwYmP
PR Review: Add public REST search API for document discoveryOverall this is a clean, well-structured addition. The domain rename is consistent and correct throughout. The search API is well-documented with solid test coverage. A few issues worth addressing before merge: Bug: _search_global can exceed the requested limit In _search_global the doc_limit math can return more results than limit when the limit is small. With limit=1: corpus_limit=max(0,1)=1, doc_limit=max(0,1)=1. After corpus takes its slot, remaining=0, but doc_limit=max(0,1)=1 — so up to 2 results for limit=1. The test_limit_parameter test likely passes only because ?q=indemnification does not match 'Sample Indemnity Agreement' (different word), but the logic is wrong. Fix: use remaining directly as the doc slice limit. Security: No rate limiting on the new endpoint Every other crawler-facing endpoint references RATE_LIMIT_REQUESTS, but search_api has none. A search endpoint triggering DB queries and potentially ML embedding lookups is a more attractive DoS target than static text files. Consider applying the same rate-limit middleware used for /mcp/, or at minimum a @cache_page decorator for identical queries. Exception handling in vector search is too narrow The except clauses only catch ValueError, TypeError, and RuntimeError. corpus.embed_text() and Document.objects.search_by_embedding() can raise pgvector/psycopg errors, AttributeError if no embedder is configured, etc. An unhandled exception here returns a 500 to the caller. A broader except Exception as exc with a debug log is safer for a public-facing fallback path. Telemetry helper not guarded against failures _record_discovery_event is now injected into robots.txt, sitemap.xml, llms.txt, etc. If PostHog is misconfigured and record_event raises, it breaks those endpoints. Wrapping the body in try/except Exception: pass would prevent telemetry failures from cascading. Minor: public_corpus_ids second queryset worth a comment In _search_global a fresh Corpus.objects.visible_to_user(user) queryset is used for the document filter — intentionally fetching all public corpus IDs, not just title-matched ones. This is correct but reads as a potential oversight. A one-line comment ('re-query to get all public corpus IDs, not just title-matched ones') would make intent clear. What looks good:
|
|
PR Review summary - 8 items found. See below for details. |
|
PR Review: Add public REST search API for document discovery Overall this is well-structured and the test coverage is solid. The domain rename is comprehensive. A few issues worth addressing before merge: Bug 1 - Vector search early return bypasses text fallback In _search_within_corpus, if embed_text succeeds and query_vector is truthy, the function returns immediately after the vector search even when doc_results is empty. Text fallback is only reached on exceptions, not on empty vector results. A corpus with embeddings but no matching documents returns empty even when a text match exists. Fix: only return early if results is non-empty, then fall through to text search. Bug 2 - Total results can exceed limit in _search_global With limit=1: corpus_limit=1, doc_limit=max(0,1)=1. After 1 corpus result, remaining=0 and doc_limit=max(0,1)=1 - up to 2 items returned for a limit=1 request. Consider adding results = results[:limit] before returning, and documenting the actual guarantee. Bug 3 - Migration dependency may be incorrect in 0005_update_site_domain.py The dependency on sites/0004_alter_options_ordering_domain refers to the django.contrib.sites built-in migration, not the previous migration in this app. Verify the app label and add a self-referential dependency to guarantee ordering within the custom app migrations. Perf 4 - visible_to_user evaluated twice in _search_global Once for corpus text-search, again to build public_corpus_ids. Both round-trips hit the database. Either reuse the IDs from the first list() call, or pass the queryset without list() to keep it as a SQL subquery. Perf 5 - Missing rate limiting on a public unauthenticated endpoint Every other discovery endpoint is cache-paged or rate-limited. MAX_SEARCH_RESULTS caps response size but not request frequency - this endpoint can enumerate all public content cheaply. Consider IP-level rate limiting (the MCP endpoint has a working pattern) or a short @cache_page for repeated identical queries. Perf 6 - Overly narrow exception handling in vector search path Only ValueError, TypeError, and RuntimeError are caught. ORM/connection errors and pgvector-specific exceptions will surface as 500s on a public endpoint. Add a broad except Exception as a final catch to ensure text fallback is always attempted. Minor 7 - _record_discovery_event fires before validation succeeds The telemetry call happens before the corpus lookup, so 404 responses from private/missing corpuses are counted as successful hits. Move the call to after results are determined. Minor 8 - Inline import in setUpTestData DocumentPath is imported inside the method body. Move it to the top-level imports alongside Document. What is Good
|
…riendliness-S7jUb
…ling - Fix FieldError in _search_global: use 'path_records' (correct reverse relation from Document to DocumentPath) instead of 'document_paths' - Add is_public=True to test public_doc so visible_to_user(AnonymousUser) finds it - In _search_within_corpus, skip document-level visible_to_user since corpus access was already verified - Fix vector search early return: fall through to text search when vector results are empty instead of returning immediately - Broaden exception handling in vector search to catch all exceptions with debug logging, preventing 500s on the public endpoint - Guard _record_discovery_event with try/except to prevent telemetry failures from breaking discovery endpoints - Move telemetry call after input validation in search_api - Cap _search_global results at limit to prevent exceeding requested count - Move DocumentPath import to top-level in test file
| "user_agent": user_agent, | ||
| }, | ||
| ) | ||
| except Exception: |
|
test |
Code ReviewOverall this is a well-structured PR. The domain rename is clean and consistent, the new search endpoint is useful, and test coverage is solid. A few issues worth addressing before merge. Breaking Change: AUTH0_ADMIN_CLAIM_NAMESPACE default The change to Missing Rate Limiting on Search Endpoint
Telemetry Fires Before Corpus Validation In Broad The
In
Migration Dependency Could Be Clearer
Missing CHANGELOG Update Per Minor: The test verifies Minor: The test asserts What's Working Well
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…riendliness-S7jUb Add public REST search API for document discovery
Summary
This PR adds a new public REST search API endpoint (
/api/search/) that enables crawlers, AI agents, and lightweight integrations to search across public corpuses and documents without requiring GraphQL or MCP connections.Key Changes
New Search API Endpoint (
GET /api/search/)q(required),corpus(optional),limit(optional, capped at MAX_SEARCH_RESULTS)Dual Search Strategies
Documentation Updates
llms.txtandllms-full.txtdiscovery endpointsSEO & Discovery Improvements
frontend/index.htmlwith SearchAction and SoftwareSourceCode typesComprehensive Test Coverage
Implementation Details
MAX_SEARCH_RESULTSconstant)visible_to_user()queryset filtering to enforce access controlqparameter; missing/empty queries return 400 errorshttps://claude.ai/code/session_015NyrhCvpdCxtju7jhWwYmP