Cleanup constantize of worker_class_names #21470
Conversation
agrare
force-pushed
the
worker_class_name_constantize
branch
from
64f667f to
286a981
Compare
agrare
force-pushed
the
worker_class_name_constantize
branch
from
286a981 to
86ac151
Compare
| _log.log_backtrace(error) | ||
| next | ||
| end | ||
| MiqWorkerType.worker_class_names.map(&:constantize).each_with_object({}) do |klass, result| |
There was a problem hiding this comment.
Wondering if MiqWorkerType.worker_class_names.map(&:constantize) should be extracted to a method like MiqWorkerType.worker_classes
There was a problem hiding this comment.
💯 may or may not already have that branch as a follow-up to this :)
There was a problem hiding this comment.
doing a search in the core repo shows
$ git grep worker_class_names
app/models/miq_server/worker_management/monitor.rb:44: MiqWorkerType.worker_class_names.each do |class_name|
app/models/miq_server/worker_management/monitor.rb:163: MiqWorkerType.worker_class_names_in_kill_order.each do |class_name|
app/models/miq_server/worker_management/monitor/settings.rb:11: MiqWorkerType.worker_class_names.each do |class_name|
app/models/miq_server/worker_management/monitor/start.rb:13: starting = MiqWorker.find_starting.find_all { |w| MiqWorkerType.worker_class_names.include?(w.class.name)
app/models/miq_server/worker_management/monitor/systemd.rb:51: MiqWorkerType.worker_class_names.map(&:constantize).map(&:service_base_name)
app/models/miq_worker_type.rb:27: def self.worker_class_names
app/models/miq_worker_type.rb:31: def self.worker_class_names_in_kill_order
spec/models/miq_server/worker_management/monitor_spec.rb:99: allow(MiqWorkerType).to receive(:worker_class_names).and_return(%w[ManageIQ::Providers::Foreman::Provis
spec/models/miq_server/worker_management/monitor_spec.rb:107: allow(MiqWorkerType).to receive(:worker_class_names).and_return(%w(MiqGenericWorker MiqPriorityWorker)
spec/models/miq_server/worker_management/monitor_spec.rb:114: allow(MiqWorkerType).to receive(:worker_class_names).and_return([])
spec/models/miq_worker/systemd_common_spec.rb:10: found_units = MiqWorkerType.worker_class_names.flat_map do |klass_name|I would bet that each of those callers is doing a constantize to get the actual classes.
There was a problem hiding this comment.
Yes not all of them are but the majority
There was a problem hiding this comment.
Note, worker_classes could add that constantize check on the class name matching the worker type name if that is really still needed. Looking at my original PR, it looks like the problem was in const_get doing the wrong thing. I guess that could still happen if rails autoload still uses it. 🤷
I can look to see if that's still an issue. But here's more context: #19400 (comment)
There was a problem hiding this comment.
Right AFAIK constantizing classes which don't exist won't always fail (could resolve to the non-namespaced base class), but constantizing classes which do exist will always resolve to the right thing?
There was a problem hiding this comment.
@agrare I believe that's correct.
There was a problem hiding this comment.
Note that newer versions of Ruby / Rails may have eliminated this problem entirely as well, but in this case you know all the values will exist (because they were derived from existing values), so you are safe.
| MiqWorkerType.worker_class_names.each do |class_name| | ||
| begin | ||
| c = class_name.constantize | ||
| raise NameError, "Constant problem: expected: #{class_name}, constantized: #{c.name}" unless c.name == class_name |
There was a problem hiding this comment.
Looks like this line was lost in the refactoring. It was probably added for better diagnostics.
There was a problem hiding this comment.
Well removing this was actually the point of this change. We have control over what makes it into miq_worker_types so this shouldn't happen. I believe this was from before we had the miq_worker_types table back when it was self.class.monitor_class_names
There was a problem hiding this comment.
No, this was added specifically because of that weird issue with Ruby where you can ask for a particular constant and get back the wrong one if someone happened to defined one in your module ancestry. See the commit message in afe1f02
There was a problem hiding this comment.
Right, but how would one of those "incorrect" class names make it in to MiqWorkerType ?
There was a problem hiding this comment.
Yeah I was thinking if we prevent bad class names from hitting MiqWorkerType in the first place then we don't need to check this at runtime, and everyone else who does constantize on a worker_class_name doesn't have to duplicate this check.
There was a problem hiding this comment.
@agrare Oh I'm not disagreeing with you ...just clarifying what the purpose of this line actually was and showing that yes, it was from before the refactor to a table
There was a problem hiding this comment.
Yeah, I agree. You're both right. 👍
| it "contains properly subclassed workers", :providers_common => true do | ||
| described_class.worker_class_names.each do |class_name| | ||
| klass = class_name.constantize | ||
| raise NameError, "Constant problem: expected: #{class_name}, constantized: #{klass.name}" unless klass.name == class_name |
There was a problem hiding this comment.
This should catch the same issues we were handling at runtime but before they make it in
There was a problem hiding this comment.
Note, I don't know that this would catch all of the errors because the problems occur when the parts of the namespace are loaded out of order or not the order that's expected and Rails tries to autoload something from a partially loaded tree.
I do think this is hopefully fixed with zeitwerk if we ever get to that.
I'm fine with not doing this at runtime (the removal in the code above) for 2 reasons:
- Output constant must match the constantized class name #19400 didn't actually fix anything, it only reported the problem early instead constantizing the wrong constant name and trying to call a method on the wrong constant.
- We haven't seen this NameError issue reported since the code was added so I'm guessing it's not a common thing and letting it fail on it's own as an exception is just as good, until we can fix this with zeitwerk.
There was a problem hiding this comment.
I recall it basically finding bugs where people didn't subclass things properly, and once those were all worked out, we never saw it again, but it was a good safety net. That being said, safety nets are better when in specs, so I think this is good.
It's basically not possible to happen anymore because of the way the code works now, where it seeds the table with the "real" class names determined by calling descendants, and then the worker_class_names queries the table. Even so, I like having this test in case we change something in the future.
There was a problem hiding this comment.
yeah, I edited my comment. I meant, the move from code to spec is fine. I am fine with it in specs as it describes our expectation. The code removal is what I think wasn't really doing too much. Just reporting early in the unlikely event this incorrect loading of classes occured.
agrare
force-pushed
the
worker_class_name_constantize
branch
from
4c9f4d4 to
f30b447
Compare
|
Checked commits agrare/manageiq@86ac151~...f30b447 with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint |
|
Kicking travis now that the issue with xenial / ca certs is fixed |
Now that we only pull worker class names from the seeded database this seems like it shouldn't be an issue anymore but would like @jrafanie's opinion