Optimised SQL query for exporting members #22005
Conversation
|
Deployed to staging with ID: |
WalkthroughThe changes introduce a new filtering mechanism for member data retrieval in the Ghost CMS export service. A variable Changes
Sequence DiagramsequenceDiagram
participant Exporter as Member Exporter
participant DB as Database
Exporter->>+DB: Check for Filters
alt Has Filters
Exporter->>+DB: Fetch Filtered Member IDs
DB-->>-Exporter: Filtered Member IDs
Exporter->>+DB: Fetch Member Details
else No Filters
Exporter->>+DB: Fetch All Members
end
DB-->>-Exporter: Member Details
Exporter->>Exporter: Process Tiers and Labels
Poem
Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
ghost/core/core/server/services/members/exporter/query.js (1)
84-100: Optimize performance by using lookup maps for products and labelsUsing
.find()within a loop can lead to performance issues, especially with large datasets. Consider creating a lookup map forallProductsandallLabelsto improve the efficiency of your code.Example Implementation:
Create lookup maps outside the loop:
const productMap = new Map(allProducts.map(product => [product.id, product])); const labelMap = new Map(allLabels.map(label => [label.id, label]));Refactor the tier and label mapping:
const tierDetails = tierIds.map((id) => { const tier = productMap.get(id); if (tier) { return { name: tier.get('name') }; } }).filter(Boolean); const labelDetails = labelIds.map((id) => { const label = labelMap.get(id); if (label) { return { name: label.get('name') }; } }).filter(Boolean);
|
Deployed to staging with ID: |
1 similar comment
|
Deployed to staging with ID: |
vershwal
force-pushed
the
exportMemberSQLOptimisation
branch
from
ba67fcd to
ad16d4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/core/server/services/members/exporter/query.js (2)
34-44: Optimize product and label retrieval for filtered cases.When
hasFilteris true, consider fetching only the required products and labels based on the filtered member IDs to improve performance with large datasets.const allProducts = await knex('products') + .modify((query) => { + if (hasFilter) { + query.whereIn('id', function() { + this.select('product_id') + .from('members_products') + .whereIn('member_id', ids) + .distinct(); + }); + } + }) .select('id', 'name') .then(rows => rows.reduce((acc, product) => { acc[product.id] = product.name; return acc; }, {})); const allLabels = await knex('labels') + .modify((query) => { + if (hasFilter) { + query.whereIn('id', function() { + this.select('label_id') + .from('members_labels') + .whereIn('member_id', ids) + .distinct(); + }); + } + }) .select('id', 'name') .then(rows => rows.reduce((acc, label) => { acc[label.id] = label.name; return acc; }, {}));
Line range hint
13-23: Remove commented-out code.The commented-out code block appears to be from the old implementation and should be removed to maintain code cleanliness. If needed, this code can be referenced from version control history.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/core/server/services/members/exporter/query.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Unit tests (Node 20.11.1)
🔇 Additional comments (3)
ghost/core/core/server/services/members/exporter/query.js (3)
46-52: LGTM! Clean and efficient member query implementation.The query is well-structured with appropriate field selection and filter handling.
54-70: Ensure database compatibility of GROUP_CONCAT function.The use of GROUP_CONCAT in the queries may not be compatible across all database systems, particularly PostgreSQL which uses STRING_AGG instead.
89-114: LGTM! Robust data mapping with proper null handling.The implementation efficiently maps related data using appropriate data structures and includes proper null checks with the
?operator. The use of Map and Set for lookups is a good performance optimization.
|
Deployed to staging with ID: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/core/server/services/members/exporter/query.js (2)
Line range hint
22-34: Remove commented codeThe commented code block appears to be obsolete and should be removed to maintain code cleanliness.
🧰 Tools
🪛 eslint
[error] 149-149: Operator '/' must be spaced.
(space-infix-ops)
149-149: Fix operator spacingAdd spaces around the division operator.
-logging.info('[MembersExporter] Total time taken: ' + (Date.now() - start)/1000 + 's'); +logging.info('[MembersExporter] Total time taken: ' + (Date.now() - start) / 1000 + 's');🧰 Tools
🪛 eslint
[error] 149-149: Operator '/' must be spaced.
(space-infix-ops)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/core/server/services/members/exporter/query.js(2 hunks)
🧰 Additional context used
🪛 eslint
ghost/core/core/server/services/members/exporter/query.js
[error] 149-149: Operator '/' must be spaced.
(space-infix-ops)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Lint
- GitHub Check: i18n
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Ghost-CLI tests
🔇 Additional comments (10)
ghost/core/core/server/services/members/exporter/query.js (10)
4-4: LGTM! Good addition of logging moduleAdding structured logging will help with monitoring and debugging the export process.
40-44: LGTM! Efficient data structure transformationConverting products to a lookup map will provide O(1) access time during processing.
56-62: LGTM! Clean and efficient query structureThe query is well-organized with proper filtering conditions.
68-75: Ensure database compatibility ofGROUP_CONCATfunctionThe use of
GROUP_CONCATmay not be compatible across all database systems. PostgreSQL usesSTRING_AGGinstead.
81-88: Ensure database compatibility ofGROUP_CONCATfunction for labelsSimilar to tiers, the
GROUP_CONCATusage here may not be compatible with PostgreSQL.
94-101: LGTM! Good use of MIN aggregationUsing MIN to get a single customer ID per member is a clean approach.
107-113: LGTM! Clean subscription queryThe query efficiently fetches distinct subscribed members.
119-122: LGTM! Efficient use of Map and SetUsing Map and Set data structures provides optimal lookup performance during processing.
133-138:⚠️ Potential issueHandle potential undefined label names
Similar to products, accessing
allLabels[id]needs null checking.const labelDetails = labelIds.map((id) => { + const name = allLabels[id]; + if (!name) { + return null; + } return { - name: allLabels[id] + name }; -}); +}).filter(Boolean);Likely invalid or redundant comment.
125-130:⚠️ Potential issueHandle potential undefined product names
When accessing
allProducts[id], there's no check for undefined values which could cause issues if a product ID doesn't exist.const tierDetails = tierIds.map((id) => { + const name = allProducts[id]; + if (!name) { + return null; + } return { - name: allProducts[id] + name }; -}); +}).filter(Boolean);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ghost/core/core/server/services/members/exporter/query.js (3)
Line range hint
23-35: Remove commented-out code.The commented-out code block appears to be from the old implementation and should be removed to maintain code cleanliness.
94-101: Document the use of MIN for stripe_customer_id.Please add a comment explaining why
MIN(customer_id)is used to select the Stripe customer ID. This will help future maintainers understand the business logic behind this choice.
147-151: Overall implementation feedback.The changes introduce good performance logging and efficient data structures. However, please address:
- Database compatibility issues with GROUP_CONCAT
- Null checks for product and label lookups
- Remove commented-out code
- Document the Stripe customer ID selection logic
The performance logging will be valuable for monitoring and optimization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/core/server/services/members/exporter/query.js(2 hunks)
🔇 Additional comments (8)
ghost/core/core/server/services/members/exporter/query.js (8)
4-4: LGTM! Good addition of performance logging.The addition of logging will help track performance metrics across different stages of the export process.
Also applies to: 8-8
40-50: LGTM! Efficient data structure transformation.Converting products and labels to lookup objects is an efficient approach for the subsequent data processing.
56-62: LGTM! Clean and efficient member data retrieval.The query is well-structured and only fetches the required columns with appropriate filtering.
68-75: Ensure database compatibility ofGROUP_CONCATfunctionThe use of
GROUP_CONCATmay not be compatible across all database systems. For example, PostgreSQL usesSTRING_AGGinstead.Consider using database-agnostic methods or handle database-specific functions conditionally:
const isPostgres = knex.client.config.client === 'pg'; query.select( 'member_id', isPostgres ? knex.raw('STRING_AGG(product_id::text, \',\') as tiers') : knex.raw('GROUP_CONCAT(product_id) as tiers') );
81-88: Ensure database compatibility ofGROUP_CONCATfunction for labelsSimilar to tiers, the use of
GROUP_CONCATfor labels needs database-specific handling.
107-113: LGTM! Clean subscription status retrieval.The query efficiently retrieves distinct member subscriptions.
119-123: LGTM! Efficient use of Maps and Sets.Using Maps for lookups and Sets for membership tests is a performant approach.
125-139:⚠️ Potential issueAdd null checks for product and label lookups.
The current implementation might throw errors if
allProducts[id]orallLabels[id]is undefined.Apply this fix:
const tierDetails = tierIds.map((id) => { + const name = allProducts[id]; + if (!name) { + return null; + } return { - name: allProducts[id] + name }; -}); +}).filter(Boolean); const labelDetails = labelIds.map((id) => { + const name = allLabels[id]; + if (!name) { + return null; + } return { - name: allLabels[id] + name }; -}); +}).filter(Boolean);Likely invalid or redundant comment.
|
Deployed to staging with ID: |
ref https://linear.app/ghost/issue/ONC-699/lever-member-export-unresponsive