search

active-hash / active_hash

A readonly ActiveRecord-esque base class that lets you use a hash, a Yaml file or a custom file as the datasource
MIT License
1.21k stars 179 forks source link

fix: do not suppress errors when loading classes #309

Closed andreynering closed 6 months ago

andreynering commented 6 months ago

This rescue block was suppressing real errors on our codebase. I had to remove this block on a local fork to be able to see the real cause of a bug that was silently happening.

I noticed that removing the blocks do not break any tests. Looks like we should just remove them?

kbrock commented 6 months ago

@andreynering Thanks. good catch. I think it was missed when we converted the constantize to safe_constantize in #297

Can you share the example of what error was being squashed here? (for my own personal knowledge - no code necessary but a quick description would be appreciated)

I did have concerns with the constantize here since loading the class here has caused race conditions in another codebase. But the added benefits of checking this class outweighed the rare issue.

Do you think you could put together a test here for when we specify an invalid class? (if it is super hard, then just say so and we can punt.

@flavorjones I missed this try/catch when you added the safe_constantize. I'm assuming that we just missed it and I'd like to merge this change.

But I do have to ask: Did you leave this try/catch for a particular reason? There is an issue when the inflection for the class is slightly different. (e.g.: class named VM but we try and load Vm) Is this the reason you left it in? Or was it just an oversight? This is a very rare edge case, but wanted to double check.

andreynering commented 6 months ago

@kbrock We had a concern included on a given ActiveRecord model, but the content of included do; end block wasn't actually been ran. Ends up there was an error being raised, but that block was suppressing it.