Allow TransactionGateway.Search to return more than 50 transactions.

[dsmcfarl]

What

Fetch the Ids for all search results then fetch the transaction details
one page at a time. TransactionGateway.Search should be backwards
compatible. The only difference being additional fields in
TransactionSearchResult and more than 50 transactions in the
Transaction field.

Per
https://developers.braintreepayments.com/reference/general/searching/search-results/java
up to 20,000 transactions can be returned.

Much of this work is based on the work by @rkulla:
https://github.com/dailyburn/braintree-go/commit/d44aa131caad6055c0e4aac2f3a24450aa818c63

Why

The Braintree API only returns 50 transactions with all details at one
time. To fetch all results from a query, the transaction ids must be
fetched first, then the details can be fetched by specifying 50 ids per
API call. This issue was first raised in #112.

[leighmcculloch]

@dsmcfarl Thanks for taking what @rkulla had done and submitting this PR. Also thanks for making it backwards compatible and being cognizant of that.

I think we need to iterate on this though, because as it stands this will expand the paged results such that all the transactions (up to 20,000) will be loaded into memory and returned in a single slice. That might not be practical depending on the memory applications have access to, so I want to look into mirroring the logic in the other Braintree SDKs where the search results are downloaded as they're needed and only the page size is held in memory at any one time.

This will need us to break the backwards compatibility of it though, which sucks, but I think it's necessary and all part of this library being v0.x.

What do you think?

[leighmcculloch]

I dropped some changes directly into this PR, moving things around such that instead of loading all the pages into memory in one hit the user requests each additional page. I managed to keep it backwards compatible still. Search returns the first page of results and a new function SearchNext that takes the last set of results will go and get the next set of results.

I'm still pondering if we should break the API slightly and instead have Search return an iterator. I'd appreciate input from @dsmcfarl, @rkulla, and others on this. The iterator would give you access to:

• All the IDs returned []string.
• Pages of TransactionSearchResult similar to how this PR does now.
• Transactions one at a time.

My motivation for thinking about an iterator is that iterating over the pages is a little annoying. Two loops are needed:

query := &SearchQuery{}
query.AddTextField("customer-first-name").StartsWith = "prefix"

results, err := txg.Search(ctx, query)
if err != nil {
	...
}

for {
	for _, tx := range results.Transactions {
		...
	}

	results, err = txg.SearchNext(ctx, results)
	if err != nil {
		...
	}
	if results == nil {
		break
	}
}

If someone just wants to iterate transaction-by-transaction the iterator would help simplify that usage down to something like:

query := &SearchQuery{}
query.AddTextField("customer-first-name").StartsWith = "prefix"

results, err := txg.Search(ctx, query)
if err != nil {
	...
}

for {
	tx, err := results.Next()
	if err != nil {
		...
	}
	if tx == nil {
		break
	}

	...
}

But the iterator could still expose the page-by-page view as well:

query := &SearchQuery{}
query.AddTextField("customer-first-name").StartsWith = "prefix"

results, err := txg.Search(ctx, query)
if err != nil {
	...
}

for {
	page, err := results.NextPage()
	if err != nil {
		...
	}
	if page == nil {
		break
	}

	for _, tx := range page.Transactions {
		...
	}
}

Things I'm thinking about if we add an iterator:

• Will the Next/NextPage functions of the iterator take a Context, or will the iterator reference it internally from the original Search call.
• Will the Next/NextPage functions of the iterator take a TransactionGateway, or will the iterator reference it internally from the original Search call.
• Can the existing TransactionSearchResult be the iterator so that it's backwards compatible with existing uses, but exposes a way to page through all the results.

[leighmcculloch]

I'm going to run with this change as is without an iterator. I've made two additional changes:

1. I changed the lookup of transaction IDs so that method is an exported method which means the user could grab the list of IDs without needing to also get the first page.

2. I kept the search query out of the results, so it's possible to serialize the results across page loads, and the query can be rebuilt manually without the query needing to be serializable.

This entire change is backwards compatible, and if we decide an iterator is valuable we can add one on-top of this behavior.

[dsmcfarl]

@leighmcculloch Sorry for the radio silence on this (just had a baby boy!), and thanks for finishing and merging this. Your updates will work fine for my use case, however, I have no objection to the iterator approach that breaks backward compatibility. In the long run it would probably be nicer and would be trivial to update my code.

[rkulla]

Congrats and thanks for doing that!

[leighmcculloch]

@dsmcfarl Congrats on the baby boy!

I'm up for adding an iterator, but I think we could add the iterator around the existing result, so that both are useable. That also make make the API too confusing too to have both.

[leighmcculloch]

Revisiting this in #246 .