Fix combine service sometimes selecting additional incorrect target f…

…rames

In cases where a valid target frame exists,
but there are also other frames otherwise valid but with different day type.
Eg. combining Monday-Friday but there's also Saturday for same validity time.

Mostly copypasted query parts from the replace method,
to ensure that target frame is filtered correctly.

src/main/kotlin/fi/hsl/jore4/timetables/repository/VehicleScheduleFrameRepository.kt

@@ -95,23 +95,69 @@ class VehicleScheduleFrameRepository(private val dsl: DSLContext, config: Defaul
         stagingVehicleScheduleFrame: VehicleScheduleFrame,
         targetPriority: TimetablesPriority
     ): List<VehicleScheduleFrame> {
+        val stagingVehicleScheduleFrameId = stagingVehicleScheduleFrame.vehicleScheduleFrameId
+        val getOverlappingSchedules = GetOverlappingSchedules.GET_OVERLAPPING_SCHEDULES
+        val overlappingSchedulesCTE = DSL.name("overlapping_schedules")
+
+        val stagingFrameIdName: Name = DSL.name("stagingVehicleScheduleFrameId")
+        val targetFrameIdName: Name = DSL.name("targetVehicleScheduleFrameId")
+        val stagingVehicleScheduleFrameIdField = DSL.field(stagingFrameIdName, UUID::class.java)
+        val targetVehicleScheduleFrameIdField =
+            DSL.field(targetFrameIdName, UUID::class.java)
+
+        val stagingFrame = VEHICLE_SCHEDULE_FRAME.`as`("staging")
+        val targetFrame = VEHICLE_SCHEDULE_FRAME.`as`("target")
+
+        val framesOverlappedByStagingQuery = dsl
+            .with(
+                overlappingSchedulesCTE
+                    .fields(
+                        stagingFrameIdName,
+                        targetFrameIdName
+                    )
+                    .`as`(
+                        dsl
+                            .select(
+                                getOverlappingSchedules.CURRENT_VEHICLE_SCHEDULE_FRAME_ID.`as`(
+                                    stagingVehicleScheduleFrameIdField.name
+                                ),
+                                getOverlappingSchedules.OTHER_VEHICLE_SCHEDULE_FRAME_ID.`as`(
+                                    targetVehicleScheduleFrameIdField.name
+                                )
+                            )
+                            .from(
+                                getOverlappingSchedules(
+                                    arrayOf(stagingVehicleScheduleFrameId),
+                                    arrayOf(),
+                                    true
+                                )
+                            )
+                    )
+            )
+            .select(
+                stagingVehicleScheduleFrameIdField,
+                targetVehicleScheduleFrameIdField
+            )
+            .from(targetFrame)
+            .join(overlappingSchedulesCTE)
+            .on(targetFrame.VEHICLE_SCHEDULE_FRAME_ID.eq(targetVehicleScheduleFrameIdField))
+            .join(stagingFrame)
+            .on(stagingFrame.VEHICLE_SCHEDULE_FRAME_ID.eq(stagingVehicleScheduleFrameIdField))
+            // The overlapping schedules query returns other frames as well, filter out unwanted ones.
+            .where(stagingFrame.VEHICLE_SCHEDULE_FRAME_ID.eq(stagingVehicleScheduleFrameId))
+            .and(targetFrame.PRIORITY.eq(targetPriority.value))
+
         return dsl
             .select()
-            .from(VEHICLE_SCHEDULE_FRAME)
-            .where(VEHICLE_SCHEDULE_FRAME.PRIORITY.eq(targetPriority.value))
+            // Returns a row for each day type id.
+            // We are not interested in those here, just the overlapping frame ids.
+            .distinctOn(targetVehicleScheduleFrameIdField)
+            .from(framesOverlappedByStagingQuery)
+            .join(VEHICLE_SCHEDULE_FRAME)
+            .on(VEHICLE_SCHEDULE_FRAME.VEHICLE_SCHEDULE_FRAME_ID.eq(targetVehicleScheduleFrameIdField))
             // The validity times must be exactly the same.
-            .and(VEHICLE_SCHEDULE_FRAME.VALIDITY_START.eq(stagingVehicleScheduleFrame.validityStart))
+            .where(VEHICLE_SCHEDULE_FRAME.VALIDITY_START.eq(stagingVehicleScheduleFrame.validityStart))
             .and(VEHICLE_SCHEDULE_FRAME.VALIDITY_END.eq(stagingVehicleScheduleFrame.validityEnd))
-            // And should have some routes (journey patterns) in common.
-            .andExists(
-                DSL.selectOne().from(
-                    getOverlappingSchedules(
-                        arrayOf(stagingVehicleScheduleFrame.vehicleScheduleFrameId),
-                        arrayOf(),
-                        true
-                    )
-                )
-            )
             .fetchInto(VehicleScheduleFrame::class.java)
     }
 }

src/test/kotlin/fi/hsl/jore4/timetables/service/CombineTimetablesServiceTest.kt

@@ -102,6 +102,37 @@ class CombineTimetablesServiceTest @Autowired constructor(
             assertNull(vehicleScheduleFrameRepository.fetchOneByVehicleScheduleFrameId(stagingFrameId))
         }
 
+        @Test
+        fun `combines the timetable when in addition to actual target there also exists other frames with identical validity but different days of week`() {
+            val testData = TimetablesDataset.createFromResource("datasets/combine.json")
+            testData.getNested("_vehicle_schedule_frames.staging._vehicle_services.monFri")["day_type_id"] =
+                "MONDAY_FRIDAY"
+            testData.getNested("_vehicle_schedule_frames.target._vehicle_services.monFri")["day_type_id"] =
+                "WEDNESDAY"
+
+            // Add another frame with identical validity range (should match combined) but different days of week (-> should not match).
+            val saturdayFrameId = UUID.fromString("ccc9f432-f1f1-4baa-935b-39590a8d9380")
+            val saturdayTarget = testData.getNested("_vehicle_schedule_frames.target").deepClone()
+                .setFrameProperties(
+                    mutableMapOf(
+                        "vehicle_schedule_frame_id" to saturdayFrameId,
+                        "name" to "2023 tammikuu lauantai"
+                    )
+                )
+            saturdayTarget.getNested("_vehicle_services.monFri")["day_type_id"] = "SATURDAY"
+            testData.getNested("_vehicle_schedule_frames")["saturdayTarget"] = saturdayTarget
+
+            timetablesDataInserterRunner.truncateAndInsertDataset(testData.toJSONString())
+            val stagingFrameId = UUID.fromString("aa0e95c6-34d1-4d09-8546-3789b04ea494")
+
+            val result = combineTimetablesService.combineTimetables(
+                listOf(stagingFrameId),
+                TimetablesPriority.STANDARD
+            )
+            assertEquals(listOf(UUID.fromString("bb2abc90-8e91-4b0b-a7e4-751a04e81ba3")), result)
+            assertNull(vehicleScheduleFrameRepository.fetchOneByVehicleScheduleFrameId(stagingFrameId))
+        }
+
         @Test
         fun `fails when called with invalid target priority = staging`() {
             val testData = TimetablesDataset.createFromResource("datasets/combine.json")
@@ -384,17 +415,6 @@ class CombineTimetablesServiceTest @Autowired constructor(
             )
         }
 
-        private fun MutableMap<String, Any?>.setFrameProperties(
-            patch: MutableMap<String, Any?>,
-            serviceId: UUID = UUID.randomUUID()
-        ): MutableMap<String, Any?> {
-            this.putAll(patch)
-            // Service id is hard coded in the dataset (for asserts),
-            // need to reset since these will be cloned.
-            this.getNested("_vehicle_services.monFri")["vehicle_service_id"] = serviceId
-            return this
-        }
-
         @Test
         fun `combine each staging timetables and returns target vehicle schedule frame ids`() {
             timetablesDataInserterRunner.truncateAndInsertDataset(testData.toJSONString())
@@ -435,4 +455,15 @@ class CombineTimetablesServiceTest @Autowired constructor(
             assertEquals(initialServices, servicesAfter)
         }
     }
+
+    private fun MutableMap<String, Any?>.setFrameProperties(
+        patch: MutableMap<String, Any?>,
+        serviceId: UUID = UUID.randomUUID()
+    ): MutableMap<String, Any?> {
+        this.putAll(patch)
+        // Service id is hard coded in the dataset (for asserts),
+        // need to reset since these will be cloned.
+        this.getNested("_vehicle_services.monFri")["vehicle_service_id"] = serviceId
+        return this
+    }
 }