Reimplement CDO Configuration Object without OpenStruct - #51493
Conversation
Hamms
added
the
Ruby Update
| # Inspired by OpenStruct; if unfrozen, allow assigning new configuration | ||
| # values with `=` and default undefined values to nil | ||
| # See https://github.com/ruby/ostruct/blob/e61b4464a033c38a65657eaf0467d12e2c7b9ec1/lib/ostruct.rb#L207-L229 | ||
| def method_missing(mid, *args) |
There was a problem hiding this comment.
Could we use ActiveSupport::OrderedOptions? Looks like it also overrides method_missing to provide the functionality we need, and it sub-classes Hash. https://www.akshaykhot.com/accessing-hash-values-like-methods/
There was a problem hiding this comment.
In theory I think it'd probably be possible, but would require much more extensive refactoring than this approach. In particular, our dynamic configuration rerendering logic as it's currently implemented requires us to be able to cache and restore the underlying data store between each interation, and I have no idea how we'd go about doing that with something that subclasses rather than wraps its data store. I'd also be worried about the couple of instances of full-table iteration that we currently do in the secrets config.
I'd prefer to stick with something that maintains existing functionality for now, rather than embark on a full rewrite of this class
There was a problem hiding this comment.
See also this piece on why subclassing hashes causes pain and performance problems
There was a problem hiding this comment.
Even if a long-term port to OrderedOptions makes sense, I think its smart to go step-wise, I like the approach of extracting and reworking some bits of OpenStruct to make this a relatively minimal change
snickell
left a comment
snickell
left a comment
There was a problem hiding this comment.
Looks good to my relatively out-of-shape ruby eyes. Thanks for tackling moving away from a not-recommended structure.
| # Inspired by OpenStruct; if unfrozen, allow assigning new configuration | ||
| # values with `=` and default undefined values to nil | ||
| # See https://github.com/ruby/ostruct/blob/e61b4464a033c38a65657eaf0467d12e2c7b9ec1/lib/ostruct.rb#L207-L229 | ||
| def method_missing(mid, *args) |
There was a problem hiding this comment.
Even if a long-term port to OrderedOptions makes sense, I think its smart to go step-wise, I like the approach of extracting and reworking some bits of OpenStruct to make this a relatively minimal change
| end | ||
|
|
||
| def freeze | ||
| def freeze_config |
There was a problem hiding this comment.
My biggest question is "why create a new method rather than overloading freeze"? Config::freeze_config feels slightly redundant. But really not a big deal at all.
There was a problem hiding this comment.
Mostly because we don't want to actually freeze the Config object itself or to in any way take advantage of existing freeze functionality; we want to implement something analogous but distinct to native freeze. If the way we implemented this was to instead call @table.freeze and rely on the table being immutable for our table mutation methods to fail, then it'd make sense to me to handle this by overloading freeze on the Config object, but because we do something much more bespoke it feels right that it's all happening in a custom method.
There was a problem hiding this comment.
Maybe name the method ice instead? That implies the values will sometimes be thawed :)
3 participants
OpenStruct has undergone some changes in Ruby 3.0 which break a few things with our CDO configuration option management object which inherits from it. We could probably patch up those incompatibilities, but OpenStruct is generally not recommended for use, so instead we just reimplement the specific functionality that we want on the class directly, and stop inheriting from it.
The specific OpenStruct functionality that we lose here is mostly a lot of logic to support redefining values that have already been defined (something we explicitly want to avoid anyway), as well as workarounds to prevent accidental overloading of various internal methods (something our specific use case of the structure renders irrelevant), and various miscellaneous YAGNI helpers that we aren't currently taking advantage of. I also chose not to carry over the manual protected accessor for the internal table object in favor of explicit
@tablereferences, and to define our ownfreeze_configmethod for applying our custom soft-freeze logic rather than overriding the builtin method just so we can prevent the default functionality.Links
Testing story
Tweaked existing unit tests to reflect new slightly modified edge cases and method names. Otherwise relying on existing test coverage to verify that this does not result in any significant change in functionality.
Also tested on an adhoc