Merge pull request #96 from code4lib/resumption-token-incomplete-list

Include blank resumption token element in final partial list response

Author: barmintor

lib/oai/provider/model.rb

@@ -29,7 +29,7 @@ module OAI::Provider
   # see the ResumptionToken class for more details.
   #
   class Model
-    attr_reader :timestamp_field, :identifier_field
+    attr_reader :timestamp_field, :identifier_field, :limit
 
     def initialize(limit = nil, timestamp_field = 'updated_at', identifier_field = 'id')
       @limit = limit

lib/oai/provider/model/activerecord_caching_wrapper.rb

@@ -78,13 +78,7 @@ def next_set(token_string)
       raise ResumptionTokenException.new unless @limit
 
       token = ResumptionToken.parse(token_string)
-      total = model.where(token_conditions(token)).count
-
-      if token.last * @limit + @limit < total
-        select_partial(token)
-      else
-        select_partial(token).records
-      end
+      select_partial(token)
     end
 
     # select a subset of the result set, and return it with a
@@ -102,10 +96,13 @@ def select_partial(token)
 
       raise ResumptionTokenException.new unless oaitoken
 
+      total = model.where(token_conditions(token)).count
+      # token offset should be nil if this is the last set
+      offset = (token.last * @limit + @limit >= total) ? nil : token.last + 1
       PartialResult.new(
         hydrate_records(
           oaitoken.entries.limit(@limit).offset(token.last * @limit)),
-        token.next(token.last + 1)
+        token.next(offset)
       )
     end
 

lib/oai/provider/model/activerecord_wrapper.rb

@@ -129,15 +129,7 @@ def next_set(find_scope, token_string)
       raise OAI::ResumptionTokenException.new unless @limit
 
       token = ResumptionToken.parse(token_string)
-      total = find_scope.where(token_conditions(token)).count
-
-      if @limit < total
-        select_partial(find_scope, token)
-      else # end of result set
-        find_scope.where(token_conditions(token))
-          .limit(@limit)
-          .order("#{identifier_field} asc")
-      end
+      select_partial(find_scope, token)
     end
 
     # select a subset of the result set, and return it with a
@@ -147,8 +139,10 @@ def select_partial(find_scope, token)
         .limit(@limit)
         .order("#{identifier_field} asc")
       raise OAI::ResumptionTokenException.new unless records
-      offset = records.last.send(identifier_field)
 
+      total = find_scope.where(token_conditions(token)).count
+      # token offset should be nil if this is the last set
+      offset = (@limit >= total) ? nil : records.last.send(identifier_field)
       PartialResult.new(records, token.next(offset))
     end
 

lib/oai/provider/resumption_token.rb

@@ -36,7 +36,7 @@ class ResumptionToken
     attr_reader :prefix, :set, :from, :until, :last, :last_str, :expiration, :total
 
     # parses a token string and returns a ResumptionToken
-    def self.parse(token_string)
+    def self.parse(token_string, expiration = nil, total = nil)
       begin
         options = {}
         matches = /(.+):([^ :]+)$/.match(token_string)
@@ -54,7 +54,7 @@ def self.parse(token_string)
             options[:until] = Time.parse(part.sub(/^u\(/, '').sub(/\)$/, '')).localtime
           end
         end
-        self.new(options)
+        self.new(options, expiration, total)
       rescue => err
         raise OAI::ResumptionTokenException.new
       end
@@ -122,6 +122,8 @@ def last=(value)
     end
 
     def encode_conditions
+      return "" if last_str.nil? || last_str.to_s.strip.eql?("")
+
       encoded_token = @prefix.to_s.dup
       encoded_token << ".s(#{set})" if set
       encoded_token << ".f(#{self.from.utc.xmlschema})" if self.from

test/activerecord_provider/tc_caching_paging_provider.rb

@@ -17,8 +17,9 @@ def test_full_harvest
     token = doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
     assert_equal 26, doc.elements["/OAI-PMH/ListRecords"].size
     doc = Document.new(@provider.list_records(:resumption_token => token))
-    assert_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
-    assert_equal 25, doc.elements["/OAI-PMH/ListRecords"].size
+    assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
+    assert_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
+    assert_equal 26, doc.elements["/OAI-PMH/ListRecords"].to_a.size
   end
 
   def test_from_and_until
@@ -37,8 +38,9 @@ def test_from_and_until
     token = doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
     assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
     doc = Document.new(@provider.list_records(:resumption_token => token))
-    assert_equal 25, doc.elements["/OAI-PMH/ListRecords"].size
-    assert_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
+    assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
+    assert_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
+    assert_equal 26, doc.elements["/OAI-PMH/ListRecords"].to_a.size
   end
 
   def setup

test/activerecord_provider/tc_simple_paging_provider.rb

@@ -20,8 +20,9 @@ def test_full_harvest
     assert_equal 26, doc.elements["/OAI-PMH/ListRecords"].to_a.size
 
     doc = Document.new(@provider.list_records(:resumption_token => token))
-    assert_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
-    assert_equal 25, doc.elements["/OAI-PMH/ListRecords"].to_a.size
+    assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
+    assert_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
+    assert_equal 26, doc.elements["/OAI-PMH/ListRecords"].to_a.size
   end
 
   def test_non_integer_identifiers_resumption
@@ -58,8 +59,9 @@ def test_from_and_until
     assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
     token = doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
     doc = Document.new(@provider.list_records(:resumption_token => token))
-    assert_equal 25, doc.elements["/OAI-PMH/ListRecords"].to_a.size
-    assert_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
+    assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
+    assert_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
+    assert_equal 26, doc.elements["/OAI-PMH/ListRecords"].to_a.size
   end
 
   def setup

test/provider/models.rb

@@ -68,7 +68,7 @@ def find(selector, opts={})
           if token.last < @groups.size - 1
             PartialResult.new(@groups[token.last], token.next(token.last + 1))
           else
-            @groups[token.last]
+            PartialResult.new(@groups[token.last], token.next(nil))
           end
         rescue
           raise OAI::ResumptionTokenException.new

test/provider/tc_functional_tokens.rb

@@ -5,6 +5,11 @@ class ResumptionTokenFunctionalTest < Test::Unit::TestCase
 
   def setup
     @provider = ComplexProvider.new
+    @provider.model.instance_variable_set(:@limit, 120)
+  end
+
+  def teardown
+    @provider.model.instance_variable_set(:@limit, 100)
   end
 
   def test_resumption_tokens
@@ -13,15 +18,15 @@ def test_resumption_tokens
     end
     doc = Document.new(@provider.list_records(:metadata_prefix => 'oai_dc'))
     assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
-    assert_equal 101, doc.elements["/OAI-PMH/ListRecords"].to_a.size
+    assert_equal (@provider.model.limit + 1), doc.elements["/OAI-PMH/ListRecords"].to_a.size
     token = doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
     doc = Document.new(@provider.list_records(:resumption_token => token))
     assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
-    assert_equal 101, doc.elements["/OAI-PMH/ListRecords"].to_a.size
+    assert_equal (@provider.model.limit + 1), doc.elements["/OAI-PMH/ListRecords"].to_a.size
   end
 
   def test_from_and_until_with_resumption_tokens
-    # Should return 300 records broken into 3 groups of 100.
+    # Should return 300 records broken into 3 groups of 120, 120, and 60.
     assert_nothing_raised do
       Document.new(@provider.list_records(:metadata_prefix => 'oai_dc'))
     end
@@ -31,17 +36,20 @@ def test_from_and_until_with_resumption_tokens
         :from => Time.parse("September 1 2004"),
         :until => Time.parse("November 30 2004"))
       )
-    assert_equal 101, doc.elements["/OAI-PMH/ListRecords"].to_a.size
+    assert_equal (@provider.model.limit + 1), doc.elements["/OAI-PMH/ListRecords"].to_a.size
+    assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
     token = doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
 
     doc = Document.new(@provider.list_records(:resumption_token => token))
     assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
-    assert_equal 101, doc.elements["/OAI-PMH/ListRecords"].to_a.size
+    assert_equal (@provider.model.limit + 1), doc.elements["/OAI-PMH/ListRecords"].to_a.size
     token = doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
 
     doc = Document.new(@provider.list_records(:resumption_token => token))
-    assert_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
-    assert_equal 100, doc.elements["/OAI-PMH/ListRecords"].to_a.size
+    # assert that ListRecords includes remaining records and an empty resumption token
+    assert_equal (301 % @provider.model.limit), doc.elements["/OAI-PMH/ListRecords"].to_a.size
+    assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
+    assert_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
   end
 
 end