feat(pqlmanifest): implement S3 polling support for PQL manifest#2726
Merged
feat(pqlmanifest): implement S3 polling support for PQL manifest#2726
Conversation
StarpTech
requested review from
Noroth,
SkArchon,
devsergiy,
endigma and
jensneuse
as code owners
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Contributor
WalkthroughAdds ProviderRegistry, introduces ManifestFetcher and StorageFetcher, refactors router bootstrap to use the registry, reworks persisted-operations manifest polling (conditional reads, decompression, changed detection), updates storage client signatures, and adds tests and docs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2726 +/- ##
==========================================
- Coverage 63.35% 60.86% -2.50%
==========================================
Files 249 251 +2
Lines 26671 26763 +92
==========================================
- Hits 16898 16289 -609
- Misses 8408 9117 +709
+ Partials 1365 1357 -8
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/core/router.go`:
- Around line 1277-1301: The code can panic if storageProviderID is set but
pClient is nil, so before constructing the storageFetcher
(pqlmanifest.NewStorageFetcher) check pClient for nil and return a clear error
(or fallback) if it is nil; specifically verify pClient != nil before calling
pClient.ReadManifest and before creating storageFetcher/poller (references:
storageProviderID, pClient, pClient.ReadManifest, storageFetcher,
pqlmanifest.NewStorageFetcher, pqlmanifest.NewPoller, poller.FetchInitial,
r.pqlPoller) and fail fast with a descriptive fmt.Errorf when misconfigured.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0939e58f-06bc-42cb-9bb2-88b8a6432714
📒 Files selected for processing (4)
router/core/router.gorouter/internal/persistedoperation/pqlmanifest/poller.gorouter/internal/persistedoperation/pqlmanifest/storage_fetcher.gorouter/internal/persistedoperation/pqlmanifest/storage_fetcher_test.go
…ests and registry support
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
router/core/router.go (1)
1235-1253:⚠️ Potential issue | 🟠 MajorReject bad explicit provider IDs even without a graph token.
An explicit
Storage.ProviderIDis only rejected inside ther.graphApiToken != ""branch. With a misspelled provider ID and no token, this falls through toreturn nil, nil, nil, which silently disables persisted-operation storage and leaves the manifest path without a reader instead of failing fast on the config error.🔧 Suggested fix
- if r.graphApiToken != "" { - if providerID != "" { - return nil, nil, fmt.Errorf("unknown storage provider id '%s' for persisted operations", providerID) - } + if providerID != "" { + return nil, nil, fmt.Errorf("unknown storage provider id %q for persisted operations", providerID) + } + + if r.graphApiToken != "" { c, err := cdn.NewClient(r.cdnConfig.URL, r.graphApiToken, cdn.Options{ Logger: r.logger, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/router.go` around lines 1235 - 1253, The code only rejects an explicit providerID inside the r.graphApiToken != "" branch causing a misspelled Storage.ProviderID to silently disable persisted operations when no token is present; add a guard at the start (check providerID != "" before the graphApiToken branch) that returns an explicit error (same format used elsewhere, e.g. fmt.Errorf("unknown storage provider id '%s' for persisted operations", providerID)) so the function fails fast when Storage.ProviderID/providerID is set but unknown; keep the existing CDN client creation (r.graphApiToken, cdn.NewClient, r.logger.Debug, return c, c.ReadManifest) logic unchanged for the token-present case.
🧹 Nitpick comments (1)
router/core/router.go (1)
1258-1277: Return early before resolving Redis storage when APQ is disabled.The disabled path currently still resolves/logs the Redis provider and may allocate a client that gets discarded immediately. Moving the
Enabledguard to the top keeps disabled APQ side-effect free.♻️ Suggested refactor
func (r *Router) buildAPQClient(registry *ProviderRegistry) (apq.Client, error) { + if !r.automaticPersistedQueriesConfig.Enabled { + return nil, nil + } + var kvClient apq.KVClient if provider, ok := registry.Redis(r.automaticPersistedQueriesConfig.Storage.ProviderID); ok { c, err := apq.NewRedisClient(&apq.RedisOptions{ Logger: r.logger, StorageConfig: &provider, @@ ) } - - if !r.automaticPersistedQueriesConfig.Enabled { - return nil, nil - } apqClient, err := apq.NewClient(&apq.Options{ Logger: r.logger, ApqConfig: &r.automaticPersistedQueriesConfig,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/router.go` around lines 1258 - 1277, In buildAPQClient, the APQ disabled path still resolves and logs the Redis provider; move the guard checking r.automaticPersistedQueriesConfig.Enabled to the top of Router.buildAPQClient so you return early when APQ is disabled before calling registry.Redis or apq.NewRedisClient, preventing allocation of a Redis client and avoiding r.logger.Info side effects when not needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/core/provider_registry.go`:
- Around line 20-55: NewProviderRegistry currently only checks per-type maps
(s3, cdn, redis, fileSystem) for duplicate IDs, allowing the same ID to exist in
multiple provider types; update NewProviderRegistry to enforce global uniqueness
by tracking a single set of seen IDs (e.g., a map[string]struct{}) and checking
it before inserting into any per-type map so that any duplicate ID across types
returns an explicit error (ambiguity) referencing the conflicting ID; apply the
same global-check logic used when populating s3, cdn, redis, and fileSystem (and
the similar code region later at the other occurrence) to ensure provider IDs
are globally unique.
In `@router/internal/persistedoperation/operationstorage/s3/client.go`:
- Around line 121-145: ReadManifest currently uses the caller-supplied
modifiedSince and local time for re-polling which causes races; change
ReadManifest to return object metadata (Last-Modified timestamp and/or ETag)
along with the *pqlmanifest.Manifest so callers can use storage-provided
timestamps for If-Modified-Since/ETag checks. Concretely: update the
ReadManifest signature to return (*pqlmanifest.Manifest, time.Time, string,
error) (or similar), call the S3 API to fetch metadata (use c.client.StatObject
or minioReader.Stat()/GetObjectInfo after GetObject) to read LastModified and
ETag, keep the existing 304 handling, return the parsed manifest plus the
object's LastModified and ETag, and then update the higher-level fetcher to
store and reuse that LastModified/ETag instead of time.Now() for subsequent
modifiedSince checks.
In `@router/internal/persistedoperation/PO_MANIFEST_S3_GUIDE.md`:
- Around line 12-18: The docs currently say the router resolves the manifest at
<object_prefix>/manifest.json but mention support for .gz/.zst without stating
how to select a compressed manifest; update the PO_MANIFEST_S3_GUIDE.md to
document the configuration option or pattern for choosing a compressed manifest
(e.g., explain that the router will attempt <object_prefix>/manifest.json, then
<object_prefix>/manifest.json.gz and/or <object_prefix>/manifest.json.zst based
on the object key suffix or provide the explicit config field for the compressed
manifest path), and link this behavior to the existing `object_prefix`/manifest
lookup and `provider_id: s3` so readers know how to specify a compressed
manifest or remove the compression note if no config/behavior exists.
- Around line 49-53: Clarify that "manifest is authoritative" applies only to
storage-backed, hash-only lookups: when a request contains only an operation
hash and relies on S3/CDN-backed storage, missing hashes in the manifest cause
immediate rejection and per-request fetches are disabled; explicitly call out
the exceptions — if LogUnknown is enabled or if APQ receives a full query body
(i.e., not a hash-only request), the router will still accept/record or fetch
the operation as before — and mention the relevant symbols LogUnknown and APQ so
readers can locate the runtime/config behavior.
---
Duplicate comments:
In `@router/core/router.go`:
- Around line 1235-1253: The code only rejects an explicit providerID inside the
r.graphApiToken != "" branch causing a misspelled Storage.ProviderID to silently
disable persisted operations when no token is present; add a guard at the start
(check providerID != "" before the graphApiToken branch) that returns an
explicit error (same format used elsewhere, e.g. fmt.Errorf("unknown storage
provider id '%s' for persisted operations", providerID)) so the function fails
fast when Storage.ProviderID/providerID is set but unknown; keep the existing
CDN client creation (r.graphApiToken, cdn.NewClient, r.logger.Debug, return c,
c.ReadManifest) logic unchanged for the token-present case.
---
Nitpick comments:
In `@router/core/router.go`:
- Around line 1258-1277: In buildAPQClient, the APQ disabled path still resolves
and logs the Redis provider; move the guard checking
r.automaticPersistedQueriesConfig.Enabled to the top of Router.buildAPQClient so
you return early when APQ is disabled before calling registry.Redis or
apq.NewRedisClient, preventing allocation of a Redis client and avoiding
r.logger.Info side effects when not needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 605e5649-4675-4578-a4ce-3066422998f6
📒 Files selected for processing (15)
router-tests/operations/pql_manifest_test.gorouter/core/init_config_poller.gorouter/core/provider_registry.gorouter/core/provider_registry_test.gorouter/core/router.gorouter/core/router_config.gorouter/internal/persistedoperation/PO_MANIFEST_S3_GUIDE.mdrouter/internal/persistedoperation/client.gorouter/internal/persistedoperation/operationstorage/cdn/client.gorouter/internal/persistedoperation/operationstorage/fs/client.gorouter/internal/persistedoperation/operationstorage/s3/client.gorouter/internal/persistedoperation/operationstorage/s3/client_test.gorouter/internal/persistedoperation/pqlmanifest/poller.gorouter/internal/persistedoperation/pqlmanifest/storage_fetcher.gorouter/internal/persistedoperation/pqlmanifest/storage_fetcher_test.go
💤 Files with no reviewable changes (2)
- router/internal/persistedoperation/operationstorage/fs/client.go
- router/internal/persistedoperation/client.go
🚧 Files skipped from review as they are similar to previous changes (2)
- router/internal/persistedoperation/pqlmanifest/poller.go
- router/internal/persistedoperation/pqlmanifest/storage_fetcher.go
SkArchon
approved these changes
Apr 2, 2026
Contributor
SkArchon
left a comment
SkArchon
left a comment
There was a problem hiding this comment.
approved, but make sure to resolve / dismiss the coderabbit issues highlighted.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/internal/persistedoperation/PO_MANIFEST_S3_GUIDE.md`:
- Line 61: Update the documentation sentence that currently refers to a fixed
"manifest.json" to use a generic, configurable filename reference: mention that
the manifest file is given by the configured manifest.file_name (which may
include compression suffixes) instead of hardcoding "manifest.json"; adjust the
schema section wording to say "the manifest file (configured via
manifest.file_name)" or similar so it accurately reflects that the filename is
configurable and may be compressed.
- Line 80: Update the `revision` field description to replace the fragment "Can
be any non-empty string" with a complete sentence; e.g., change it to "It can be
any non-empty string (e.g. a git SHA, timestamp, or UUID)." Ensure the full
sentence reads smoothly with the preceding sentence and retains the requirement
that the value must be changed whenever operations are updated.
In `@router/pkg/config/config.schema.json`:
- Around line 206-210: The "file_name" string property in config.schema.json
currently allows empty values which can break manifest key resolution; update
the "file_name" schema (the property named "file_name") to disallow empty
strings by adding "minLength": 1 to its definition so validation fails for ""
while keeping the existing "default": "manifest.json".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d46966a6-dc6d-4338-8d4a-c8a5c87fc2ea
📒 Files selected for processing (7)
router/core/router.gorouter/internal/persistedoperation/PO_MANIFEST_S3_GUIDE.mdrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
✅ Files skipped from review due to trivial changes (3)
- router/pkg/config/fixtures/full.yaml
- router/pkg/config/testdata/config_defaults.json
- router/pkg/config/testdata/config_full.json
🚧 Files skipped from review as they are similar to previous changes (1)
- router/core/router.go
5 tasks
Noroth
reviewed
Apr 2, 2026
| return fmt.Errorf("failed to create CDN client: %w", err) | ||
| } | ||
| pClient = c | ||
| providerID := r.persistedOperationsConfig.Storage.ProviderID |
Contributor
There was a problem hiding this comment.
Just a question, are empty "" storage provider IDs allowed? At the end of the function we are returning nil, nil, nil when nothing was found. If providerID != "" should we log a warning at least?
This was referenced Apr 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.