Thursday, June 13, 2024
HomeRuby On RailsSix methods to stop a monkey-patch drift from the unique code

Six methods to stop a monkey-patch drift from the unique code


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?

  1. It’s near the modified code — within the undertaking supply, versus any exterior documentation medium.

  2. 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::AbstractSyntaxTree and Ripper nonetheless embrace formatting within the output, defeating the aim

  • parser and syntax_tree are 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 🤞



RELATED ARTICLES

LEAVE A REPLY

Please enter your comment!
Please enter your name here

Most Popular

Recent Comments