feat: add bypassDataItemFilter flag and convert to object parameters (PE-8173) #433
Conversation
…(PE-8173) * Rename bypassFilter to bypassBundleFilter for clarity * Add new bypassDataItemFilter flag that only bypasses ANS104_INDEX_FILTER * Convert all queue methods from positional to object parameters * Update system.queueBundle to accept QueueBundleOptions interface * Update DataImporter, Ans104Unbundler, and ANS-104 parser to use object params * Update admin API to support both new and legacy parameter formats * Update data verification worker to use new parameter structure * Update all unit tests to use new object parameter format * Maintain backward compatibility for legacy bypassFilter parameter The new implementation allows independent control of bundle and data item filtering, providing more granular control over the unbundling process. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis change refactors the filtering mechanism for bundle processing by replacing a single Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Route as /ar-io/admin/queue-bundle
participant System
participant DataImporter
participant Ans104Unbundler
participant Ans104Parser
Client->>API_Route: POST /ar-io/admin/queue-bundle (with bypassBundleFilter, bypassDataItemFilter)
API_Route->>System: queueBundle({ item, prioritized, bypassBundleFilter, bypassDataItemFilter })
System->>DataImporter: queueItem({ item, prioritized, bypassBundleFilter, bypassDataItemFilter })
DataImporter->>Ans104Unbundler: queueItem({ item, prioritized, bypassBundleFilter, bypassDataItemFilter })
Ans104Unbundler->>Ans104Parser: parseBundle({ ..., bypassDataItemFilter })
Ans104Parser-->>Ans104Unbundler: Processed bundle (with/without data item filter)
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
yarn install v1.22.22 ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
src/routes/ar-io.ts (1)
273-289: Boolean-type validation can never hit theundefinedbranchBecause
bypassBundleFilterreceives a default (= true) the derivedeffectiveBypassBundleFilteris always defined.
The... !== undefinedguard therefore always evaluates totrue, making theundefinedbranch dead code.Not harmful, but removing the unnecessary check would simplify the condition:
- if ( - effectiveBypassBundleFilter !== undefined && - typeof effectiveBypassBundleFilter !== 'boolean' - ) { + if (typeof effectiveBypassBundleFilter !== 'boolean') {src/workers/ans104-unbundler.test.ts (1)
71-73: MissingawaitonqueueItemmay hide async failures
queueItemreturns aPromise<void>; invoking it withoutawaitmeans any rejection would be unhandled and the test would still pass.-ans104Unbundler.queueItem({ item: mockItem, prioritized: false }); +await ans104Unbundler.queueItem({ item: mockItem, prioritized: false });Same pattern repeats in this block of tests.
src/lib/ans-104.ts (1)
284-291: Constructor overload expanded cleanly – consider documenting the new flag
bypassDataItemFilteris now part of the public contract ofparseBundle.
Please update any JSDoc / README snippet so integrators know the exact behaviour (bundle filter may still run while data-item filter is bypassed).src/workers/data-importer.test.ts (1)
167-173: Hard-coding both bypass flags tofalseis fine, but widen test coverageConsider adding another test where one or both flags are
true, to ensure they propagate throughdownload→queueItem→Ans104Unbundler.src/workers/data-verification.ts (3)
46-52: Type definition duplicated – lift to a sharedOptionstype?The same
{ item, prioritized?, bypassBundleFilter?, bypassDataItemFilter? }shape now appears in several modules.
Extracting aQueueBundleOptionsinterface (e.g. intypes.ts) would reduce drift as the parameter list evolves.
160-165:bypassDataItemFilternot forwardedYou set
bypassBundleFilter: truebut omitbypassDataItemFilter, relying on the defaultfalse.
If future callers ever need to tweak just the data-item flag, they’ll have to touch this code again.
Consider passing it explicitly (even asfalse) for clarity.
195-197:prioritizedomitted when re-queueing corrupted bundles
DataImporter.queueItemtreatsprioritizedas optional, but semantics suggest retries should stay prioritized.
Addprioritized: truefor consistency with the earlier call.src/workers/ans104-unbundler.ts (1)
110-120: Consider defaultingprioritizedtofalsein the parameter destructuring
prioritizedis later compared to the boolean literaltrue. Supplying a default avoids the extraundefinedcheck and communicates the intent more clearly.- async queueItem({ - item, - prioritized, + async queueItem({ + item, + prioritized = false,src/workers/data-importer.ts (2)
40-45: Makeprioritizedoptional inDataImporterQueueItemfor consistencyThe field is optional in callers, yet the interface declares it as mandatory (albeit
boolean | undefined).
Declaring it as optional (prioritized?: boolean) keeps the types aligned with usage and removes the confusingundefinedunion.- prioritized: boolean | undefined; + prioritized?: boolean;
84-94: Defaultprioritizedtofalseto avoid tri-state logicSame rationale as in the unbundler: defaulting removes the need for
undefinedchecks later while preserving current behaviour.- async queueItem({ - item, - prioritized, + async queueItem({ + item, + prioritized = false,src/system.ts (2)
614-619: Minor:prioritizedcan be marked optional inQueueBundleOptions
prioritizedalready has a default inqueueBundle, so marking it optional in the interface tightens type expectations.- prioritized?: boolean; + prioritized?: boolean;(This is purely cosmetic and can be skipped if interface consistency is preferred.)
676-687: Magic number-1for parent index could be extractedUsing a named constant (e.g.,
NO_PARENT_INDEX = -1) improves readability and reduces the chance of silent misuse elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/lib/ans-104.ts(4 hunks)src/routes/ar-io.ts(2 hunks)src/system.ts(4 hunks)src/workers/ans104-unbundler.test.ts(6 hunks)src/workers/ans104-unbundler.ts(6 hunks)src/workers/data-importer.test.ts(5 hunks)src/workers/data-importer.ts(5 hunks)src/workers/data-verification.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/workers/ans104-unbundler.test.ts (1)
Learnt from: karlprieb
PR: ar-io/ar-io-node#228
File: src/workers/ans104-unbundler.ts:41-45
Timestamp: 2024-10-31T17:48:59.599Z
Learning: In `src/workers/ans104-unbundler.ts`, when defining `isNormalizedBundleDataItem`, avoid checking for `null` values of `root_parent_offset` and `data_offset`, as `null` values signify that the item is a `NormalizedOptimisticDataItem`, not a `NormalizedBundleDataItem`.
src/workers/ans104-unbundler.ts (1)
Learnt from: karlprieb
PR: ar-io/ar-io-node#228
File: src/workers/ans104-unbundler.ts:41-45
Timestamp: 2024-10-31T17:48:59.599Z
Learning: In `src/workers/ans104-unbundler.ts`, when defining `isNormalizedBundleDataItem`, avoid checking for `null` values of `root_parent_offset` and `data_offset`, as `null` values signify that the item is a `NormalizedOptimisticDataItem`, not a `NormalizedBundleDataItem`.
🧬 Code Graph Analysis (4)
src/workers/data-verification.ts (2)
src/types.d.ts (1)
PartialJsonTransaction(49-61)src/system.ts (1)
QueueBundleResponse(610-613)
src/workers/data-importer.test.ts (1)
src/system.ts (1)
bundleDataImporter(600-606)
src/routes/ar-io.ts (1)
src/types.d.ts (1)
PartialJsonTransaction(49-61)
src/system.ts (2)
src/types.d.ts (1)
PartialJsonTransaction(49-61)src/workers/data-verification.ts (1)
QueueBundleResponse(33-36)
🔇 Additional comments (9)
src/routes/ar-io.ts (3)
260-266: DefaultingbypassBundleFiltertotruesilently relaxes filtering for callers who omit the flagPrior behaviour required an explicit
bypassFilter:trueto ignore bundle-level filtering; now the same result occurs when the flag is simply absent.
If any existing automation relied on “filter unless I explicitly say otherwise”, this change weakens that guarantee.Please double-check downstream consumers / scripts before merging.
If the relaxed default is intentional, consider calling it out in the API docs and bumping a minor version.
298-306:bypassDataItemFilterignored when delegating to/queue-txpathWhen
effectiveBypassBundleFilter === false, the request is downgraded to a TX-queue.
Any user-suppliedbypassDataItemFilteris silently discarded, which may surprise clients that set it.If this is intentional, consider returning a warning message; if not, propagate the flag (where technically feasible).
313-320: Object parameter construction looks goodNice adoption of the new
{ item, prioritized, bypassBundleFilter, bypassDataItemFilter }pattern – improves readability and future extensibility.src/workers/ans104-unbundler.test.ts (1)
118-123: Great coverage forbypassBundleFilterpathVerifying that
bypassDataItemFilterdefaults tofalsein the forwarded payload tightens confidence in the new flag handling. 👍src/lib/ans-104.ts (2)
352-353: Worker message extended correctlyPassing
bypassDataItemFilterthrough the worker message keeps the main/worker APIs symmetrical – nice.
490-492: Short-circuit keeps filter cost negligible when bypassing
if (bypassDataItemFilter || (await filter.match(...)))is the right precedence – avoids an unnecessary async call when bypassing.
Looks solid.src/workers/data-importer.test.ts (1)
108-112: UpdatedqueueIteminvocation aligns with new signatureCall site now uses the object-parameter style; tests compile under the new API – good catch.
src/workers/ans104-unbundler.ts (2)
135-140:prioritizedflag is discarded after deciding betweenunshiftandpushIf we ever need this flag again in the
unbundlephase (e.g., to influence metrics or logging), it will be unavailable because it is not forwarded into the queue payload.
That may be intentional, but worth a quick double-check.
185-187: Good propagation of the newbypassDataItemFilterflagThe flag is forwarded to
ans104Parser.parseBundle, keeping behaviour consistent with the refactor.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #433 +/- ##
===========================================
+ Coverage 72.88% 72.89% +0.01%
===========================================
Files 49 49
Lines 12526 12576 +50
Branches 727 727
===========================================
+ Hits 9129 9167 +38
- Misses 3391 3403 +12
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
Add bypassDataItemFilter flag and convert function parameters to object-based pattern for better maintainability and extensibility.
Changes
bypassDataItemFilterparameter to bypass data item filtering during bundle processingbypassFilterparameter in API endpointTest plan
🤖 Generated with Claude Code