Holidays with identical names can return incorrect results based on order in which regions are loaded

[ppeble]

Summary

Regions that share a holiday name but have different definitions for that holiday could see incorrect results based on the order that the regions are called.

Current Behavior

Loading the br region first means that the correct definition for co is not used, resulting in 2014-06-23 returning nothing.

~/Code/ppeble/holidays(master ✗) make console
bundle exec rake console
irb -r rubygems -I lib -r holidays.rb
irb(main):001:0> Holidays::Factory::Definition.regions_repository.all_loaded
=> []
irb(main):002:0> Holidays.on(Date.civil(2014, 6, 19), :br)
=> [{:date=>#<Date: 2014-06-19 ((2456828j,0s,0n),+0s,2299161j)>, :name=>"Corpus Christi", :regions=>[:br]}]
irb(main):003:0> Holidays::Factory::Definition.regions_repository.all_loaded
=> [:br]
irb(main):004:0> Holidays.on(Date.civil(2014, 6, 23), :co)
=> []
irb(main):005:0> Holidays::Factory::Definition.regions_repository.all_loaded
=> [:br, :co]

Expected Behavior

The holiday 'Corpus Christi' for the co region should be returned when querying for 2014-06-23 even if other regions also containing a 'Corpus Christi' holiday are loaded first.

Expected correct output while loading br region before co region:

~/Code/ppeble/holidays(master ✗) make console
bundle exec rake console
irb -r rubygems -I lib -r holidays.rb
irb(main):001:0> Holidays::Factory::Definition.regions_repository.all_loaded
=> []
irb(main):002:0> Holidays.on(Date.civil(2014, 6, 19), :br)
=> [{:date=>#<Date: 2014-06-19 ((2456828j,0s,0n),+0s,2299161j)>, :name=>"Corpus Christi", :regions=>[:br]}]
irb(main):003:0> Holidays::Factory::Definition.regions_repository.all_loaded
=> [:br]
irb(main):004:0> Holidays.on(Date.civil(2014, 6, 23), :co)
=> [{:date=>#<Date: 2014-06-23 ((2456832j,0s,0n),+0s,2299161j)>, :name=>"Corpus Christi", :regions=>[:co]}]
irb(main):005:0> Holidays::Factory::Definition.regions_repository.all_loaded
=> [:br, :co]

Steps to Reproduce

1. Have two regions that have a holiday with the exact same name.
2. Have the definitions for that holiday differ between the two regions (e.g. different function_modifier values or have them take place on different days).
3. Load holidays via make console.
4. Call the first region with the expected date matching its definition. You should receive the target holiday.
5. Call the second region with the differing date matching the second region's definition. You should receive an empty array.

The call to the second region will return [] because the first region's definition is used, even though the second region was explicitly provided as an argument.

Detailed Description

This specific scenario was found via the PR to fix Colombian holidays and only presents itself using the updates in that PR. However this behavior would exist in any other similar scenario.

The issue in this ruby implementation is that the co and br regions both have an entry named Corpus Christi but they have different logic:

Colombia:

- name: Corpus Christi
   regions: [co]
   function: easter(year)
   function_modifier: 64

Brazil:

- name: Corpus Christi
   regions: [br]
   function: easter(year)
   function_modifier: 60

[ppeble]

While writing tests I have discovered that this only seems to occur in the following situations:

Two regions each contains a function with the same name but different logic

1. :region_1 contains a function named my_custom_method
2. :region_2 contains a function that is ALSO named my_custom_method

From what I can tell when :region_2 is loaded it seems to overwrite the logic that was originally loaded by :region_1, resulting in incorrect behavior.

Two regions each contains a function with the same name and same logic but also have different function modifier values

1. :region_1 contains a function named easter with a function_modifier of 1
2. :region_2 contains a function that is ALSO named easter (with the same logic as :region_1) but with a function_modifier of 10

This scenario is what specifically triggered this issue (because of the conflict between br and co). This is most likely in the cases of using easter methods but then having different function_modifier values. The end result is that the function modifier from :region_1 seems to always take priority and the function_modifier value from :region_2 is never used.

I now have failing tests to cover the two scenarios above as well as passing tests for a variety of other similar scenarios that do not seem to trigger the incorrect behavior.

[ttwo32]

@ppeble Thank you for writing this issue.:)
This is a serious and dificcult problem.

[ppeble]

Quick update: I have a fix locally that addresses the second scenario from my last comment (regarding function_modifier). Unfortunately the first scenario is more tricky. It requires some refactoring of how we handle functions and it has a potentially large impact since functions are used in so many holiday calculations. Because of this I think I'll need another 1-2 days before I'm ready with a PR.

[ppeble]

After a lot more digging I have decided to open a PR with my fix for the function_modifier scenario only. This change should fix the issue from the definitions PR from which this issue sprouted and should allow us to release v8.0.0 without having to wait.

The other issue unfortunately requires a much more complex solution. I'll leave this issue open so I can continue to track progress on it but it will come in a later PR.