active_hash validates the IDs correctly

Gemfile

@@ -16,7 +16,7 @@ gem 'grape-swagger-entity'
 gem 'grape_logging'
 
 gem 'actionpack'
-gem 'active_hash', github: 'Mapotempo/active_hash', branch: 'mapo'
+gem 'active_hash', github: 'mapotempo/active_hash', branch: 'dev' # waiting for the following PRs to get merged and "released!" https://github.com/zilkey/active_hash/pull/231 and https://github.com/zilkey/active_hash/pull/233
 gem 'activemodel'
 
 gem 'charlock_holmes'

Gemfile.lock

@@ -1,11 +1,3 @@
-GIT
-  remote: https://github.com/Mapotempo/active_hash.git
-  revision: e2b18fedb32aec0cc03f0c747dbc682cbb8fc488
-  branch: mapo
-  specs:
-    active_hash (3.1.0)
-      activesupport (>= 5.0.0)
-
 GIT
   remote: https://github.com/Mapotempo/balanced_vrp_clustering.git
   revision: c76cca8fe53bf802aca2f998631f7c24c59cb3c4
@@ -16,6 +8,14 @@ GIT
       color-generator
       geojson2image
 
+GIT
+  remote: https://github.com/mapotempo/active_hash.git
+  revision: d04b246227675806f040be09e7bf0dfaa8ffc590
+  branch: dev
+  specs:
+    active_hash (3.1.0)
+      activesupport (>= 5.0.0)
+
 GIT
   remote: https://github.com/senhalil/rack.git
   revision: a23188bb32972dde155977655f57dea035eee6ea

lib/heuristics/dichotomious_approach.rb

@@ -89,15 +89,15 @@ def self.dichotomious_heuristic(service_vrp, job = nil, &block)
             if sub_service_vrp[:vrp].resolution_minimum_duration
               sub_service_vrp[:vrp].resolution_minimum_duration *= sub_service_vrp[:vrp].services.size / service_vrp[:vrp].services.size.to_f * 2
             end
+            matrix_indices = sub_service_vrp[:vrp].points.map{ |point|
+              service_vrp[:vrp].points.find{ |r_point| point.id == r_point.id }.matrix_index
+            }
+            SplitClustering.update_matrix_index(sub_service_vrp[:vrp])
+            SplitClustering.update_matrix(service_vrp[:vrp].matrices, sub_service_vrp[:vrp], matrix_indices)
             result = OptimizerWrapper.define_process(sub_service_vrp, job, &block)
-            if index.zero? && result
-              transfer_unused_vehicles(result, sub_service_vrps)
-              matrix_indices = sub_service_vrps[1][:vrp].points.map{ |point|
-                service_vrp[:vrp].points.find{ |r_point| point.id == r_point.id }.matrix_index
-              }
-              SplitClustering.update_matrix_index(sub_service_vrps[1][:vrp])
-              SplitClustering.update_matrix(service_vrp[:vrp].matrices, sub_service_vrps[1][:vrp], matrix_indices)
-            end
+
+            transfer_unused_vehicles(result, sub_service_vrps) if index.zero? && result
+
             result
           }
           service_vrp[:vrp].resolution_split_number = sub_service_vrps[1][:vrp].resolution_split_number
@@ -212,9 +212,7 @@ def self.build_initial_routes(results)
           next if mission_ids.empty?
 
           Models::Route.create(
-            vehicle: {
-              id: route[:vehicle_id]
-            },
+            vehicle_id: route[:vehicle_id],
             mission_ids: mission_ids
           )
         }

lib/heuristics/periodic_heuristic.rb

@@ -1167,7 +1167,7 @@ def prepare_output_and_collect_routes(vrp)
           computed_activities, start_time, end_time = get_activities(day, route_data, vrp, vrp_vehicle)
 
           routes << {
-            vehicle: { id: vrp_vehicle.id },
+            vehicle_id: vrp_vehicle.id,
             mission_ids: computed_activities.collect{ |stop| stop[:service_id] }.compact
           }
 

lib/tsp_helper.rb

@@ -19,14 +19,15 @@
 module TSPHelper
   def self.create_tsp(vrp, vehicle)
     services = []
+    tsp_suffix = Digest::MD5.hexdigest((vrp.points.map(&:id) + vrp.services.map(&:id) + vrp.vehicles.map(&:id)).join)
     vrp.points.each{ |pt|
       service = vrp.services.find{ |service_| service_.activity.point_id == pt.id }
       next if !service
 
       services << {
-        id: service[:id],
+        id: "#{tsp_suffix}_#{service[:id]}",
         activity: {
-          point_id: service.activity.point_id,
+          point_id: "#{tsp_suffix}_#{service.activity.point_id}",
           duration: service.activity.duration
         }
       }
@@ -36,19 +37,19 @@ def self.create_tsp(vrp, vehicle)
       matrices: vrp.matrices,
       points: vrp.points.collect{ |pt|
         {
-          id: pt.id,
+          id: "#{tsp_suffix}_#{pt.id}",
           matrix_index: pt.matrix_index
         }
       },
       vehicles: [{
-        id: vehicle.id,
-        start_point_id: vehicle.start_point_id,
+        id: "#{tsp_suffix}_#{vehicle.id}",
+        start_point_id: "#{tsp_suffix}_#{vehicle.start_point_id}",
         matrix_id: vehicle.matrix_id
       }],
       services: services
     }
 
-    Models::Vrp.create(problem)
+    Models::Vrp.create(problem, false)
   end
 
   def self.solve(tsp)

models/concerns/distance_matrix.rb

@@ -58,7 +58,6 @@ def compute_need_matrix(&block)
       }.uniq
 
       i = 0
-      id = 0
       uniq_need_matrix = Hash[uniq_need_matrix.collect{ |mode, dimensions, options|
         block&.call(nil, i += 1, uniq_need_matrix.size, 'compute matrix', nil, nil, nil)
         # set matrix_time and matrix_distance depending of dimensions order
@@ -67,7 +66,6 @@ def compute_need_matrix(&block)
         router_matrices = OptimizerWrapper.router.matrix(OptimizerWrapper.config[:router][:url], mode, dimensions, matrix_points, matrix_points, options)
         log "matrix computed in #{(Time.now - tic).round(2)} seconds"
         m = Models::Matrix.create(
-          id: 'm' + (id += 1).to_s,
           time: (router_matrices[dimensions.index(:time)] if dimensions.index(:time)),
           distance: (router_matrices[dimensions.index(:distance)] if dimensions.index(:distance)),
           value: (router_matrices[dimensions.index(:value)] if dimensions.index(:value))

models/concerns/validate_data.rb

@@ -27,7 +27,6 @@ def check_consistency(hash)
     hash[:relations] ||= []
     @hash = hash
 
-    ensure_uniq_ids
     ensure_no_conflicting_skills
 
     configuration = @hash[:configuration]
@@ -44,16 +43,6 @@ def check_consistency(hash)
     check_configuration(configuration, periodic_heuristic) if configuration
   end
 
-  def ensure_uniq_ids
-    # TODO: Active Hash should be checking this
-    [:matrices, :units, :points, :rests, :zones, :timewindows,
-     :vehicles, :services, :subtours].each{ |key|
-      next if @hash[key]&.collect{ |v| v[:id] }&.uniq!.nil?
-
-      raise OptimizerWrapper::DiscordantProblemError.new("#{key} IDs should be unique")
-    }
-  end
-
   def ensure_no_conflicting_skills
     all_skills = (@hash[:vehicles] + @hash[:services]).flat_map{ |mission|
       mission[:skills]

optimizer_wrapper.rb

@@ -147,6 +147,7 @@ def self.define_main_process(services_vrps, job = nil, &block)
             msg = "#{"repetition #{repetition_index + 1}/#{service_vrp_repeats.size} - " if service_vrp_repeats.size > 1}#{message}" unless message.nil?
             callback_join&.call(wrapper, avancement, total, msg, cost, time, solution)
           }
+          Models.delete_all if service_vrp_repeats.size > 1 # needed to prevent duplicate ids because expand_repeat uses Marshal.load/dump
         }
 
         (result, position) = repeated_results.each.with_index(1).min_by { |result, _| result[:unassigned].size } # find the best result and its index

test/lib/heuristics/periodic_functions_test.rb

@@ -166,7 +166,9 @@ def test_add_missing_visits
       vrp = TestHelper.load_vrp(self, fixture_file: 'periodic_with_post_process')
       vrp.vehicles = TestHelper.expand_vehicles(vrp)
       s = Wrappers::PeriodicHeuristic.new(vrp)
-      s.instance_variable_set(:@candidate_routes, Marshal.load(File.binread('test/fixtures/add_missing_visits_candidate_routes.bindump'))) # rubocop: disable Security/MarshalLoad
+      add_missing_visits_candidate_routes = Marshal.load(File.binread('test/fixtures/add_missing_visits_candidate_routes.bindump')) # rubocop: disable Security/MarshalLoad
+      add_missing_visits_candidate_routes.each_value{ |vehicle| vehicle.each_value{ |route| route[:matrix_id] = vrp.vehicles.first.matrix_id } }
+      s.instance_variable_set(:@candidate_routes, add_missing_visits_candidate_routes)
       s.instance_variable_set(:@uninserted, Marshal.load(File.binread('test/fixtures/add_missing_visits_uninserted.bindump'))) # rubocop: disable Security/MarshalLoad
       services_data = Marshal.load(File.binread('test/fixtures/add_missing_visits_services_data.bindump')) # rubocop: disable Security/MarshalLoad
       s.instance_variable_set(:@services_data, services_data)

test/lib/interpreters/split_clustering_test.rb

@@ -29,21 +29,19 @@ def test_same_location_different_clusters
       vrp = TestHelper.load_vrp(self, fixture_file: 'cluster_dichotomious')
       service_vrp = { vrp: vrp, service: :demo }
       vrp.services.each{ |s| s.skills = [] } # The instance has old "Pas X" style day skills, we purposely ignore them otherwise balance is not possible
+      vrp.vehicles = [vrp.vehicles.first]    # entity: `vehicle` setting only works if the number of clusters is equal to the number of vehicles.
+
       while service_vrp[:vrp].services.size > 100
         total = Hash.new(0)
         service_vrp[:vrp].services.each{ |s| s.quantities.each{ |q| total[q.unit.id] += q.value } }
         service_vrp[:vrp].vehicles.each{ |v|
-          v.capacities = []
-          service_vrp[:vrp].units.each{ |u|
-            v.capacities << Models::Capacity.create(unit: u, limit: total[u.id] * 0.65)
+          v.capacities = service_vrp[:vrp].units.collect{ |u|
+            Models::Capacity.create(unit: u, limit: total[u.id] * 0.65)
           }
         }
 
-        # entity: `vehicle` setting only works if the number of clusters is equal to the number of vehicles.
         original = service_vrp[:vrp].vehicles.first
-        service_vrp[:vrp].vehicles = [original]
         service_vrp[:vrp].vehicles << Models::Vehicle.create(
-          id: "#{original.id}_copy",
           duration: original.duration,
           matrix_id: original.matrix_id,
           skills: original.skills,

test/models/vrp_consistency_test.rb

@@ -154,7 +154,7 @@ def test_point_id_not_defined
       exception = assert_raises ActiveHash::RecordNotFound do
         OptimizerWrapper.wrapper_vrp('demo', { services: { vrp: [:demo] }}, TestHelper.create(vrp), nil)
       end
-      assert_equal('Couldn\'t find Models::Point with ID=missing_point_id', exception.message)
+      assert_equal("Couldn't find Models::Point with ID=#{vrp[:services][0][:activity][:point_id].inspect}", exception.message)
     end
 
     def test_reject_if_shipments_and_periodic_heuristic
@@ -246,64 +246,30 @@ def test_available_intervals_compatibility
     end
 
     def test_duplicated_ids_are_not_allowed
-      assert TestHelper.create(VRP.basic) # this should not produce any error
-
-      vrp = VRP.basic
-      vrp[:vehicles] << vrp[:vehicles].first
-      assert_raises OptimizerWrapper::DiscordantProblemError do
-        TestHelper.create(vrp)
-      end
-
-      vrp = VRP.basic
-      vrp[:services] << vrp[:services].first
-      assert_raises OptimizerWrapper::DiscordantProblemError do
-        TestHelper.create(vrp)
-      end
-
-      vrp = VRP.basic
-      vrp[:matrices] << vrp[:matrices].first
-      assert_raises OptimizerWrapper::DiscordantProblemError do
-        TestHelper.create(vrp)
-      end
-
-      vrp = VRP.basic
-      vrp[:points] << vrp[:points].first
-      assert_raises OptimizerWrapper::DiscordantProblemError do
-        TestHelper.create(vrp)
-      end
-
-      vrp = VRP.basic
-      vrp[:rests] = Array.new(2){ |_rest| { id: 'same_id', timewindows: [{ start: 1, end: 2 }], duration: 1 } }
-      assert_raises OptimizerWrapper::DiscordantProblemError do
-        TestHelper.create(vrp)
-      end
-
-      assert TestHelper.create(VRP.pud) # this should not produce any error
-      vrp = VRP.pud
-      vrp[:shipments] << vrp[:shipments].first
-      assert_raises OptimizerWrapper::DiscordantProblemError do
-        TestHelper.create(vrp)
-      end
-
-      vrp = VRP.pud
-      vrp[:vehicles].first[:capacities] = [{ unit_id: 'unit_0', value: 10 }]
-      vrp[:shipments].first[:quantities] = [{ unit_id: 'unit_0', value: -5 }]
-      vrp[:units] << vrp[:units].first
-      assert_raises OptimizerWrapper::DiscordantProblemError do
-        TestHelper.create(vrp)
-      end
-
-      vrp = VRP.pud
-      vrp[:zones] = Array.new(2){ |_zone| { id: 'same_zone', polygon: { type: 'Polygon', coordinates: [[[0.5, 48.5], [1.5, 48.5]]] }} }
-      assert_raises OptimizerWrapper::DiscordantProblemError do
-        TestHelper.create(vrp)
-      end
-
-      vrp = VRP.pud
-      vrp[:subtours] = Array.new(2){ |_tour| { id: 'same_tour', time_bouds: 180 } }
-      assert_raises OptimizerWrapper::DiscordantProblemError do
-        TestHelper.create(vrp)
-      end
+      vrp_base = VRP.basic
+      # add missing fields so that they can be duplicated for the test
+      vrp_base[:rests] = [{ id: 'rest', timewindows: [{ start: 1, end: 2 }], duration: 1 }]
+      vrp_base[:shipments] = [{
+        id: 'shipment',
+        pickup: { point_id: 'point_3', duration: 3 },
+        delivery: { point_id: 'point_2', duration: 3 },
+        quantities: [{ unit_id: 'unit', value: 3 }]
+      }]
+      vrp_base[:zones] = [{ id: 'zone', polygon: { type: 'Polygon', coordinates: [[[0.5, 48.5], [1.5, 48.5]]] }}]
+      vrp_base[:subtours] = [{ id: 'tour', time_bounds: 180 }]
+      vrp_base[:units] = [{ id: 'unit' }]
+      vrp_base[:vehicles].first[:capacities] = [{ unit_id: 'unit', limit: 10 }]
+      vrp_base = Oj.dump(vrp_base)
+
+      assert TestHelper.create(Oj.load(vrp_base)) # this should not produce any errors
+
+      %i[
+        matrices points rests services shipments units vehicles zones
+      ].each{ |symbol|
+        vrp = Oj.load(vrp_base)
+        vrp[symbol] << vrp[symbol].first
+        assert_raises ActiveHash::IdError do TestHelper.create(vrp) end
+      }
     end
 
     def test_dates_cannot_be_mixed_with_indices
@@ -370,7 +336,7 @@ def test_shipments_should_have_a_pickup_and_a_delivery
                          { type: 'shipment', linked_ids: ['service_2'] },
                          { type: 'shipment', linked_ids: ['service_1', 'service_3'] }]
       error = assert_raises OptimizerWrapper::DiscordantProblemError do
-        Models::Vrp.create(TestHelper.coerce(vrp))
+        TestHelper.create(TestHelper.coerce(vrp))
       end
       assert_equal 'Shipment relations need to have two services -- a pickup and a delivery. ' \
                    'Relations of following services does not have exactly two linked_ids: ' \
@@ -384,12 +350,12 @@ def test_multi_pickup_or_multi_delivery_relations_are_accepted
       # multi-pickup single delivery
       vrp[:relations] = [{ type: 'shipment', linked_ids: ['service_1', 'service_3'] },
                          { type: 'shipment', linked_ids: ['service_2', 'service_3'] }]
-      assert Models::Vrp.create(TestHelper.coerce(vrp)), 'Multi-pickup shipment should not be rejected'
+      assert TestHelper.create(TestHelper.coerce(vrp)), 'Multi-pickup shipment should not be rejected'
 
       # single pickup multi-delivery
       vrp[:relations] = [{ type: 'shipment', linked_ids: ['service_1', 'service_3'] },
                          { type: 'shipment', linked_ids: ['service_1', 'service_2'] }]
-      assert Models::Vrp.create(TestHelper.coerce(vrp)), 'Multi-delivery shipment should not be rejected'
+      assert TestHelper.create(TestHelper.coerce(vrp)), 'Multi-delivery shipment should not be rejected'
     end
 
     def test_ensure_no_skill_matches_with_internal_skills_format
@@ -405,7 +371,7 @@ def test_reject_when_duplicated_ids
       vrp = VRP.toy
       vrp[:services] << vrp[:services].first
 
-      assert_raises OptimizerWrapper::DiscordantProblemError do
+      assert_raises ActiveHash::IdError do
         OptimizerWrapper.wrapper_vrp('demo', { services: { vrp: [:demo] }}, TestHelper.create(vrp), nil)
       end
     end

test/models/vrp_test.rb

@@ -321,7 +321,7 @@ def test_original_skills_and_skills_are_equal_after_create
       vrp[:vehicles].first[:skills] = [['skill_to_output']]
       vrp[:services].first[:skills] = ['skill_to_output']
 
-      created_vrp = Models::Vrp.create(vrp)
+      created_vrp = TestHelper.create(vrp)
       assert_equal 1, created_vrp.services.first.skills.size
       assert_equal created_vrp.services.first.original_skills.size, created_vrp.services.first.skills.size
     end

test/real_cases_periodic_solver_test.rb

@@ -28,6 +28,7 @@ def test_periodic_and_ortools
         unassigned = result[:unassigned].size
 
         vrp.resolution_solver = true
+        Models.delete_all # needed to prevent duplicate ids while calling wrapper_vrp second time
         result = OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, vrp, nil)
         assert unassigned >= result[:unassigned].size, "Increased number of unassigned with ORtools : had #{unassigned}, has #{result[:unassigned].size} now"
       }