Bugfix: limit and offset params caused reordered resource list

CHANGELOG.md

@@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Nothing should go in this section, please add to the latest unreleased version
   (and update the corresponding date), or add a new version.
 
+## [1.19.2] - 2022-01-13
+
+### Fixed
+- Previously, including `limit` or `offset` parameters to a resource list request
+  resulted in the returned list being unexpectedly sorted. Now, all resource list
+  request results are sorted by resource ID.
+  [cyberark/conjur#2702](https://github.com/cyberark/conjur/pull/2702)
+
 ## [1.19.1] - 2022-12-08
 
 ### Security

app/models/resource.rb

@@ -104,6 +104,9 @@ def search account: nil, kind: nil, owner: nil, offset: nil, limit: nil, search:
       # Filter by string search
       scope = scope.textsearch(search) if search
 
+      # Sort results alphabetically by resource ID
+      scope = scope.order(:resource_id)
+
       if offset || limit
         # 'limit' must be an integer greater than 0 if given
         if limit && (!numeric?(limit) || limit.to_i <= 0)
@@ -114,7 +117,7 @@ def search account: nil, kind: nil, owner: nil, offset: nil, limit: nil, search:
           raise ArgumentError, "'offset' contains an invalid value. 'offset' must be an integer greater than or equal to 0."
         end
 
-        scope = scope.order(:resource_id).limit(
+        scope = scope.limit(
           (limit || 10).to_i,
           (offset || 0).to_i
         )

spec/controllers/resources_controller_spec.rb

@@ -34,82 +34,139 @@
       { 'HTTP_AUTHORIZATION' => token_auth_str }
     end
 
-    def list_resources_without_limit()
-      get(resources_url, env: token_auth_header())
+    def list_resources(limit: nil, offset: nil, count: false)
+      params = {}
+      params.merge!({ :limit => limit }) if limit
+      params.merge!({ :offset => offset }) if offset
+      params.merge!({ :count => count }) if count
+      get(resources_url, env: token_auth_header, params: params)
     end
 
-    def list_resources_with_limit(limit)
-      get(resources_url, env: token_auth_header(), params: {:limit => limit})
-    end
-
-    def count_resources_without_limit()
-      get(resources_url, env: token_auth_header(), params: {:count => "true"})
-    end
-
-    def count_resources_with_limit(limit)
-      get(resources_url, env: token_auth_header(), params: {:limit => limit, :count => "true"})
+    def count_resources(limit: nil)
+      list_resources(limit: limit, count: true)
     end
 
     def load_variables()
-      payload = '[!variable preexisting\n!variable preexisting1]'
+      payload = '[!variable a, !variable b, !variable c, !variable d, !host a, !host b, !host c, !host d, !layer a, !layer b, !layer c]'
       put(policies_url, env: token_auth_header.merge({ 'RAW_POST_DATA' => payload }))
     end
 
     context 'with default configuration' do
-      it "should list all resources when limit not defined" do
-        list_resources_without_limit()
-        expect(response.code).to eq("200")
-        expect(JSON.parse(response.body).size).to eq(2)
+      context 'with no query params defined' do
+        before(:each) do
+          list_resources()
+          @resources = JSON.parse(response.body)
+        end
+
+        it 'should return a 200 status code' do
+          expect(response.code).to eq("200")
+        end
+
+        it 'should list all resources' do
+          expect(@resources.size).to eq(12)
+        end
+
+        it 'should order resources alphabetically by resource id' do
+          @resources.each_with_index do |resource, idx|
+            next if idx == 0
+            expect(resource["id"]).to be > @resources[idx-1]["id"]
+          end
+        end
       end
 
-      it "should list resources according to query param limit when limit defined" do
-        list_resources_with_limit(1)
-        expect(response.code).to eq("200")
-        expect(JSON.parse(response.body).size).to eq(1)
+      context 'with limit query param defined' do
+        before(:each) do
+          list_resources(limit: 5)
+          @resources = JSON.parse(response.body)
+        end
+
+        it 'should return a 200 status code' do
+          expect(response.code).to eq("200")
+        end
+
+        it 'should list resources according to the provided limit' do
+          expect(@resources.size).to eq(5)
+        end
+
+        it 'should order resources alphabetically by resource id' do
+          @resources.each_with_index do |resource, idx|
+            next if idx == 0
+            expect(resource["id"]).to be > @resources[idx-1]["id"]
+          end
+        end
+      end
+
+      context 'with offset query param defined' do
+        before(:each) do
+          list_resources(offset: 1)
+          @resources = JSON.parse(response.body)
+        end
+
+        it 'should return a 200 status code' do
+          expect(response.code).to eq("200")
+        end
+
+        it 'should offset resources according to the provided offset' do
+          list_resources()
+          all_resources = JSON.parse(response.body)
+
+          expect(@resources[0]).to eq(all_resources[1])
+        end
+
+        it 'should limit list to 10 resources when offset defined and limit not defined' do
+          expect(@resources.size).to eq(10)
+        end
+
+        it 'should order resources alphabetically by resource id' do
+          @resources.each_with_index do |resource, idx|
+            next if idx == 0
+            expect(resource["id"]).to be > @resources[idx-1]["id"]
+          end
+        end
       end
     end
 
     context 'with custom configuration' do
       it "should list resources according to custom configuration when limit not defined" do
         Rails.application.config.conjur_config.api_resource_list_limit_max = 1
-        list_resources_without_limit()
+        list_resources()
         expect(response.code).to eq("200")
         expect(JSON.parse(response.body).size).to eq(1)
       end
 
       it "should list resources according to query param limit when custom configuration exceeds limit" do
         Rails.application.config.conjur_config.api_resource_list_limit_max = 2
-        list_resources_with_limit(1)
+        list_resources(limit: 1)
         expect(response.code).to eq("200")
         expect(JSON.parse(response.body).size).to eq(1)
       end
 
       it "should throw error when limit exceeds custom configuration" do
         Rails.application.config.conjur_config.api_resource_list_limit_max = 1
-        list_resources_with_limit(2)
+        list_resources(limit: 2)
         expect(response.code).to eq("422")
       end
     end
 
     context 'when validating count request' do
       it "should count all resources when custom configuration defined" do
         Rails.application.config.conjur_config.api_resource_list_limit_max = 1
-        count_resources_without_limit()
+        count_resources()
         expect(response.code).to eq("200")
-        expect(response.body).to eq("{\"count\":2}")
+        expect(response.body).to eq("{\"count\":12}")
       end
 
       it "should count all resources when custom configuration not defined" do
-        count_resources_without_limit()
+        count_resources()
         expect(response.code).to eq("200")
-        expect(response.body).to eq("{\"count\":2}")
+        expect(response.body).to eq("{\"count\":12}")
       end
 
       # There is a currently a bug in the API when supplying both the `limit`
       # and `count` parameters. A count response shouldn't be affected by
       # the `limit` parameter. This should be changed when the bug is fixed (ONYX-22079)
       it "should count resources according to query param limit " do
-        count_resources_with_limit(1)
+        count_resources(limit: 1)
         expect(response.body).to eq("{\"count\":1}")
       end
     end