Feature/bill 729

[gusIreland]

Required Sections

Description

Issue Link

Added stupidedi gem. This PR comments out dependencies that cause conflicts with Onelife code The `blank?` method is monkeypatched, which can cause spec failures in onelife. This code comments out a requirement The specs still pass locally, though the gem is not setup for CI #### Screenshots

Regression Testing Notes

Any change to part of a complex system includes the risk of regression in other parts of the system that use the same underlying code. Please make a note of any functionality that may change as a side-effect of this PR. This will help the QE team make sure that no new regressions have been introduced

• [x] No expected regression risk
• [ ] Possible regression risks
  • Insert details here

Qual Environment

Risky Changes Check

Definition Of Risky Changes

• Changes that cannot be tested in an environment outside of production
  • Examples:
    • Changes to the configuration for the production environment like domain names
    • Data that is unique to production
    • Networking configurations in production
  • Anything that requires downtime

Verify If These Are Risky Changes

• [x] These are not risky changes as defined above

OR

• [ ] These are risky changes

Risky Changes Checklist

• [ ] Alert Infra and SRE along with relevant metrics to watch to ensure the changes are functional
• [ ] Determine if the changes can be released in a regularly scheduled maintenance window.
  • Generally this will be possible
  • There is a process document: link to document
• [ ] Follow the process for maintenance window changes
• [ ] In the instance it is not possible to release the changes in a maintenance window, follow the normal release process. Monitor the release with infra and SRE’s help to ensure that a rapid rollback is possible.

Optional sections

Dependencies

Database Impact

Diligence

• [ ] I have read https://github.com/onemedical/onelife/wiki/Database-Migrations-v2
• [ ] My migrations are safe, as per the above document

Details on the database impact of the change

• Scope of the migration:
• Expected time to complete:

Elasticsearch Impact

Diligence

• [ ] My Elasticsearch index changes are in accordance with the Elasticsearch zero-downtime reindexing process
• [ ] My Elasticsearch index changes will cause downtime
  • Index being modified:
  • Downtime justification:

Cache Impact

Diligence

• [ ] I have not modified the value of a cache entry such that it would become backwards incompatible
• [ ] I've ensured my new key is tested to the same degree that the old key was

Details on cache changes

• Old key:
• New key:
• Places in use:
• Reason for change:

Post Deploy Tasks

• rake tasks
  • [ ] I have notified DL-Engineers and #pd-engineers of any environment-breaking tasks that need to be run
• seed data

[kputnam]

Hi @gusIreland, I know you opened this by mistake, but I noticed the issue you're having regarding blank?. This library no longer uses monkey patching, and those customizations are implemented using refinements that only take effect when brought into scope with using Stupidedi::Refinements.

So the definitions of blank? used by stupidedi shouldn't be invoked by your client code, unless you write using Stupidedi::Refinements in the lexical scope of the caller. The other way those definitions could be called is indirectly, by calling something in stupidedi that calls blank?.

Your change to comment out the import of blank.rb might fix the rest of your specs. But it might also cause unexpected behavior in stupidedi itself, which relies on that particular definition. So here's some debugging code to help narrow down who/what exactly is calling the blank? method defined in stupidedi. Let me know if you run this and see something unexpected, like your code which doesn't have the refinements brought into scope, is calling String#blank? and it's somehow invoking stupidedi's definition. Hopefully you will find nothing besides stupidedi itself is calling that method, though.

# lib/ruby/blank.rb

refine XYZ do
  def blank?
    require "thread"
    $BLANK ||= Thread::Queue.new
    $BLANK << caller

    # original method body resumes here
    ...
  end
end

Then to print out the callers after your test suite finishes, you will need to add this:

# spec/spec_helper.rb

RSpec.configure do |config|
  config.after(:suite) do
    File.open("blank.txt", "w") do |io|
      if defined? $BLANK
        while xs = $BLANK.pop
          io.puts xs[0]
          io.puts xs[1..-1].grep_v(%r{/(?:gems|bin)/}).map{|x| "  #{x}"}
          io.puts
        end
      else
        io.puts "No callers"
      end
    end
  end
end

After running rspec you should see the output in a file named "blank.txt" in the working directory. You may need to change the regular expression given to grep_v if you want to see where blank? is being called within stupidedi, if you've installed it as a gem.

[gusIreland]

@kputnam Thanks for reaching out! The PR was pointed at the wrong repo, thanks for the debugging advice

I've tried using the debugging code, but haven't found any callers - the output in blank.txt is just No callers

The specific spec failure is

NameError:
       undefined method `blank?' for class `#<Class:0x00007feb96ae5810>'

so I can see how the sample debugging code would never actually find the caller

I'm not quite sure how to debug from here. I'm not familiar with refinements, and I can't see how the refinement would be brought into scope in our own code.

The specs still pass in stupidedi with the blank import removed, but I'd prefer to find a more longterm fix, any ideas?

[kputnam]

That's strange. Just to be sure, when you add the debug code and un-comment require "lib/stupidedi/blank" your specs fail and the debug file says "No callers". But when you comment out that require, your specs pass?

Also double checking, there are a few definitions of blank? in lib/stupidedi/blank.rb. Could you try un-commenting the require line and then commenting one definition of blank? at a time until your tests pass? When you narrow it down to which ones seem to be causing your specs to fail, then be sure you copy and paste the same debug code into each definition.

Refinements are a feature added to Ruby around version 2.0, and it's a way to redefine methods in a way that is limited to certain call sites (monkey patching changed a method definition globally). Quick example:

# patches.rb
############################################################

module Patches
  refine Array do
    def size
      "hello"
    end
  end
end

# isolated.rb
############################################################

def isolated(xs)
  xs.size
end

def isolated1(xs)
  refined(xs)
end

# refined.rb
############################################################

using Patches

def refined(xs)
  xs.size
end

def refined1(xs)
  isolated(xs)
end

# main.rb
############################################################

require_relative "patches"
require_relative "isolated"
require_relative "refined"

xs = [1, 2, 3]
puts isolated(xs) #=> 3
puts refined(xs)  #=> "hello"
puts
puts isolated1(xs) #=> "hello"
puts refined1(xs)  #=> 3

Hopefully that illustrates the idea. The scope of a refinement can be narrowed down smaller than the entire file (in refined.rb), by writing the using statement inside the method definition, or inside a class or module, etc. Most importantly, the widest scope that a using statement can alter is at the file level. That's why "isolated.rb" doesn't use the refinement, even though the using statement was written at the top of "refined.rb".

So with all that explained, it seems unlikely that your code is directly calling the patched definitions of size? unless you wrote using Stupidedi::Refinements above the call site somewhere. Also unlikely your code is calling some other gem which is in-turn affected by those refinements of size?, as they would have had to write using Stupidedi::Refinements above the call size.

One important thing though: you can see in the example that the isolated1 method indirectly uses the overridden definition of Array#size because it calls a method which imported the refinement. So I suspect somehow, your specs are calling a method in stupidedi, which in-turn calls the refined definition of size?. Maybe you're extracting a value from an X12 element and checking if it's empty, eg Stupidedi::Versions::Common::ElementTypes::AN#empty?, which calls the refined definition of size?. Hard to say though without more debugging info.