Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix GuestDevice#child_device references #22670

Merged
merged 2 commits into from
Aug 22, 2023
Merged

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Aug 17, 2023

The child_devices added unneeded scope (and therefore an extra GuestDevices.pluck(:id))

Before

has_many :child_devices, -> { where(:parent_device_id => ids) }, :foreign_key => "parent_device_id"
# rewritten to make the issue more obvious: 
has_many :child_devices, -> { where(:parent_device_id => GuestDevice.ids) }, :foreign_key => "parent_device_id"

SELECT "guest_devices".*
FROM "guest_devices"
WHERE "guest_devices"."parent_device_id" = 26000000000035
  AND "guest_devices"."parent_device_id" IN (26000000000032, 26000000000033, 26000000000034, 26000000000035, 26000000000036, 26000000000037, 26000000000038)

After

has_many :child_devices, :foreign_key => "parent_device_id"

SELECT "guest_devices".*
FROM "guest_devices"
WHERE "guest_devices"."parent_device_id" = 26000000000035

This was introduce in #15371
The way the PR was batted around, it looked like this was not an intentional side effect.

@kbrock
Copy link
Member Author

kbrock commented Aug 18, 2023

update:

  • fixed cap: useless assignment in spec

update:

  • added parent_device to make rubocop happy

@kbrock kbrock mentioned this pull request Aug 21, 2023
4 tasks
The child_devices added unneeded scope.
This scope pulled back all ids from the guest devices table.

Since the foreign_key is added to the query, this did not do any harm.
But it adds an unneeded constraint an unneeded query.

This was introduce in ManageIQ#15371
The way the PR was batted around, it looked like this was not an intentional side effect.

Before
======

```ruby
has_many :child_devices, -> { where(:parent_device_id => ids) }, :foreign_key => "parent_device_id"
has_many :child_devices, -> { where(:parent_device_id => GuestDevice.ids) }, :foreign_key => "parent_device_id"

SELECT "guest_devices".*
FROM "guest_devices"
WHERE "guest_devices"."parent_device_id" = 26000000000035
  AND "guest_devices"."parent_device_id" IN (26000000000032, 26000000000033, 26000000000034, 26000000000035, 26000000000036, 26000000000037, 26000000000038)
```

After
=====

```ruby
has_many :child_devices, :foreign_key => "parent_device_id"

SELECT "guest_devices".*
FROM "guest_devices"
WHERE "guest_devices"."parent_device_id" = 26000000000035
```
To be honest, I only added this to make rubocop happy.
But it did feel like it was missing and made the specs a little prettier.
@kbrock
Copy link
Member Author

kbrock commented Aug 22, 2023

update:

  • rebased (updated puma) to fix build

@miq-bot
Copy link
Member

miq-bot commented Aug 22, 2023

Checked commits kbrock/manageiq@7299d1a~...732039f with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 👍

@agrare agrare merged commit eacd100 into ManageIQ:master Aug 22, 2023
9 checks passed
@agrare agrare assigned agrare and unassigned jrafanie Aug 22, 2023
@agrare agrare added the core label Aug 22, 2023
@kbrock kbrock deleted the guest_devices branch August 22, 2023 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants