Apply documentation changes from code review

Co-authored-by: Nicolas Fricke <[email protected]>

README.md

@@ -278,7 +278,10 @@ This is due to how the cursor is generated.
 It requires the record's primary key to always be present.
 Therefore, even if it is not selected by you, it will be added to the query.
 
-You can specify a different primary key using the `primary_key:` parameter in case you use something different than IDs (e.g. UUID) or if your query is aggregated by some value. Always make sure that the primary key is unique!
+By default, the `:id` is used as primary key.
+You can specify a different primary key using the `primary_key:` parameter in case you use something different than IDs (e.g. UUID) or if your query is aggregated by some value.
+Always **make sure that the primary key is unique**!
+Otherwise, the generated cursors will not identify a unique record and the pagination breaks.
 
 For example, you could have a query like the one below to get the latest date an author created a post:
 
@@ -290,13 +293,15 @@ RailsCursorPagination::Paginator
   )
 ```
 
-Without the `primary_key:` parameter this would also `SELECT` the `id` parameter (which is the default primary key), causing the query to fail MySQL's validation.
+Without the `primary_key:` parameter this would also `SELECT` the `id` parameter (which is the default primary key).
+But since the `id` is not part of the `.group` call, depending on the database used, this can lead to errors.
 
-**Always use `primary_key:` when you know what you are doing!**
+**Only use `primary_key:` when you know what you are doing!**
 
 The same goes for any field that is specified via `order_by:`, this field is also required for building the cursor and will therefore automatically be requested from the database.
 
-If `order_by:` is not specified it will use the `primary_key:` value. If neither is specified, both will default to `:id`.
+If `order_by:` is not specified it will use the `primary_key:` value.
+If neither is specified, both will default to `:id`.
 
 ## How does it work?
 
@@ -333,7 +338,7 @@ LIMIT 2
 ```
 
 This will return the first page of results, containing post #1 and #2.
-Since no custom order is defined, each item in the returned collection will have a cursor that only encodes the record's primary key (it's ID).
+Since no custom order is defined, each item in the returned collection will have a cursor that only encodes the record's primary key (its ID).
 
 If we want to now request the next page, we can pass in the cursor of record #2 which would be `"Mg=="`.
 So now we can request the next page by calling:

lib/rails_cursor_pagination/cursor.rb

@@ -67,7 +67,7 @@ def decode(encoded_string:, order_field: :id, primary_key: :id)
 
     # Initializes the record
     #
-    # @param primary_key_value
+    # @param primary_key_value [Object]
     #   The identifier of the cursor record
     # @param order_field [Symbol]
     #   The column or virtual column for ordering

lib/rails_cursor_pagination/paginator.rb

@@ -38,12 +38,16 @@ class Paginator
     # @param order [Symbol, nil]
     #   Ordering to apply, either `:asc` or `:desc`. Defaults to `:asc`.
     # @param primary_key [Symbol, String]
-    #   Column to use as primary key instead of ID. If none is provided it will
-    #   default to `id`. This is intended for cases where you can't create a
-    #   compound index like described in the documentation for `order_by` or you
-    #   need to order by some computed value like `MAX(id)`. Note that setting
-    #   this parameter will replace `:id` everywhere when fetching the query so
-    #   use it only if you know what you are doing!
+    #   Column to use as primary key instead of ID. This column needs to only have
+    #   unique values for the passed relation! If this parameter is not provided, it will
+    #   default to `id`.
+    #   This is intended for cases where your relation does not have an ID column
+    #   (e.g. because you use UUIDs), where you want to order by a more complex
+    #   logic but cannot create a compound index like described in the documentation
+    #   for `order_by` or you need to order by some computed value like `MAX(id)`.
+    #   Note that setting this parameter will replace `:id` everywhere when fetching
+    #   the query and that this is being used to compute the cursor. Therefore, it needs
+    #   to be unique, otherwise pagination breaks.
     #
     # @raise [RailsCursorPagination::ParameterError]
     #   If any parameter is not valid
@@ -424,6 +428,12 @@ def relation_with_cursor_fields
     # @return [ActiveRecord::Relation]
     def sorted_relation
       unless custom_order_field?
+        # By default, ActiveRecord automagically prefixes columns with the
+        # respective table name to avoid collisions on joined relations. 
+        # However, if a computed primary key like e.g. `MAX(id)` is used, this
+        # should not be prefixed with the table name. Therefore, it has to be
+        # passed as a string instead of a hash to the `#reorder` method to
+        # disable this prefixing behavior.
         order_statement = if @primary_key.is_a?(String)
                             "#{@primary_key} #{pagination_sorting.upcase}"
                           else