Ensure that there is always a primary key

change several user errors into programmer errors,
and other small refactoring/fixes

doc/development/api_graphql_styleguide.md

@@ -146,6 +146,10 @@ query($project_path: ID!) {
 }
 ```
 
+To ensure that we get consistent ordering, we will append an ordering on the primary
+key, in descending order.  This is usually `id`, so basically we will add `order(id: :desc)`
+to the end of the relation.  A primary key _must_ be available on the underlying table.
+
 ### Exposing permissions for a type
 
 To expose permissions the current user has on a resource, you can call

lib/gitlab/graphql/connections/keyset/conditions/base_condition.rb

@@ -18,10 +18,6 @@ module Gitlab
 
             attr_reader :arel_table, :names, :values, :operator, :before_or_after
 
-            def assemble_conditions(conditions)
-              conditions.join
-            end
-
             def table_condition(attribute, value, operator)
               case operator
               when '>'

lib/gitlab/graphql/connections/keyset/conditions/not_null_condition.rb

@@ -7,18 +7,16 @@ module Gitlab
         module Conditions
           class NotNullCondition < BaseCondition
             def build
-              conditions = []
-              conditions << first_attribute_condition
+              conditions = [first_attribute_condition]
 
               # If there is only one order field, we can assume it
               # does not contain NULLs, and don't need additional
               # conditions
               unless names.count == 1
-                conditions << second_attribute_condition
-                conditions << final_condition
+                conditions << [second_attribute_condition, final_condition]
               end
 
-              assemble_conditions(conditions)
+              conditions.join
             end
 
             private

lib/gitlab/graphql/connections/keyset/conditions/null_condition.rb

@@ -7,11 +7,7 @@ module Gitlab
         module Conditions
           class NullCondition < BaseCondition
             def build
-              conditions = []
-              conditions << first_attribute_condition
-              conditions << final_condition
-
-              assemble_conditions(conditions)
+              [first_attribute_condition, final_condition].join
             end
 
             private

lib/gitlab/graphql/connections/keyset/connection.rb

@@ -2,7 +2,7 @@
 
 # Keyset::Connection provides cursor based pagination, to avoid using OFFSET.
 # It basically sorts / filters using WHERE sorting_value > cursor.
-# We do this for performance reasons (https://gitlab.com/gitlab-org/gitlab-ce/issues/45756),
+# We do this for performance reasons (https://gitlab.com/gitlab-org/gitlab-foss/issues/45756),
 # as well as for having stable pagination
 # https://graphql-ruby.org/pro/cursors.html#whats-the-difference
 # https://coderwall.com/p/lkcaag/pagination-you-re-probably-doing-it-wrong
@@ -82,6 +82,10 @@ module Gitlab
 
           def ordered_nodes
             strong_memoize(:order_nodes) do
+              unless nodes.primary_key.present?
+                raise ArgumentError.new('Relation must have a primary key')
+              end
+
               list = OrderInfo.build_order_list(nodes)
 
               # ensure there is a primary key ordering

lib/gitlab/graphql/connections/keyset/order_info.rb

@@ -33,22 +33,22 @@ module Gitlab
 
           def self.validate_ordering(relation, order_list)
             if order_list.empty?
-              raise ::ArgumentError.new('A minimum of 1 ordering field is required')
+              raise ArgumentError.new('A minimum of 1 ordering field is required')
             end
 
             if order_list.count > 2
-              raise Gitlab::Graphql::Errors::ArgumentError.new('A maximum of 2 ordering fields are allowed')
+              raise ArgumentError.new('A maximum of 2 ordering fields are allowed')
             end
 
             # make sure the last ordering field is non-nullable
             attribute_name = order_list.last&.attribute_name
 
             if relation.columns_hash[attribute_name].null
-              raise Gitlab::Graphql::Errors::ArgumentError.new("Column `#{attribute_name}` must not allow NULL")
+              raise ArgumentError.new("Column `#{attribute_name}` must not allow NULL")
             end
 
             if order_list.last.attribute_name != relation.primary_key
-              raise Gitlab::Graphql::Errors::ArgumentError.new("Last ordering field must be the primary key, `#{relation.primary_key}`")
+              raise ArgumentError.new("Last ordering field must be the primary key, `#{relation.primary_key}`")
             end
           end
 

lib/gitlab/graphql/connections/keyset/query_builder.rb

@@ -9,7 +9,7 @@ module Gitlab
             @arel_table, @order_list, @decoded_cursor, @before_or_after = arel_table, order_list, decoded_cursor, before_or_after
 
             if order_list.empty?
-              raise Gitlab::Graphql::Errors::ArgumentError.new('No ordering scopes have been supplied')
+              raise ArgumentError.new('No ordering scopes have been supplied')
             end
           end
 
@@ -44,12 +44,10 @@ module Gitlab
             attr_values = attr_names.map { |name| decoded_cursor[name] }
 
             if attr_names.count == 1 && attr_values.first.nil?
-              raise Gitlab::Graphql::Errors::ArgumentError.new('Only one sortable scope and nil was supplied')
+              raise Gitlab::Graphql::Errors::ArgumentError.new('Before/after cursor invalid: `nil` was provided as only sortable value')
             end
 
-            operators = comparison_operators
-
-            if attr_names.count == 1 || attr_values.first.presence
+            if attr_names.count == 1 || attr_values.first.present?
               Keyset::Conditions::NotNullCondition.new(arel_table, attr_names, attr_values, operators, before_or_after).build
             else
               Keyset::Conditions::NullCondition.new(arel_table, attr_names, attr_values, operators, before_or_after).build
@@ -60,7 +58,7 @@ module Gitlab
 
           attr_reader :arel_table, :order_list, :decoded_cursor, :before_or_after
 
-          def comparison_operators
+          def operators
             order_list.map { |field| field.operator_for(before_or_after) }
           end
         end

spec/lib/gitlab/graphql/connections/keyset/conditions/not_null_condition_spec.rb

@@ -14,7 +14,7 @@ describe Gitlab::Graphql::Connections::Keyset::Conditions::NotNullCondition do
           ("issues"."id" > 500)
         SQL
 
-        expect(condition.build).to eq expected_sql
+        expect(condition.build.squish).to eq expected_sql.squish
       end
     end
 
@@ -32,7 +32,7 @@ describe Gitlab::Graphql::Connections::Keyset::Conditions::NotNullCondition do
           OR ("issues"."relative_position" IS NULL)
         SQL
 
-        expect(condition.build).to eq expected_sql
+        expect(condition.build.squish).to eq expected_sql.squish
       end
     end
 
@@ -49,7 +49,7 @@ describe Gitlab::Graphql::Connections::Keyset::Conditions::NotNullCondition do
           )
         SQL
 
-        expect(condition.build).to eq expected_sql
+        expect(condition.build.squish).to eq expected_sql.squish
       end
     end
   end

spec/lib/gitlab/graphql/connections/keyset/conditions/null_condition_spec.rb

@@ -18,7 +18,7 @@ describe Gitlab::Graphql::Connections::Keyset::Conditions::NullCondition do
         )
         SQL
 
-        expect(condition.build).to eq expected_sql
+        expect(condition.build.squish).to eq expected_sql.squish
       end
     end
 
@@ -35,7 +35,7 @@ describe Gitlab::Graphql::Connections::Keyset::Conditions::NullCondition do
           OR ("issues"."relative_position" IS NOT NULL)
         SQL
 
-        expect(condition.build).to eq expected_sql
+        expect(condition.build.squish).to eq expected_sql.squish
       end
     end
   end

spec/lib/gitlab/graphql/connections/keyset/connection_spec.rb

@@ -22,34 +22,34 @@ describe Gitlab::Graphql::Connections::Keyset::Connection do
     let(:cursor)  { connection.cursor_from_node(project) }
 
     it 'returns an encoded ID' do
-      expect(decoded_cursor(cursor)).to eq({ 'id' => project.id.to_s })
+      expect(decoded_cursor(cursor)).to eq('id' => project.id.to_s)
     end
 
     context 'when an order is specified' do
       let(:nodes) { Project.order(:updated_at) }
 
       it 'returns the encoded value of the order' do
-        expect(decoded_cursor(cursor)).to include({ 'updated_at' => project.updated_at.to_s })
+        expect(decoded_cursor(cursor)).to include('updated_at' => project.updated_at.to_s)
       end
 
       it 'includes the :id even when not specified in the order' do
-        expect(decoded_cursor(cursor)).to include({ 'id' => project.id.to_s })
+        expect(decoded_cursor(cursor)).to include('id' => project.id.to_s)
       end
     end
 
-    context 'when multiple orders is specified' do
+    context 'when multiple orders are specified' do
       let(:nodes) { Project.order(:updated_at).order(:created_at) }
 
       it 'returns the encoded value of the order' do
-        expect(decoded_cursor(cursor)).to include({ 'updated_at' => project.updated_at.to_s })
+        expect(decoded_cursor(cursor)).to include('updated_at' => project.updated_at.to_s)
       end
     end
 
-    context 'when multiple orders with SQL is specified' do
+    context 'when multiple orders with SQL are specified' do
       let(:nodes) { Project.order(Arel.sql('projects.updated_at IS NULL')).order(:updated_at).order(:id) }
 
       it 'returns the encoded value of the order' do
-        expect(decoded_cursor(cursor)).to include({ 'updated_at' => project.updated_at.to_s })
+        expect(decoded_cursor(cursor)).to include('updated_at' => project.updated_at.to_s)
       end
     end
   end
@@ -285,5 +285,19 @@ describe Gitlab::Graphql::Connections::Keyset::Connection do
         expect(last_order_name).to eq sliced.primary_key
       end
     end
+
+    context 'when there is no primary key' do
+      let(:nodes) { NoPrimaryKey.all }
+
+      it 'raises an error' do
+        expect(NoPrimaryKey.primary_key).to be_nil
+        expect { subject.sliced_nodes }.to raise_error(ArgumentError, 'Relation must have a primary key')
+      end
+    end
+  end
+
+  class NoPrimaryKey < ActiveRecord::Base
+    self.table_name  = 'no_primary_key'
+    self.primary_key = nil
   end
 end

spec/lib/gitlab/graphql/connections/keyset/order_info_spec.rb

@@ -27,7 +27,7 @@ describe Gitlab::Graphql::Connections::Keyset::OrderInfo do
 
       it 'raises an error' do
         expect { described_class.validate_ordering(relation, order_list) }
-          .to raise_error(::ArgumentError, 'A minimum of 1 ordering field is required')
+          .to raise_error(ArgumentError, 'A minimum of 1 ordering field is required')
       end
     end
 
@@ -36,7 +36,7 @@ describe Gitlab::Graphql::Connections::Keyset::OrderInfo do
 
       it 'raises an error' do
         expect { described_class.validate_ordering(relation, order_list) }
-          .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'A maximum of 2 ordering fields are allowed')
+          .to raise_error(ArgumentError, 'A maximum of 2 ordering fields are allowed')
       end
     end
 
@@ -45,7 +45,7 @@ describe Gitlab::Graphql::Connections::Keyset::OrderInfo do
 
       it 'raises an error' do
         expect { described_class.validate_ordering(relation, order_list) }
-          .to raise_error(Gitlab::Graphql::Errors::ArgumentError, "Column `updated_at` must not allow NULL")
+          .to raise_error(ArgumentError, "Column `updated_at` must not allow NULL")
       end
     end
 
@@ -54,7 +54,7 @@ describe Gitlab::Graphql::Connections::Keyset::OrderInfo do
 
       it 'raises error if primary key is not last field' do
         expect { described_class.validate_ordering(relation, order_list) }
-          .to raise_error(Gitlab::Graphql::Errors::ArgumentError, "Last ordering field must be the primary key, `#{relation.primary_key}`")
+          .to raise_error(ArgumentError, "Last ordering field must be the primary key, `#{relation.primary_key}`")
       end
     end
   end

spec/lib/gitlab/graphql/connections/keyset/query_builder_spec.rb

@@ -6,7 +6,7 @@ describe Gitlab::Graphql::Connections::Keyset::QueryBuilder do
   context 'when number of ordering fields is 0' do
     it 'raises an error' do
       expect { described_class.new(Issue.arel_table, [], {}, :after) }
-        .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'No ordering scopes have been supplied')
+        .to raise_error(ArgumentError, 'No ordering scopes have been supplied')
     end
   end
 
@@ -24,7 +24,7 @@ describe Gitlab::Graphql::Connections::Keyset::QueryBuilder do
 
         it 'raises an error' do
           expect { builder.conditions }
-            .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'Only one sortable scope and nil was supplied')
+            .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'Before/after cursor invalid: `nil` was provided as only sortable value')
         end
       end