NotificationSqlDao should provide search on both keys

[pierre]

NotificationSqlDao should provide getReadyQueueEntriesForSearchKeyS, not only because it is required for Kill Bill (to search by account_record_id and tenant_record_id), but also because the only index present today is a composite on both keys.

[sbrossie]

Confused, isn't it the case already?

[pierre]

No, we only search on one key. See https://github.com/killbill/killbill-commons/blob/master/queue/src/main/resources/org/killbill/notificationq/dao/NotificationSqlDao.sql.stg#L31 and

killbill-commons/queue/src/main/java/org/killbill/notificationq/DefaultNotificationQueue.java

Line 102 in dc64043

return getFutureNotificationsInternal(type, (NotificationSqlDao) dao.getSqlDao(), "search_key1", searchKey1);

.

[sbrossie]

Ah got it, you mean searching for both searchKeys at once....

Still confused why we would need it though (for Kill Bill): Once we specify searching by account_record_id, it will implicitly return only one tenant so then tenant_record_id becomes irrelevant.

I suppose from an API point of view (for the queue itself) it may make sense to search by both keys at once.

[pierre]

[We talked about it IRL, but just for the records]

Still confused why we would need it though (for Kill Bill): Once we specify searching by account_record_id, it will implicitly return only one tenant so then tenant_record_id becomes irrelevant.

account_record_id is eventually specified by the user via API (e.g. by specifying accountId) which is a security risk. tenant_record_id is set by Shiro looking at the tenant credentials. Searching by both prevents API users to peek at other tenants' data.

[sbrossie]

@pierre Seems like we search for both keys here. Should this be closed?

[pierre]

We should just double check we've updated Kill Bill to use it (see my comment about security issue above).