Tests for children of ListMessage express incorrect behaviours

Thanks for submitting an issue to cloud_controller_ng. We are always trying to improve! To help us, please fill out the following template.

Issue

There is an inconsistency in the understanding of how a child of ListMessage should behave if it receives a non-array value for a field of type array. There are a large number of tests (about 100) that talk about behaviours that are incorrect. These behaviours occur in unit tests, but cannot occur in reality.

Context

To take one example, the TaskListMessage spec suggests that validation should fail if names is not an array:
https://github.com/cloudfoundry/cloud_controller_ng/blob/72d262f52187a2c3427fbc6b4958b783f8140acf/spec/unit/messages/tasks_list_message_spec.rb#L166-L170

But the implementation suggests that if names is not an array, it should be converted to an array:
https://github.com/cloudfoundry/cloud_controller_ng/blob/72d262f52187a2c3427fbc6b4958b783f8140acf/app/messages/tasks_list_message.rb#L32-L34
and:
https://github.com/cloudfoundry/cloud_controller_ng/blob/72d262f52187a2c3427fbc6b4958b783f8140acf/app/messages/list_message.rb#L75-L81

So it looks as if the test (above) should fail because names will automatically be converted to an array without validation failure. But the test passes because in the unit test params is a plain hash where the key is a symbol (:names), while the implementation tries to look up the (string) key "names", and therefore does not do the conversion.

In reality, params will be a HashWithIndifferentAccess, so the behaviour will be to convert names (or :names) because this data type does not distinguish between strings and symbols. We can see the HashWithIndifferentAccess being created here:
https://github.com/cloudfoundry/cloud_controller_ng/blob/72d262f52187a2c3427fbc6b4958b783f8140acf/app/controllers/v3/application_controller.rb#L63-L65

The test expresses a behaviour that is simply incorrect, and makes it harder to understand what is going on.

We have tried changing the implementation to always use symbolised keys, and this causes over 100 unit test failure. We think most of those unit tests are simply invalid.

Steps to Reproduce

Change this:
https://github.com/cloudfoundry/cloud_controller_ng/blob/72d262f52187a2c3427fbc6b4958b783f8140acf/app/messages/list_message.rb#L75-L82

to this:

def self.from_params(params, to_array_keys) 
   opts = params.dup.symbolize_keys
   to_array_keys.each do |attribute| 
     to_array! opts, attribute.to_sym
   end 
  
   message = new(opts)
   message.requirements = parse_label_selector(opts[:label_selector]) if message.requested?(:label_selector)

Expected result

We would expect the above change to leave behaviour unaffected.

Current result

The above change causes 190 tests to fail.

Possible Fix

We think that there are a number of unit tests that should be deleted or updated.

We also think we should clarify whether the Rails query_parameters hash values can ever contain array types. Many unit tests assume this is the case.

We also think that given query_params always returns a HashWithIndifferentAccess, unit tests that fake this should use the same type, and the implementation of from_params() should check that it receives this type.

Start with spec/unit/messages/tasks_list_message_spec.rb and compare its symbol-keyed params with app/messages/list_message.rb and app/messages/tasks_list_message.rb. Read how app/controllers/v3/application_controller.rb creates query parameters, then identify the unit tests whose assumptions differ from that input type. Done means the affected tests reflect the real behavior and the suite passes.