Fixes #30512 - avoid query for smart proxy settings #7864
Conversation
|
Issues: #30512 |
|
This is needed for Katello/katello#8802 |
There was a problem hiding this comment.
Would you mind adding the additional part you added here in the PR (that it allows testing on unsaved objects) in the commit message? That really made me understand the reason for this.
Another thing to note is that this allows query caching smart_proxy_features so it may end up more efficient if you call capabilities or setting with more than one feature.
|
One of the katello test failures is unrelated and being worked on, the other is handled here: Katello/katello#8860 |
|
@ares anyone else that might could take a look? |
app/models/smart_proxy.rb
Outdated
| @@ -89,15 +89,15 @@ def has_feature?(feature_name) | |||
| end | |||
|
|
|||
| def capabilities(feature) | |||
| smart_proxy_features.find_by(:feature_id => Feature.find_by(:name => feature)).try(:capabilities) | |||
| smart_proxy_features.find { |spf| spf.feature_name.to_s == feature.to_s }.try(:capabilities) | |||
There was a problem hiding this comment.
Maybe we can add some includes statements here and in settings to make it a bit more efficient:
| smart_proxy_features.find { |spf| spf.feature_name.to_s == feature.to_s }.try(:capabilities) | |
| smart_proxy_features.includes(:feature).find { |spf| spf.feature_name.to_s == feature.to_s }.try(:capabilities) |
There was a problem hiding this comment.
actually yes, this doesn't seem to work with unsaved objects:
> SmartProxy.new(:smart_proxy_features => [SmartProxyFeature.new(:feature_id => 1, :capabilities => ['foo'])]).smart_proxy_features
=> [#<SmartProxyFeature:0x00000000128e03c0 smart_proxy_id: nil, feature_id: 1, id: nil, capabilities: ["foo"], settings: {}>]
> SmartProxy.new(:smart_proxy_features => [SmartProxyFeature.new(:feature_id => 1, :capabilities => ['foo'])]).smart_proxy_features.includes(:feature)
=> []
There was a problem hiding this comment.
So actually reverting back to this would cause only 1 query per object:
smart_proxy_features.find { |spf| spf.feature_id == Feature.find_by(:name => feature) }.try(:capabilities)
thoughts?
|
I'm wondering if we should use nulldb to ensure this method doesn't perform any DB queries. |
jlsherrill
force-pushed
the
30512
branch
2 times, most recently
from
07378fa
to
3f88750
Compare
|
[test katello] |
|
[test Redmine issues] does that work? |
|
[test redmine] |
|
[test Redmine] |
app/models/smart_proxy.rb
Outdated
| @@ -89,15 +89,17 @@ def has_feature?(feature_name) | |||
| end | |||
|
|
|||
| def capabilities(feature) | |||
| smart_proxy_features.find_by(:feature_id => Feature.find_by(:name => feature)).try(:capabilities) | |||
| feature_id = Feature.find_by(:name => feature).try(:id) | |||
There was a problem hiding this comment.
For my understanding, we have feature_ids = Feature.where(:name => feature_name).pluck(:id) in has_feature?. It feels like this should be the same. Effectively we have 3 places we convert a feature name to an ID very close together, perhaps there are more.
There was a problem hiding this comment.
has_feature? actually supports multiple features being passed in, which these two methods shouldn't. Although i'm not sure that i see has_feature? actually being used that way
There was a problem hiding this comment.
I doubt it - the name is singular so as an API user I wouldn't expect it to work that way.
There was a problem hiding this comment.
it is not saying explicitly, but if you look at the implementation we simply pass the argument to AR, which also accepts an array. So has_feature? can be used with one or more feature names.
It may be worth introducing a third method, find_features_by_name and capabilities method would use it for a single feature. has_feature? would only add pluck and kept the any? part.
It's a small DRY-up I wouldn't insist on, but a valid enhancement.
There was a problem hiding this comment.
it is not saying explicitly, but if you look at the implementation we simply pass the argument to AR, which also accepts an array. So
has_feature?can be used with one or more feature names.
As an API consumer I would expect has_features? for an array and wouldn't even try passing an array to has_feature?.
On the other hand, there's probably more refactoring we can do. For example, I wouldn't mind splitting off all the feature related parts into its own file. associate_features is a large method that maybe should even be a service rather than a method on a model. Given that's a larger refactor, I don't insist on DRY-ing up this part.
Writing this down is sometimes more to structure my own thoughts and take others on the same journey than actually asking for it. Then they can make a more informed decision.
There was a problem hiding this comment.
dry'd up a bit, i can combine with has_feature? if desired. I don't see any issue within foreman or katello with changing that method to only take 1 in, but its possible some other plugin is doing something silly
|
@ShimShtein / @ares: all green. This looks good to me, but since you were involved as well I'll let you respond as well. |
|
@jlsherrill, can you leave a comment on the |
|
sorry that took a few days, was out 🤒 |
|
[test foreman] |
1 similar comment
|
[test foreman] |
While this is less efficient, the number of smart proxy features is low. This allows settings to be read on an unsaved objects, improving testing and allowing us to rely on settings in before_create.
While this is less efficient, the number of
smart proxy features is low. This allows settings
to be read on an unsaved objects, improving testing
and allowing us to rely on settings in before_create