OUT-3686: paginate-and-filter QBO customer email lookup#241
OUT-3686: paginate-and-filter QBO customer email lookup#241SandipBajracharya merged 4 commits intomasterfrom
Conversation
QBO's /query parser silently mishandles RFC-legal special characters in PrimaryEmailAddr filters (confirmed for '+', and both '=' and 'LIKE' literal forms fail), returning 0 rows even when a matching customer exists. findOrCreateCustomer therefore missed existing QBO customers with plus-aliased emails and created duplicates. Replaces the WHERE-clause email filter with a paginated walk + JS-side match. The email never appears in the query, so any RFC-legal address works. Adds a sanitizedCompanyName parameter to disambiguate customers sharing an email across companies (one Copilot client can be enrolled in multiple companies); this absorbs the post-filter that previously did the same check in customer.service.ts. Outer wrapWithRetry intentionally dropped on getCustomerByEmail so a mid-walk 429 retries only the failing page, not the entire realm walk. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes a QBO query-parser bug where special characters in email addresses (confirmed for
Confidence Score: 3/5The walk-and-filter logic is correct, but missing ORDER BY and the O(n/1000) API cost both warrant a second look before merging. The missing ORDERBY on the paginated query means QBO can reorder its internal cursor between pages whenever a customer is inserted concurrently — the same class of false-negative the PR is fixing could reappear on a page boundary. Serialising up to O(n/1000) QBO calls on the invoice webhook hot path also introduces meaningful latency and rate-limit exposure for larger realms. src/utils/intuitAPI.ts — the pagination query in _getCustomerByEmail needs an ORDERBY Id ASC clause and the API-call cost impact on large realms should be evaluated. Important Files Changed
Sequence DiagramsequenceDiagram
participant CS as customer.service.ts
participant IA as IntuitAPI
participant QBO as QBO /query
CS->>IA: getCustomerByEmail(email, sanitizedCompanyName)
Note over IA: needle = email.trim().toLowerCase()
Note over IA: if (!needle) return undefined
loop paginate while customers.length === pageSize
IA->>QBO: SELECT ... WHERE Active IN (true,false) STARTPOSITION n MAXRESULTS 1000
QBO-->>IA: { Customer: [...] } or {}
Note over IA: customers = response.Customer ?? []
alt customers.length === 0
IA-->>CS: undefined (exhausted)
else match found (email + companyName)
IA-->>CS: CustomerQueryResponseSchema.parse(match)
else customers.length < 1000 (last partial page, no match)
IA-->>CS: undefined
else full page, no match yet
Note over IA: startPosition += 1000
end
end
Reviews (1): Last reviewed commit: "fix(OUT-3686): paginate-and-filter QBO c..." | Re-trigger Greptile |
QBO's default ordering is MetaData.LastUpdatedTime DESC. Under that ordering, a customer updated between page fetches shifts to the front and can be pushed past our STARTPOSITION cursor — a false negative for a customer that genuinely exists, exactly the failure class this PR is fixing for plus-aliased emails. Flagged on review. Id is monotonic and immutable, so concurrent updates do not move rows and any customer created during the walk lands at the end of the cursor where we will still encounter it. Both race classes closed. Cost: newly-created customers land on the last page rather than page 1, so drift recovery for a fresh customer in a 10k realm walks all pages (~5s) instead of hitting on page 1 (~500ms). The perf cost is bounded and only fires on local-DB-miss paths — full stability over a faster but racier ordering. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
c9f2389 to
1119d4b
Compare
priosshrsth
left a comment
There was a problem hiding this comment.
@SandipBajracharya Minor feedback. Otherwise everything looks good to me.
| if ((c.CompanyName || undefined) !== sanitizedCompanyName) return false | ||
| return true |
There was a problem hiding this comment.
| if ((c.CompanyName || undefined) !== sanitizedCompanyName) return false | |
| return true | |
| return c.CompanyName === sanitizedCompanyName |
There was a problem hiding this comment.
CompanyName can be empty string when returned from QBO. Since the value for sanitizedCompanyName is either string or optional, comparing CompanyName with empty string is never equivalent to undefined sanitizedCompanyName. That is the reason for such implementation.
There was a problem hiding this comment.
This is confusing tbh. So if both are undefined === undefined or "" === "" vayo vane ni true nai janu parne haina?
Your check right now can have following scenarios:
undefined === string | undefined
${string} === string | undedinfed,
So only thing you want to avoid is "" === string | undefined?
There was a problem hiding this comment.
What I want to be true is "" === undefined. Left hand side is never undefined without || undefined. QBO sends empty string when there is no value. So empty string becomes undefined with help of the logical OR operation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
/queryparser silently mishandles RFC-legal special characters inPrimaryEmailAddrfilters (confirmed for+, both=andLIKEliteral forms fail), returning 0 rows even when a matching customer exists.findOrCreateCustomertherefore missed existing QBO customers with plus-aliased emails and created duplicates.sanitizedCompanyNameparameter to disambiguate customers sharing an email across companies (one Copilot client can be enrolled in multiple companies); this absorbs the post-filter that previously did the same check incustomer.service.ts.wrapWithRetryintentionally dropped ongetCustomerByEmailso a mid-walk 429 retries only the failing page, not the entire realm walk from page 1. InnercustomQueryretries are preserved.Test plan
test/unit/utils/intuitAPI.test.tscovering pagination termination, case/whitespace normalisation, malformedPrimaryEmailAddrshapes, off-by-one full-page-then-empty edge case, the+-alias regression, and company-aware matching with multi-page walksyarn tsc --noEmitcleanyarn lint:checkclean (0 errors)user+tag@example.com, trigger an invoice webhook, confirmfindOrCreateCustomerfinds the existing customer instead of creating a duplicateCompanyName— confirm walker returns the matching-company customer, not the first email-match🤖 Generated with Claude Code