Merge pull request #79 from code4lib/non_integer_resumption_token_last_id

non-integer identifiers work in ResumptionToken

Author: eddierubeiz

lib/oai/provider/model/activerecord_wrapper.rb

@@ -159,13 +159,13 @@ def select_partial(find_scope, token)
     # the last 'id' of the previous set is used as the
     # filter to the next set.
     def token_conditions(token)
-      last = token.last
+      last_id = token.last_str
       sql = sql_conditions token.to_conditions_hash
 
-      return sql if 0 == last
+      return sql if "0" == last_id
       # Now add last id constraint
       sql.first << " AND #{identifier_field} > :id"
-      sql.last[:id] = last
+      sql.last[:id] = last_id
 
       return sql
     end

lib/oai/provider/resumption_token.rb

@@ -8,16 +8,41 @@ module OAI::Provider
   # The ResumptionToken class forms the basis of paging query results.  It
   # provides several helper methods for dealing with resumption tokens.
   #
+  # OAI-PMH spec does not specify anything about resumptionToken format, they can
+  # be purely opaque tokens.
+  #
+  # Our implementation however encodes everything needed to construct the next page
+  # inside the resumption token.
+  #
+  # == The 'last' component: offset or ID/pk to resume from
+  #
+  # The `#last` component is an offset or ID to resume from. In the case of it being
+  # an ID to resume from, this assumes that ID's are sortable and results are returned
+  # in ID order, so that the 'last' ID can be used as the place to resume from.
+  #
+  # Originally it was assumed that #last was always an integer, but since existing
+  # implementations (like ActiveRecordWrapper) used it as an ID, and identifiers and
+  # primary keys are _not_ always integers (can be UUID etc), we have expanded to allow
+  # any string value.
+  #
+  # However, for backwards compatibility #last always returns an integer (sometimes 0 if
+  # actual last component is not an integer), and #last_str returns the full string version.
+  # Trying to change #last itself to be string broke a lot of existing code in this gem
+  # in mysterious ways.
+  #
+  # Also beware that in some cases the value 0/"0" seems to be a special value used
+  # to signify some special case. A lot of "code archeology" going on here after significant
+  # period of no maintenance to this gem.
   class ResumptionToken
-    attr_reader :prefix, :set, :from, :until, :last, :expiration, :total
+    attr_reader :prefix, :set, :from, :until, :last, :last_str, :expiration, :total
 
     # parses a token string and returns a ResumptionToken
     def self.parse(token_string)
       begin
         options = {}
-        matches = /(.+):(\d+)$/.match(token_string)
-        options[:last] = matches.captures[1].to_i
-        
+        matches = /(.+):([^ :]+)$/.match(token_string)
+        options[:last] = matches.captures[1]
+
         parts = matches.captures[0].split('.')
         options[:metadata_prefix] = parts.shift
         parts.each do |part|
@@ -35,7 +60,7 @@ def self.parse(token_string)
         raise OAI::ResumptionTokenException.new
       end
     end
-    
+
     # extracts the metadata prefix from a token string
     def self.extract_format(token_string)
       return token_string.split('.')[0]
@@ -44,32 +69,32 @@ def self.extract_format(token_string)
     def initialize(options, expiration = nil, total = nil)
       @prefix = options[:metadata_prefix]
       @set = options[:set]
-      @last = options[:last]
+      self.last = options[:last]
       @from = options[:from] if options[:from]
       @until = options[:until] if options[:until]
       @expiration = expiration if expiration
       @total = total if total
     end
-          
+
     # convenience method for setting the offset of the next set of results
     def next(last)
-      @last = last
+      self.last = last
       self
     end
-    
+
     def ==(other)
       prefix == other.prefix and set == other.set and from == other.from and
-        self.until == other.until and last == other.last and 
+        self.until == other.until and last == other.last and
         expiration == other.expiration and total == other.total
     end
-    
+
     # output an xml resumption token
     def to_xml
       xml = Builder::XmlMarkup.new
       xml.resumptionToken(encode_conditions, hash_of_attributes)
       xml.target!
     end
-    
+
     # return a hash containing just the model selection parameters
     def to_conditions_hash
       conditions = {:metadata_prefix => self.prefix }
@@ -78,20 +103,31 @@ def to_conditions_hash
       conditions[:until] = self.until if self.until
       conditions
     end
-    
-    # return the a string representation of the token minus the offset
+
+    # return the a string representation of the token minus the offset/ID
+    #
+    # Q: Why does it eliminate the offset/id "last" on the end? Doesn't fully
+    #    represent state without it, which is confusing. Not sure, but
+    #    other code seems to rely on it, tests break if not.
     def to_s
       encode_conditions.gsub(/:\w+?$/, '')
     end
 
     private
-    
+
+    # take care of our logic to store an integer and a str version, for backwards
+    # compat where it was assumed to be an integer, as well as supporting string.
+    def last=(value)
+      @last = value.to_i
+      @last_str = value.to_s
+    end
+
     def encode_conditions
       encoded_token = @prefix.to_s.dup
       encoded_token << ".s(#{set})" if set
       encoded_token << ".f(#{self.from.utc.xmlschema})" if self.from
       encoded_token << ".u(#{self.until.utc.xmlschema})" if self.until
-      encoded_token << ":#{last}"
+      encoded_token << ":#{last_str}"
     end
 
     def hash_of_attributes

test/activerecord_provider/helpers/providers.rb

@@ -35,6 +35,13 @@ class SimpleResumptionProvider < OAI::Provider::Base
   source_model ActiveRecordWrapper.new(DCField, :limit => 25)
 end
 
+class SimpleResumptionProviderWithNonIntegerID < OAI::Provider::Base
+  repository_name 'ActiveRecord Resumption Provider With Non-Integer ID'
+  repository_url 'http://localhost'
+  record_prefix 'oai:test'
+  source_model ActiveRecordWrapper.new(DCField, :limit => 25, identifier_field: "source")
+end
+
 class CachingResumptionProvider < OAI::Provider::Base
   repository_name 'ActiveRecord Caching Resumption Provider'
   repository_url 'http://localhost'

test/activerecord_provider/tc_simple_paging_provider.rb

@@ -8,19 +8,38 @@ def test_full_harvest
     assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
     assert_equal 26, 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"]
     token = doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
     assert_equal 26, doc.elements["/OAI-PMH/ListRecords"].to_a.size
+
     doc = Document.new(@provider.list_records(:resumption_token => token))
     assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
     token = doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
     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
   end
 
+  def test_non_integer_identifiers_resumption
+    @provider = SimpleResumptionProviderWithNonIntegerID.new
+
+    doc = Document.new(@provider.list_records(:metadata_prefix => 'oai_dc'))
+    assert_not_nil doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
+    assert_equal 26, doc.elements["/OAI-PMH/ListRecords"].to_a.size
+    token = doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
+
+    next_doc = Document.new(@provider.list_records(:resumption_token => token))
+    assert_not_nil next_doc.elements["/OAI-PMH/ListRecords/resumptionToken"]
+    next_token = next_doc.elements["/OAI-PMH/ListRecords/resumptionToken"].text
+    assert_equal 26, next_doc.elements["/OAI-PMH/ListRecords"].to_a.size
+
+    assert_not_equal token, next_token
+  end
+
   def test_from_and_until
     first_id = DCField.order("id asc").first.id
     DCField.where("id < #{first_id + 25}").update_all(updated_at: Time.parse("September 15 2005"))

test/provider/tc_resumption_tokens.rb

@@ -43,4 +43,10 @@ def test_resumption_token_to_xml
     assert_equal "#{@token.to_s}:#{@token.last}", doc.elements['/resumptionToken'].text
   end
 
+  def test_resumption_token_id_does_not_need_to_be_numeric
+    serialized = "oai_dc.s(A).f(2005-01-01T17:00:00Z).u(2005-01-31T17:00:00Z):FA129C"
+
+    token = ResumptionToken.parse(serialized)
+    assert_equal serialized, token.send(:encode_conditions)
+  end
 end