NotificationSqlDao should provide search on both keys

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.

Confused, isn't it the case already?

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);

.

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.

[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.

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

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