Attributes order by :definition doesn't work correctly with more than 10 attributes in a serializer

[pilgunboris]

Hello, I'm migrating a project from active_model_serializer to improve speed of json generation and noticed that attribute sorting doesn't work correctly when my new oj serializer has more than 10 attributes.

I know it is not important in most situations but in this case, it is crucial to preserve the existing order of attributes in the output not to confuse customers.

I have this list of attributes

    attributes :id,
      :prefix,
      :first_name,
      :middle_name,
      :last_name,
      :suffix,
      :nickname,
      :name,
      :job_title,
      :type,
      :address

    flat_one :creator, serializer: Shared::CreatorSerializer

so the correct attributes list in the order of definition is:

[49] pry(main)> attributes = Exports::ContactOjSerializer.send(:_attributes)

=> { 
  "id" => { value_from: "id", attribute: :method, identifier: true },
  "prefix" => { value_from: "prefix", attribute: :method },
  "first_name" => { value_from: "first_name", attribute: :method },
  "middle_name" => { value_from: "middle_name", attribute: :method },
  "last_name" => { value_from: "last_name", attribute: :method },
  "suffix" => { value_from: "suffix", attribute: :method },
  "nickname" => { value_from: "nickname", attribute: :method },
  "name" => { value_from: "name", attribute: :method },
  "job_title" => { value_from: "job_title", attribute: :method },
  "type" => { value_from: "type", attribute: :method },
  "address" => { value_from: "address", attribute: :method },
  "creator11" => { value_from: "creator", association: :flat,
                   serializer: Exports::Shared::CreatorSerializer } 
}

however, after calling this method it becomes incorrect:

oj_serializers/lib/oj_serializers/serializer.rb

Lines 709 to 720 in b327a62

	def prepare_attributes(transform_keys: try(:_transform_keys), sort_by: try(:_sort_attributes_by))
	attributes = _attributes
	attributes = attributes.transform_keys(&transform_keys) if transform_keys
	if sort_by == :name
	sort_by = ->(name, options, _) { options[:identifier] ? "__#{name}" : name }
	elsif !sort_by || sort_by == :definition
	sort_by = ->(name, options, index) { options[:identifier] ? "__#{name}" : "zzz#{index}" }
	end
	attributes.sort_by.with_index { |(name, options), index| sort_by.call(name, options, index) }.to_h
	end

and looks like this (address and creator11 were moved to the wrong places):

[45] pry(main)> Exports::ContactOjSerializer.send(:prepare_attributes)
=> {
  "id" => { value_from: "id", attribute: :method, identifier: true },
  "prefix" => { value_from: "prefix", attribute: :method },
  "address" => { value_from: "address", attribute: :method },
  "creator11" => { value_from: "creator", association: :flat,
                   serializer: Exports::Shared::CreatorSerializer },
  "first_name" => { value_from: "first_name", attribute: :method },
  "middle_name" => { value_from: "middle_name", attribute: :method },
  "last_name" => { value_from: "last_name", attribute: :method },
  "suffix" => { value_from: "suffix", attribute: :method },
  "nickname" => { value_from: "nickname", attribute: :method },
  "name" => { value_from: "name", attribute: :method },
  "job_title" => { value_from: "job_title", attribute: :method },
  "type" => { value_from: "type", attribute: :method }
}

I debugged it and found that this lambda:

sort_by = ->(name, options, index) { options[:identifier] ? "__#{name}" : "zzz#{index}" }

internally builds such array of conditions for sorting:

["__id", "zzz1", "zzz2", "zzz3", "zzz4", "zzz5", "zzz6", "zzz7", "zzz8", "zzz9", "zzz10", "zzz11"]

however, if we call sort on it it makes an unobvious modification because of the way ruby handles strings sorting:

[56] pry(main)> a.sort
=> ["__id", "zzz1", "zzz10", "zzz11", "zzz2", "zzz3", "zzz4", "zzz5", "zzz6", "zzz7", "zzz8", "zzz9"]

Which leads to the incorrect order of attributes in the output.

---

What if we modify that lambda like this:

sort_by = ->(name, options, index) { options[:identifier] ? index * -1 : index }

So the IDs will come first after sorting but we do use the index as a number which keeps the order of other attributes correct.

Although, it has one disadvantage. If there is more than one it will set the identifiers in the inverse order of their positions instead of sorting them by name. But I guess it should be a rare case 🤔
So if that is critical maybe there should be a better solution.

Ideally, from sort_by == :definition I'd expect to not make any changes in the order at all and to have a separate sort_by kind for putting IDs to the first place when needed, e.g. sort_by == :identifiers_first.
But to keep the backward compatibility, might just make the suggested (or somehow improved) change.

What do you think?

For me, it is important to fix this as soon as possible, so I can try to prepare a PR with this fix if that's okay.

[ElMassimo]

Hi Borys!

Changing the sort behavior for :definition makes sense.

Perhaps we can return something equivalent to [!options[:identifier], index] in order to ensure that sorting is stable for non identifiers.

[pilgunboris]

Hi Máximo,

I've tried it but unfortunately couldn't make it to work with this code:

        sort_by = ->(name, options, index) { [!options[:identifier], index] }

Maybe it is happening because the arrays include different data types, but I'm not sure.

---

I used this solution:

        sort_by = ->(name, options, index) {
          if options[:identifier]
            0 - (_attributes.count - index)
          else
            index
          end
        }

All the identifiers will be positioned in the negative range in the correct order without affecting the position of the attribute placed at 0 index (if the :id is at the end of the list initially), so the result will remain stable:

[10] pry(main)> 20.times {|index| puts 0 - (20 - index) }
-20
-19
-18
-17
-16
-15
-14
-13
-12
-11
-10
-9
-8
-7
-6
-5
-4
-3
-2
-1
=> 20

Here is the PR #25

The new test for this use case was also included.
It fails with the existing implementation:

What do you think?

[pilgunboris]

Hi Máximo, did you have a chance to check the fix?