initializer modifies class

[durandom]

I'm a bit worried about

    def initialize(api, json_or_hash)
      @api = api

      raw_hash = json_or_hash.kind_of?(Hash) ? json_or_hash : JSON.parse(json_or_hash)
      self.class.send(:id_attr, *raw_hash['related'].keys) if raw_hash.key?('related')

      super(raw_hash)
    end

in base_model introduced in https://github.com/ansible/ansible_tower_client_ruby/pull/17

Not only does it modify a class whenever an instance is created, which can lead to different behaviour across different instances. It's also not thread safe, potentially leading to data corruption.

I'm also seeing inconsistent behaviour when accessing these _id methods

(byebug) ap i.class
AnsibleTowerClient::JobTemplate < AnsibleTowerClient::BaseModel
(byebug) ap i.to_hash['related']
{
                        "created_by" => "/api/v1/users/1/",
                       "modified_by" => "/api/v1/users/1/",
                            "labels" => "/api/v1/job_templates/76/labels/",
                         "inventory" => "/api/v1/inventories/6/",
                           "project" => "/api/v1/projects/40/",
                        "credential" => "/api/v1/credentials/6/",
      "notification_templates_error" => "/api/v1/job_templates/76/notification_templates_error/",
    "notification_templates_success" => "/api/v1/job_templates/76/notification_templates_success/",
                              "jobs" => "/api/v1/job_templates/76/jobs/",
                      "object_roles" => "/api/v1/job_templates/76/object_roles/",
        "notification_templates_any" => "/api/v1/job_templates/76/notification_templates_any/",
                       "access_list" => "/api/v1/job_templates/76/access_list/",
                            "launch" => "/api/v1/job_templates/76/launch/",
                         "schedules" => "/api/v1/job_templates/76/schedules/",
                   "activity_stream" => "/api/v1/job_templates/76/activity_stream/",
                       "survey_spec" => "/api/v1/job_templates/76/survey_spec/"
}
(byebug) ap i.cloud_credential
nil
(byebug) ap i.cloud_credential_id
nil
(byebug) ap i.credential
*** NoMethodError Exception: undefined method `credential' for #<AnsibleTowerClient::JobTemplate:0x007f9a068e92f0>
Did you mean?  credential_id

nil
(byebug) ap i.credential_id
6

TBH, I would favour no magic id columns and stick as close to the response as possible

[durandom]

cc @bdunne @bzwei @syncrou

[bdunne]

@durandom I would expect i.credential to be an AnsibleTowerCredential instance, not "/api/v1/credentials/6", especially since that referral may not be correct.

I agree that it shouldn't modify the class on instance create and your concern about thread safety.

[durandom]

I would expect i.credential to be an AnsibleTowerCredential instance, not "/api/v1/credentials/6"

The above hash is the related key from the api response. i.credential would be 6 if its was just the mapping to the response.

But you say it should be an instance of AnsibleTowerCredential and not and undefined method?

So there might be another bug?

[bdunne]

yeah, sounds like a bug to me.

[miq-bot]

This issue has been automatically marked as stale because it has not been updated for at least 3 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the traige process documentation.