Monkey-patching briefly is modifying exterior code, whose supply we don’t straight management, to suit our particular function within the undertaking. When modernising framework stack in “legacy” initiatives that is typically a necessity when an improve of a dependency will not be but attainable or would contain transferring too many blocks directly.
It’s a short-term resolution to maneuver issues ahead. The reward we get from monkey-patching is on the spot. The code is modified with out asking for anybody’s permission and with out a lot further work {that a} dependency fork would contain.
Nevertheless it comes with a hefty worth of being very brittle. We completely can not count on {that a} monkey-patch would work with any future variations of the exterior dependency. Thus speaking this short-term mortgage is essential once we’re not soloing.
Guarding patched dependency adjustments with model examine
One strategy to talk a monkey-patched dependency is to doc it with a take a look at.
Why take a look at?
-
It’s near the modified code — within the undertaking supply, versus any exterior documentation medium.
-
It’s executable, in contrast to code remark and tremendously reduces the danger of somebody not noticing an announcement.
In a undertaking I’ve just lately labored on there was already an unannounced AcitveRecord::Persistence#reload methodology patch inside Consumer mannequin. I think about myself very fortunate recognizing inside over 910000 strains of code having solely 10% take a look at protection.
A code remark would positively not assist me discover it — I got here to this undertaking solely just lately and the authors have been already engaged on it earlier than have been gone.
A take a look at I’ve added to doc it regarded like this:
# spec/fashions/user_spec.rb
require "rails_helper"
RSpec.describe "Consumer" do
specify "#reload methodology is overridden based mostly on framework implementation" do
count on(Rails.model).to eq("5.1.7"), failure_message
finish
def failure_message
<<~WARN
It seems to be such as you upgraded Rails.
Examine if Consumer#reload methodology physique corresponds to the present Rails model implementation of rails/activerecord/lib/active_record/persistence.rb#reload.
When it is prepared bump the model on this situation.
WARN
finish
finish
Now at any time when Rails model adjustments, this examine is meant to fail. The failure has a descriptive message instructing what must be checked with a view to delay the patch.
Inside an organisation counting on steady integration and aspiring to the testing tradition this must be sufficient to stop failure from such patching.
However is it sufficient developer-friendly?
Bettering model examine
One downside of strict model checks is …being too strict. Some dependencies are greatest allowed inside a variety of attainable variations. To not fail the examine on model adjustments mitigating safety points for instance:
--- spec/app/fashions/user_spec.rb
+++ spec/app/fashions/user_spec.rb
@@ -4,7 +4,7 @@ require 'rails_helper'
- count on(Rails.model).to eq('7.0.7'), failure_message
+ count on(Rails.model).to eq('7.0.7.2'), failure_message
+ # Mitigate CVE-2023-38037
+ # https://focus on.rubyonrails.org/t/cve-2023-38037-possible-file-disclosure-of-locally-encrypted-files/83544
Once we’re sure {that a} dependency follows a significant model numbering scheme, we will change the examine to confirm extra relaxed model constraints.
An instance utilizing RubyGems API:
--- spec/app/fashions/user_spec.rb
+++ spec/app/fashions/user_spec.rb
@@ -4,7 +4,7 @@ require 'rails_helper'
- count on(Rails.model).to eq('7.0.7.2'), failure_message
+ count on(Gem::Requirement.create('~> 7.0.0').satisfied_by?(Gem::Model.create(Rails.model))).to eq(true), failure_message
However do we all know for certain {that a} security-patch-release doesn’t change the code we’ve patched?
Checking if the supply didn’t change
It could be ideally suited if we might peek into the supply of the tactic we’re patching and inform if it has modified because the unique one.
Studying a tweet from my former arkency fellow shed new mild on the difficulty.
An entire take a look at utilising this method might seem like this:
# spec/fashions/user_spec.rb
require "rails_helper"
RSpec.describe "Consumer" do
specify "#reload methodology is overridden based mostly on framework implementation" do
count on(checksum_of_actual_reload_implementation).to eq(
checksum_of_expected_reload_implementation,
),
failure_message
non-public
def checksum_of_actual_reload_implementation
Digest::SHA256.hexdigest(
ActiveRecord::Persistence.instance_method(:reload).supply,
)
finish
def checksum_of_expected_reload_implementation
"3bf4f24fb7f24f75492b979f7643c78d6ddf8b9f5fbd182f82f3dd3d4c9f1600"
finish
def failure_message
#...
finish
finish
finish
The little disappointment got here shortly after I’ve realised Technique#supply will not be a typical Ruby. It’s from a method_source dependency that got here to the undertaking I’ve labored on not directly through pry. However it labored inside the scope of present undertaking dependencies and was higher than a plain model examine.
Can we do any higher?
Checking Summary Syntax Tree of the implementation
I admit that computing the hash of the supply code is neat. Nevertheless, it falls in need of “formatting” adjustments. Supply code is a textual illustration. Introducing whitespace characters — areas or line breaks doesn’t change the implementation. It behaves the identical. The hash will probably be totally different although, elevating a false destructive.
So can we do it higher? Sure, we will. With little assist of AST. In idea AST illustration ought to free us from how the patched code is formatted.
In Ruby, we have now a couple of choices to render AST of the supply code. The favored parser and syntax_tree gems. The Ripper in the usual library. Or the native RubyVM::AbstractSyntaxTree.
A pessimist might discover their limitations first:
-
RubyVM::AbstractSyntaxTreeandRippernonetheless embrace formatting within the output, defeating the aim -
parserandsyntax_treeare exterior dependencies, so not universally relevant — likelihood is they’re already a transitive dependency in your undertaking
I positively didn’t see all of it at first sight. Listed here are the implementations I might not advocate.
False hopes for checksum free from formatting
In core Ruby there may be RubyVM::AbstractSyntaxTree module, which supplies strategies to parse Ruby code into summary syntax timber. Sadly, the output consists of line and column info, making it unfit for checksumming impartial of supply formatting. Thus it isn’t higher in any facet than hexdigest on plain supply code.
# spec/fashions/user_spec.rb
require "rails_helper"
RSpec.describe "Consumer" do
specify "#reload methodology is overridden based mostly on framework implementation" do
count on(checksum_of_actual_reload_implementation).to eq(
checksum_of_expected_reload_implementation,
),
failure_message
finish
non-public
def checksum_of_actual_reload_implementation
Digest::SHA256.hexdigest(
RubyVM::AbstractSyntaxTree.parse(
ActiveRecord::Persistence.instance_method(:reload).supply,
).pretty_print_inspect,
)
finish
def checksum_of_expected_reload_implementation
"ed2f4fdf62aece74173a44a65d8919ecf3e0fca7a5d38e2cefb9e51c408a4ab4"
finish
finish
No checksum for the additional advantage of seeing precise implementation adjustments
In Ruby normal library we might also discover Ripper, a Ruby script parser. It parses the code right into a symbolic expression tree. Sadly this too incorporates line and column info within the output. Maybe with some further post-processing step, we might do away with it. I choose evaluating s-expressions to checksums — the take a look at framework has an opportunity to indicate variations within the in contrast syntax timber. Which is a pleasant bonus!
# spec/fashions/user_spec.rb
require "rails_helper"
RSpec.describe "Consumer" do
specify "#reload methodology is overridden based mostly on framework implementation" do
count on(actual_find_record_implementation).to eq(
expected_find_record_implementation,
),
failure_message
finish
non-public
def actual_reload_implementation
Ripper.sexp(ActiveRecord::Persistence.instance_method(:reload).supply)
finish
def expected_reload_implementation
[
:program,
[
[
:def,
[:@ident, "reload", [1, 8]],
[
:paren,
[
:params,
nil,
[
[
[:@ident, "options", [1, 15]],
[:var_ref, [:@kw, "nil", [1, 25]]],
],
],
nil,
nil,
nil,
nil,
nil,
],
],
[
:bodystmt,
[
[
:call,
[
:call,
[
:call,
[:var_ref, [:@kw, "self", [2, 6]]],
[:@period, ".", [2, 10]],
[:@ident, "class", [2, 11]],
],
[:@period, ".", [2, 16]],
[:@ident, "connection", [2, 17]],
],
[:@period, ".", [2, 27]],
[:@ident, "clear_query_cache", [2, 28]],
],
[
:assign,
[:var_field, [:@ident, "fresh_object", [4, 6]]],
[
:if,
[
:method_add_arg,
[:fcall, [:@ident, "apply_scoping?", [4, 24]]],
[
:arg_paren,
[
:args_add_block,
[[:var_ref, [:@ident, "options", [4, 39]]]],
false,
],
],
],
[
[
:method_add_arg,
[:fcall, [:@ident, "_find_record", [5, 8]]],
[
:arg_paren,
[
:args_add_block,
[[:var_ref, [:@ident, "options", [5, 21]]]],
false,
],
],
],
],
[
:else,
[
[
:method_add_block,
[
:call,
[
:call,
[:var_ref, [:@kw, "self", [7, 8]]],
[:@period, ".", [7, 12]],
[:@ident, "class", [7, 13]],
],
[:@period, ".", [7, 18]],
[:@ident, "unscoped", [7, 19]],
],
[
:brace_block,
nil,
[
[
:method_add_arg,
[:fcall, [:@ident, "_find_record", [7, 30]]],
[
:arg_paren,
[
:args_add_block,
[[:var_ref, [:@ident, "options", [7, 43]]]],
false,
],
],
],
],
],
],
],
],
],
],
[
:assign,
[:var_field, [:@ivar, "@association_cache", [10, 6]]],
[
:method_add_arg,
[
:call,
[:var_ref, [:@ident, "fresh_object", [10, 27]]],
[:@period, ".", [10, 39]],
[:@ident, "instance_variable_get", [10, 40]],
],
[
:arg_paren,
[
:args_add_block,
[
[
:symbol_literal,
[:symbol, [:@ivar, "@association_cache", [10, 63]]],
],
],
false,
],
],
],
],
[
:assign,
[:var_field, [:@ivar, "@attributes", [11, 6]]],
[
:method_add_arg,
[
:call,
[:var_ref, [:@ident, "fresh_object", [11, 20]]],
[:@period, ".", [11, 32]],
[:@ident, "instance_variable_get", [11, 33]],
],
[
:arg_paren,
[
:args_add_block,
[
[
:symbol_literal,
[:symbol, [:@ivar, "@attributes", [11, 56]]],
],
],
false,
],
],
],
],
[
:assign,
[:var_field, [:@ivar, "@new_record", [12, 6]]],
[:var_ref, [:@kw, "false", [12, 20]]],
],
[
:assign,
[:var_field, [:@ivar, "@previously_new_record", [13, 6]]],
[:var_ref, [:@kw, "false", [13, 31]]],
],
[:var_ref, [:@kw, "self", [14, 6]]],
],
nil,
nil,
nil,
],
],
],
]
finish
def failure_message
# ...
finish
finish
The ultimate boss
Last, “pragmatic” implementation that I’m sticking with. It is determined by parser and method_source gems. I’ve made peace with them, as they’re already within the undertaking through pry, mutant and rubocop additions.
require "parser/present"
RSpec.describe "Consumer" do
embrace AST::Sexp
specify "#reload methodology is overridden based mostly on framework implementation" do
count on(actual_find_record_implementation).to eq(
expected_find_record_implementation,
),
failure_message
finish
non-public
def actual_reload_implementation
Parser::CurrentRuby.parse(
ActiveRecord::Persistence.instance_method(:reload).supply,
)
finish
def expected_reload_implementation
s(
:def,
:reload,
s(:args, s(:optarg, :choices, s(:nil))),
s(
:start,
s(
:ship,
s(:ship, s(:ship, s(:self), :class), :connection),
:clear_query_cache,
),
s(
:lvasgn,
:fresh_object,
s(
:if,
s(:ship, nil, :apply_scoping?, s(:lvar, :choices)),
s(:ship, nil, :_find_record, s(:lvar, :choices)),
s(
:block,
s(:ship, s(:ship, s(:self), :class), :unscoped),
s(:args),
s(:ship, nil, :_find_record, s(:lvar, :choices)),
),
),
),
s(
:ivasgn,
:@association_cache,
s(
:ship,
s(:lvar, :fresh_object),
:instance_variable_get,
s(:sym, :@association_cache),
),
),
s(
:ivasgn,
:@attributes,
s(
:ship,
s(:lvar, :fresh_object),
:instance_variable_get,
s(:sym, :@attributes),
),
),
s(:ivasgn, :@new_record, s(:false)),
s(:ivasgn, :@previously_new_record, s(:false)),
s(:self),
),
)
finish
def failure_message
# ...
finish
finish
As you possibly can see, there aren’t any line or column references within the output.
For the portability, I want these dependencies weren’t wanted. Hopefully someday this all will probably be simpler sooner or later Ruby:
Fingers crossed 🤞
