From 0c51e31d1c69479e5d97db73e1a78ca6e44dd716 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 28 Sep 2018 12:29:03 +0200 Subject: [PATCH 1/2] channeldb/invoices+test: don't return invoices on reversed query at index 1 Previously a call to QueryInvoices with reversed=true and index_offset=1 would make the cursor point to the first available invoice (num 1) that would be returned as part of the response. This is inconsistent with the othre indexes, so we instead just return an empty list in this case. A test case for this situation is also added. --- channeldb/invoice_test.go | 21 +++++++++++++++++++++ channeldb/invoices.go | 23 +++++++++++++++++++++-- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/channeldb/invoice_test.go b/channeldb/invoice_test.go index 5c648112..3d659c56 100644 --- a/channeldb/invoice_test.go +++ b/channeldb/invoice_test.go @@ -460,6 +460,27 @@ func TestQueryInvoices(t *testing.T) { }, expected: invoices[10:], }, + // Fetch one invoice, at index 1, reversed. Since invoice#1 is + // the very first, there won't be any left in a reverse search, + // so we expect no invoices to be returned. + { + query: InvoiceQuery{ + IndexOffset: 1, + Reversed: true, + NumMaxInvoices: 1, + }, + expected: nil, + }, + // Same as above, but don't restrict the number of invoices to + // 1. + { + query: InvoiceQuery{ + IndexOffset: 1, + Reversed: true, + NumMaxInvoices: numInvoices, + }, + expected: nil, + }, // Fetch all pending invoices with a single query. { query: InvoiceQuery{ diff --git a/channeldb/invoices.go b/channeldb/invoices.go index e9149c4d..4b0edffe 100644 --- a/channeldb/invoices.go +++ b/channeldb/invoices.go @@ -484,9 +484,28 @@ func (d *DB) QueryInvoices(q InvoiceQuery) (InvoiceSlice, error) { // our cursor depending on the parameters set within the query. c := invoiceAddIndex.Cursor() invoiceKey := keyForIndex(c, q.IndexOffset+1) + + // If the query is specifying reverse iteration, then we must + // handle a few offset cases. if q.Reversed { - _, invoiceKey = c.Last() - if q.IndexOffset != 0 { + switch q.IndexOffset { + + // This indicates the default case, where no offset was + // specified. In that case we just start from the last + // invoice. + case 0: + _, invoiceKey = c.Last() + + // This indicates the offset being set to the very + // first invoice. Since there are no invoices before + // this offset, and the direction is reversed, we can + // return without adding any invoices to the response. + case 1: + return nil + + // Otherwise we start iteration at the invoice prior to + // the offset. + default: invoiceKey = keyForIndex(c, q.IndexOffset-1) } } From 22d0e5b43e08ef1ac9ac7b8b90c44a4debc020e9 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 28 Sep 2018 12:58:06 +0200 Subject: [PATCH 2/2] channeldb/invoice test: add more tests for edge cases --- channeldb/invoice_test.go | 105 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/channeldb/invoice_test.go b/channeldb/invoice_test.go index 3d659c56..014f8230 100644 --- a/channeldb/invoice_test.go +++ b/channeldb/invoice_test.go @@ -435,6 +435,14 @@ func TestQueryInvoices(t *testing.T) { }, expected: invoices, }, + // Fetch all invoices with a single query, reversed. + { + query: InvoiceQuery{ + Reversed: true, + NumMaxInvoices: numInvoices, + }, + expected: invoices, + }, // Fetch the first 25 invoices. { query: InvoiceQuery{ @@ -460,6 +468,33 @@ func TestQueryInvoices(t *testing.T) { }, expected: invoices[10:], }, + // Fetch all but the first invoice. + { + query: InvoiceQuery{ + IndexOffset: 1, + NumMaxInvoices: numInvoices, + }, + expected: invoices[1:], + }, + // Fetch one invoice, reversed, with index offset 3. This + // should give us the second invoice in the array. + { + query: InvoiceQuery{ + IndexOffset: 3, + Reversed: true, + NumMaxInvoices: 1, + }, + expected: invoices[1:2], + }, + // Same as above, at index 2. + { + query: InvoiceQuery{ + IndexOffset: 2, + Reversed: true, + NumMaxInvoices: 1, + }, + expected: invoices[0:1], + }, // Fetch one invoice, at index 1, reversed. Since invoice#1 is // the very first, there won't be any left in a reverse search, // so we expect no invoices to be returned. @@ -481,6 +516,76 @@ func TestQueryInvoices(t *testing.T) { }, expected: nil, }, + // Fetch one invoice, reversed, with no offset set. We expect + // the last invoice in the response. + { + query: InvoiceQuery{ + Reversed: true, + NumMaxInvoices: 1, + }, + expected: invoices[numInvoices-1:], + }, + // Fetch one invoice, reversed, the offset set at numInvoices+1. + // We expect this to return the last invoice. + { + query: InvoiceQuery{ + IndexOffset: numInvoices + 1, + Reversed: true, + NumMaxInvoices: 1, + }, + expected: invoices[numInvoices-1:], + }, + // Same as above, at offset numInvoices. + { + query: InvoiceQuery{ + IndexOffset: numInvoices, + Reversed: true, + NumMaxInvoices: 1, + }, + expected: invoices[numInvoices-2 : numInvoices-1], + }, + // Fetch one invoice, at no offset (same as offset 0). We + // expect the first invoice only in the response. + { + query: InvoiceQuery{ + NumMaxInvoices: 1, + }, + expected: invoices[:1], + }, + // Same as above, at offset 1. + { + query: InvoiceQuery{ + IndexOffset: 1, + NumMaxInvoices: 1, + }, + expected: invoices[1:2], + }, + // Same as above, at offset 2. + { + query: InvoiceQuery{ + IndexOffset: 2, + NumMaxInvoices: 1, + }, + expected: invoices[2:3], + }, + // Same as above, at offset numInvoices-1. Expect the last + // invoice to be returned. + { + query: InvoiceQuery{ + IndexOffset: numInvoices - 1, + NumMaxInvoices: 1, + }, + expected: invoices[numInvoices-1:], + }, + // Same as above, at offset numInvoices. No invoices should be + // returned, as there are no invoices after this offset. + { + query: InvoiceQuery{ + IndexOffset: numInvoices, + NumMaxInvoices: 1, + }, + expected: nil, + }, // Fetch all pending invoices with a single query. { query: InvoiceQuery{