search: allow advanced filters #1995
Conversation
The search key is opaque to the system. Interpret the prefix marker _q=1 as the beginning of an advanced query, allowing to filter on specific fields, e.g.: _q=1&[email protected]¤cy=USD It is the responsability of the user to add relevant indexes. Signed-off-by: Pierre-Alexandre Meyer <[email protected]>
Simple solution to work around lack of implicit casting on PostgreSQL. Good enough for now? Signed-off-by: Pierre-Alexandre Meyer <[email protected]>
This allows us to find all open and paid invoices for instance. Signed-off-by: Pierre-Alexandre Meyer <[email protected]>
util/src/main/resources/org/killbill/billing/util/entity/dao/EntitySqlDao.sql.stg
Show resolved
Hide resolved
account/src/main/java/org/killbill/billing/account/dao/DefaultAccountDao.java
Show resolved
Hide resolved
account/src/main/java/org/killbill/billing/account/dao/DefaultAccountDao.java
Show resolved
Hide resolved
invoice/src/main/resources/org/killbill/billing/invoice/dao/InvoiceSqlDao.sql.stg
Show resolved
Hide resolved
invoice/src/main/resources/org/killbill/billing/invoice/dao/InvoiceSqlDao.sql.stg
Show resolved
Hide resolved
| searchQuery = new SearchQuery(SqlOperator.OR); | ||
| searchQuery.addSearchClause("currency", SqlOperator.EQ, searchKey); | ||
| } else if (searchKey.startsWith(SEARCH_QUERY_MARKER)) { | ||
| final Matcher matcher = BALANCE_QUERY_PATTERN.matcher(searchKey); |
There was a problem hiding this comment.
Is the use case that we have to show (return) invoices matching specific balance criteria for the given tenant - and not for a specific account?
There was a problem hiding this comment.
Yes, the use-case is to filter invoices for that tenant and balance criteria, e.g., show me all unpaid invoices.
There is a branch in the code because it doesn't support additional filtering capabilities: it's either balance filter or any other filter. Cannot do: show me all unpaid invoices in the last month unfortunately. We might be able to do it, it's just a bit hard with the balance specific query...
| >> | ||
|
|
||
| searchByCurrency(ordering) ::= << | ||
| searchInvoicesByBalance(ordering, comparisonOperator) ::= << |
There was a problem hiding this comment.
The result(s) we return will match the criteria, but since we just return the columns (fileds), the exact balance will not be returned, is this correct - i.e. balance computation is just used for search criteria and not to populate the objects being returned?
There was a problem hiding this comment.
balance computation is just used for search criteria and not to populate the objects being returned
Correct.
since we just return the columns (fileds)
That's a good point, I need to check if the objects are re-hydrated at higher levels of the stack or if I should select more columns here.
There was a problem hiding this comment.
I am just wondering if we could use the amount computed from the underlying invoiceBalanceQuery() so we don't have to rehydrate the invoices as it is very expensive. However, not returning the balance is an issue as well.
There was a problem hiding this comment.
We could:
- Modify
InvoiceModelDaoto have abalancefield. This is set only in this new codepath, when computing the balance in SQL (thebalancecolumn from the SQL query should automatically populate it if the getter and setter for balance are added inInvoiceModelDao. - Modify
DefaultInvoiceto have a privatebalancefield. If set during construction of the object (pulled fromInvoiceModelDao),getBalance()returns it, otherwise, it goes through the computation like today.
@reshmabidikar Could you give this a try? When doing searches, the invoices will still be shallow, but when searchInvoicesByBalance is called, the balance should be returned. If this work, we could also look at updating the other search queries to do the same?
util/src/main/java/org/killbill/billing/util/entity/dao/SearchQuery.java
Show resolved
Hide resolved
util/src/main/java/org/killbill/billing/util/entity/dao/SearchQuery.java
Show resolved
Hide resolved
|
Note to self: unresolved comments must be addressed before merging. |
Address review comments on 1995
| , x.tenant_record_id | ||
| , SUM(x.amount) as balance | ||
| FROM ( | ||
| SELECT |
There was a problem hiding this comment.
Do we need that first query - is it because we want to return also that DRAFT or VOID invoices?
There was a problem hiding this comment.
We could:
- Modify
InvoiceModelDaoto have abalancefield. This is set only in this new codepath, when computing the balance in SQL (thebalancecolumn from the SQL query should automatically populate it if the getter and setter for balance are added inInvoiceModelDao. - Modify
DefaultInvoiceto have a privatebalancefield. If set during construction of the object (pulled fromInvoiceModelDao),getBalance()returns it, otherwise, it goes through the computation like today.
@reshmabidikar Could you give this a try? When doing searches, the invoices will still be shallow, but when searchInvoicesByBalance is called, the balance should be returned. If this work, we could also look at updating the other search queries to do the same?
| JOIN invoices inv ON ii.invoice_id = inv.id | ||
| LEFT OUTER JOIN tags tg ON ii.invoice_id = tg.object_id | ||
| WHERE | ||
| ii.type in ('TAX', 'EXTERNAL_CHARGE', 'FIXED', 'USAGE', 'RECURRING', 'ITEM_ADJ', 'REPAIR_ADJ', 'PARENT_SUMMARY', 'CBA_ADJ', 'CREDIT_ADJ') |
There was a problem hiding this comment.
It looks like all the InvoiceItemType are present here, so is this needed?
The search key is opaque to the system. Interpret the prefix marker
_q=1as the beginning of an advanced query, allowing to filter on specific fields, e.g.:It is the responsibility of the user to add relevant indexes.