feat: adds basic SQL prefilter for bid screening requests#3184
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR tightens bid-screening schemas, adds cluster-state hydration and criteria aggregation, implements DB-side candidate querying in BidScreeningRepository, refactors the service to consume candidates, updates tests, and removes legacy provider-inventory methods. ChangesBid-Screening Candidate Filtering Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3184 +/- ##
==========================================
- Coverage 63.69% 62.94% -0.76%
==========================================
Files 1089 1050 -39
Lines 26433 25431 -1002
Branches 6408 6248 -160
==========================================
- Hits 16837 16008 -829
+ Misses 8400 8238 -162
+ Partials 1196 1185 -11
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/bid-screening/http-schemas/bid-screening.schema.ts`:
- Around line 14-31: The StorageResourceSchema currently allows any
attributes[].key but must enforce the same provider-inventory SDL key format;
update validation so AttributeSchema.key (or the attributes array validation
inside StorageResourceSchema) matches the provider-inventory SDL key pattern
(use the exact regex/validator used in provider-inventory) and return Zod issues
on the specific attribute key (path like ["attributes", i, "key"]) with a clear
message when it fails; ensure the change reuses AttributeSchema if possible so
other schemas stay consistent and preserve OpenAPI metadata.
In
`@apps/provider-inventory/src/lib/hydrate-cluster-state/hydrate-cluster-state.ts`:
- Around line 16-43: The hydration functions assume well-formed input and can
throw when raw is malformed; make hydrateClusterState, hydrateNode and
hydratePair defensive: ensure cluster.nodes is an array before mapping (use
Array.isArray(cluster.nodes) ? cluster.nodes : []), ensure cluster.storage is a
plain object before Object.entries (fallback to {}), and in hydrateNode guard
node.gpu and node.gpu.quantity (use node.gpu?.quantity ?? 0 and node.gpu?.info ?
node.gpu.info : []) and default fields like storageClasses/cpus to [] when not
arrays; replace direct BigInt conversions in hydratePair with a safe conversion
helper (e.g., coerce to number/string then try/catch BigInt or default to 0) so
BigInt(pair.allocatable) and BigInt(pair.allocated) never throw on malformed
inputs.
In
`@apps/provider-inventory/src/repositories/bid-screening/bid-screening.repository.integration.ts`:
- Line 12: Replace the relative import of the repository with the backend source
path alias: change the import line that currently brings in AUDITOR and
BidScreeningRepository from "./bid-screening.repository" to use the `@src/`* alias
(e.g., import { AUDITOR, BidScreeningRepository } from
"@src/repositories/bid-screening/bid-screening.repository"), updating the module
specifier so the test uses the aliased backend source path.
In
`@apps/provider-inventory/src/repositories/bid-screening/bid-screening.repository.ts`:
- Line 11: Replace the relative import of aggregateCriteria and
BidScreeningCriteria from "./bid-screening.aggregator" with the backend source
path alias import using `@src` so the file uses the project aliasing convention;
update the import statement that references aggregateCriteria and type
BidScreeningCriteria in bid-screening.repository.ts to import from the
corresponding module via the `@src/`* alias (keeping the same exported symbols and
module name bid-screening.aggregator).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2a5fb9f-7083-48c2-823a-43a914f5396a
📒 Files selected for processing (13)
apps/api/src/bid-screening/http-schemas/bid-screening.schema.tsapps/provider-inventory/src/http-schemas/bid-screening.schema.tsapps/provider-inventory/src/lib/hydrate-cluster-state/hydrate-cluster-state.spec.tsapps/provider-inventory/src/lib/hydrate-cluster-state/hydrate-cluster-state.tsapps/provider-inventory/src/repositories/bid-screening/bid-screening.aggregator.spec.tsapps/provider-inventory/src/repositories/bid-screening/bid-screening.aggregator.tsapps/provider-inventory/src/repositories/bid-screening/bid-screening.repository.integration.tsapps/provider-inventory/src/repositories/bid-screening/bid-screening.repository.tsapps/provider-inventory/src/repositories/provider-inventory/provider-inventory.repository.integration.tsapps/provider-inventory/src/repositories/provider-inventory/provider-inventory.repository.spec.tsapps/provider-inventory/src/repositories/provider-inventory/provider-inventory.repository.tsapps/provider-inventory/src/services/bid-screening/bid-screening.service.spec.tsapps/provider-inventory/src/services/bid-screening/bid-screening.service.ts
💤 Files with no reviewable changes (2)
- apps/provider-inventory/src/repositories/provider-inventory/provider-inventory.repository.integration.ts
- apps/provider-inventory/src/repositories/provider-inventory/provider-inventory.repository.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/provider-inventory/src/repositories/bid-screening/bid-screening.aggregator.spec.ts (2)
5-5: ⚡ Quick winUse backend source alias import instead of relative path.
Switch Line 5 to
@src/*alias to match backend import rules.Suggested fix
-import { aggregateCriteria } from "./bid-screening.aggregator"; +import { aggregateCriteria } from "@src/repositories/bid-screening/bid-screening.aggregator";As per coding guidelines "apps/**/*.ts: Use path aliases
@src/*for source files and@test/*for test files in backend applications".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/provider-inventory/src/repositories/bid-screening/bid-screening.aggregator.spec.ts` at line 5, The test import currently uses a relative path; update the import in bid-screening.aggregator.spec.ts so it uses the backend source alias (e.g., replace the relative import of aggregateCriteria from "./bid-screening.aggregator" with the `@src/`* alias equivalent) to conform to backend rules; ensure the import still references the same exported symbol aggregateCriteria and that any test runner/tsconfig path mappings already support the `@src` alias.
26-31: ⚡ Quick winAdd max-per-replica assertions for the
count=0case.This test already defines the edge case; asserting
maxPerReplicaCpu/Memory/Gpustay0nwould lock in correct behavior and catch regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/provider-inventory/src/repositories/bid-screening/bid-screening.aggregator.spec.ts` around lines 26 - 31, The test "count=0 contributes nothing to totals" should also assert that per-replica maxima remain zero: update the spec that calls aggregateCriteria([makeUnit({... count: 0 })], makeRequirements()) to include expect(c.maxPerReplicaCpu).toBe(0n), expect(c.maxPerReplicaMemory).toBe(0n) and expect(c.maxPerReplicaGpu).toBe(0n); this ensures aggregateCriteria's max-per-replica behavior for the zero-count unit (referenced symbols: aggregateCriteria, makeUnit, makeRequirements) is locked in and will catch regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/provider-inventory/src/repositories/bid-screening/bid-screening.aggregator.ts`:
- Around line 46-48: The max-per-replica variables (maxPerReplicaCpu,
maxPerReplicaMemory, maxPerReplicaGpu) are being updated even for entries with
count === 0; change the comparisons so they only update when the item’s count is
greater than zero (e.g., guard the existing checks with count > 0), so use
conditions like if (count > 0 && cpu > maxPerReplicaCpu) and similarly for
memory and gpu to avoid zero-count entries inflating the maxima in the
aggregation logic in bid-screening.aggregator (variables: cpu, memory, gpu,
count, maxPerReplicaCpu, maxPerReplicaMemory, maxPerReplicaGpu).
---
Nitpick comments:
In
`@apps/provider-inventory/src/repositories/bid-screening/bid-screening.aggregator.spec.ts`:
- Line 5: The test import currently uses a relative path; update the import in
bid-screening.aggregator.spec.ts so it uses the backend source alias (e.g.,
replace the relative import of aggregateCriteria from
"./bid-screening.aggregator" with the `@src/`* alias equivalent) to conform to
backend rules; ensure the import still references the same exported symbol
aggregateCriteria and that any test runner/tsconfig path mappings already
support the `@src` alias.
- Around line 26-31: The test "count=0 contributes nothing to totals" should
also assert that per-replica maxima remain zero: update the spec that calls
aggregateCriteria([makeUnit({... count: 0 })], makeRequirements()) to include
expect(c.maxPerReplicaCpu).toBe(0n), expect(c.maxPerReplicaMemory).toBe(0n) and
expect(c.maxPerReplicaGpu).toBe(0n); this ensures aggregateCriteria's
max-per-replica behavior for the zero-count unit (referenced symbols:
aggregateCriteria, makeUnit, makeRequirements) is locked in and will catch
regressions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab3adc42-42bc-4eca-9d2a-e1ec2f75efc4
📒 Files selected for processing (13)
apps/api/src/bid-screening/http-schemas/bid-screening.schema.tsapps/provider-inventory/src/http-schemas/bid-screening.schema.tsapps/provider-inventory/src/lib/hydrate-cluster-state/hydrate-cluster-state.spec.tsapps/provider-inventory/src/lib/hydrate-cluster-state/hydrate-cluster-state.tsapps/provider-inventory/src/repositories/bid-screening/bid-screening.aggregator.spec.tsapps/provider-inventory/src/repositories/bid-screening/bid-screening.aggregator.tsapps/provider-inventory/src/repositories/bid-screening/bid-screening.repository.integration.tsapps/provider-inventory/src/repositories/bid-screening/bid-screening.repository.tsapps/provider-inventory/src/repositories/provider-inventory/provider-inventory.repository.integration.tsapps/provider-inventory/src/repositories/provider-inventory/provider-inventory.repository.spec.tsapps/provider-inventory/src/repositories/provider-inventory/provider-inventory.repository.tsapps/provider-inventory/src/services/bid-screening/bid-screening.service.spec.tsapps/provider-inventory/src/services/bid-screening/bid-screening.service.ts
💤 Files with no reviewable changes (2)
- apps/provider-inventory/src/repositories/provider-inventory/provider-inventory.repository.integration.ts
- apps/provider-inventory/src/repositories/provider-inventory/provider-inventory.repository.spec.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/api/src/bid-screening/http-schemas/bid-screening.schema.ts
- apps/provider-inventory/src/http-schemas/bid-screening.schema.ts
- apps/provider-inventory/src/lib/hydrate-cluster-state/hydrate-cluster-state.ts
- apps/provider-inventory/src/services/bid-screening/bid-screening.service.ts
- apps/provider-inventory/src/repositories/bid-screening/bid-screening.repository.ts
- apps/provider-inventory/src/lib/hydrate-cluster-state/hydrate-cluster-state.spec.ts
- apps/provider-inventory/src/repositories/provider-inventory/provider-inventory.repository.ts
- apps/provider-inventory/src/services/bid-screening/bid-screening.service.spec.ts
- apps/provider-inventory/src/repositories/bid-screening/bid-screening.repository.integration.ts
2a05397 to
3dd07f6
Compare
|
question: I noticed ip leases filters providers quite significantly, is this taken into account? |
Why
ref CON-341
What
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests