NOTICKET: correctly ensure entityCount is number #607
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
![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
🧹 Outside diff range and nitpick comments (2)
src/services/performanceDbApi.js (2)
109-109: Good fix for the entityCount validation!The change from
entityCount && entityCount >= 0toNumber.isInteger(entityCount) && entityCount >= 0correctly fixes the issue where an entityCount of 0 would fail the condition. The use ofNumber.isInteger()also adds proper type validation.For additional type safety, consider adding JSDoc type annotations for the entityCount parameter in the function signature.
Add this type annotation to the entityCounts array in the params object:
/** * @param {Object} params * @param {string[]} params.datasetsFilter * @param {Array<{dataset: string, resource: string, entityCount?: number}>} params.entityCounts */
Line range hint
41-44: Update the LpaOverview typedef as indicatedThe comment indicates that the typedef needs updating. Consider adding entityCount-related properties to properly reflect the data structure.
Update the typedef to include:
/** * @typedef {object} Dataset * @property {'Not submitted' | 'Error' | 'Needs fixing' | 'Warning' | 'Live' } status * @property {string} endpoint * @property {number} issue_count * @property {number} entity_count * @property {?string} error */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/services/performanceDbApi.js(1 hunks)test/unit/performanceDbApi.test.js(1 hunks)
🔇 Additional comments (3)
test/unit/performanceDbApi.test.js (2)
51-53: Test data properly validates zero entity count case
The addition of a test case with entityCount: 0 effectively validates the bug fix mentioned in the PR objectives, ensuring the query handles zero counts correctly.
58-59: Comprehensive test coverage with appropriate assertions
The additional assertions properly verify that both datasets are included in the query output, regardless of their entity count values.
Let's verify the test coverage for the related implementation:
✅ Verification successful
Test coverage is appropriate and correctly verifies dataset inclusion
The test correctly verifies that both datasets are included in the query output, regardless of their entity count values. Looking at the implementation in performanceDbApi.js, we can confirm:
- The
lpaOverviewQueryfunction properly handles both datasets with zero and non-zero entity counts - The test case covers both scenarios by using
entityCount: 10for dataset-three andentityCount: 0for dataset-four - The implementation ensures that datasets are included in the query via the
entityCountsSelectsarray, which is correctly populated regardless of the entity count value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for entityCount handling
# Expected: Find the implementation of lpaOverviewQuery and its entityCount validation
# Search for the implementation
ast-grep --pattern 'export function lpaOverviewQuery($_) {
$$$
}'
# Look for related tests
rg -l "entityCount.*[=><].*0" "test/"
Length of output: 107
Script:
#!/bin/bash
# Let's try a broader search to find the implementation and test files
# Search for files containing 'dataset-three' or 'dataset-four'
rg -l "dataset-(three|four)" .
# Search for performance-related query implementations
rg -l "lpaOverviewQuery|performanceDb" .
# Look for the test file content
cat test/unit/performanceDbApi.test.js
Length of output: 4593
Script:
#!/bin/bash
# Let's examine the implementation to verify the entityCount handling
cat src/services/performanceDbApi.js
Length of output: 14206
src/services/performanceDbApi.js (1)
Line range hint 111-116: Verify the SQL query construction with entityCount handling
The SQL query construction looks robust:
- Uses LEFT OUTER JOIN to include rows without matching entity_counts
- Handles missing values with COALESCE(ec.entity_count, 0)
- Includes a fallback for empty entityCountsSelects
Let's verify the query construction and execution:
What type of PR is this? (check all applicable)
Description
A condition would always fail for
entityCount = 0.The clauses inserted into the query were used with an outer join, so this bug did not narrow down the result set. In fact the results would still contain the correct value of zero in the
entity_countcolumn.Related Tickets & Documents
No ticket.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
Summary by CodeRabbit
Bug Fixes
entityCountin queries to ensure only integer values are accepted.Tests
lpaOverviewQueryfunction to include additional datasets and validate outputs.