gnodet-bot commented on code in PR #26802: URL: https://github.com/apache/camel/pull/26802#discussion_r4085073438
########## core/camel-core/src/test/java/org/apache/camel/impl/ErrorRegistrySourceLocationTest.java: ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.impl; + +import java.util.regex.Pattern; + +import org.apache.camel.CamelContext; +import org.apache.camel.ContextTestSupport; +import org.apache.camel.builder.RouteBuilder; +import org.apache.camel.spi.BacklogErrorEventMessage; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The step strings of the error registry say where each step is in the source, so a reader holding the file can go to + * the line instead of mapping a generated node id back to it by hand (CAMEL-24972). + */ +public class ErrorRegistrySourceLocationTest extends ContextTestSupport { + + /** Such as ErrorRegistrySourceLocationTest:57 */ + private static final Pattern LOCATION = Pattern.compile("\\S+:\\d+"); + + @Override + protected CamelContext createCamelContext() throws Exception { + CamelContext context = super.createCamelContext(); + context.getErrorRegistry().setEnabled(true); + context.setMessageHistory(true); + context.setSourceLocationEnabled(true); + return context; + } + + @Test + public void testStepsCarryTheSourceLocation() throws Exception { + getMockEndpoint("mock:dead").expectedMessageCount(1); + template.sendBody("direct:start", "Hello World"); + assertMockEndpointsSatisfied(); + + BacklogErrorEventMessage entry = context.getErrorRegistry().browse().iterator().next(); + String[] steps = entry.getMessageHistory(); + assertNotNull(steps, "Message history should be captured when enabled"); + assertTrue(steps.length > 0, "Message history should have at least one entry"); + for (String step : steps) { + assertTrue(LOCATION.matcher(step).find(), "Step should say where it is in the source: " + step); + } + // the location sits next to the body type, so both facts about a step arrive together + assertTrue(steps[0].contains("bodyType="), steps[0]); + } + + @Test + public void testLocationIsLeftOutWhenSourceLocationIsOff() throws Exception { + context.setSourceLocationEnabled(false); + context.getRouteController().stopRoute("foo"); + context.getRouteController().startRoute("foo"); + + getMockEndpoint("mock:dead").expectedMessageCount(1); + template.sendBody("direct:start", "Hello World"); + assertMockEndpointsSatisfied(); + + BacklogErrorEventMessage entry = context.getErrorRegistry().browse().iterator().next(); + assertNotNull(entry.getMessageHistory()); + // nothing to say, so nothing is said - the step keeps its shape + assertTrue(entry.getMessageHistory()[0].contains("bodyType="), entry.getMessageHistory()[0]); Review Comment: ⚠️ **Weak assertion — does not verify the negative case.** The comment says "nothing to say, so nothing is said", but the here only checks that is present in the step string. It does **not** assert that no source location token appears. If fails to flush previously captured line numbers from the route nodes (line numbers are captured at load time, and whether toggling then restarting fully clears them is not obvious), this test passes vacuously even when the behaviour is wrong. Add the complementary : ```suggestion // nothing to say, so nothing is said - the step keeps its shape assertTrue(entry.getMessageHistory()[0].contains("bodyType="), entry.getMessageHistory()[0]); assertFalse(LOCATION.matcher(entry.getMessageHistory()[0]).find(), "Step should NOT carry source location when source location tracking is off: " + entry.getMessageHistory()[0]); ``` Also add at line 64. ########## core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/DefaultErrorRegistry.java: ########## @@ -300,6 +300,11 @@ private static String[] captureMessageHistory(Exchange exchange) { String nodeId = mh.getNode() != null ? mh.getNode().getId() : null; long elapsed = mh.getElapsed(); String step = mh.getRouteId() + "[" + nodeId + "]"; + // where the step is in the source, so the reader can go to the line (CAMEL-24972) Review Comment: 📝 ** Javadoc does not match the new format.** line 129 still documents the step format as: > After CAMEL-24844 (body type/size) and this PR (source location), the actual format is now: > where each bracketed part is conditional. The PR description explicitly notes that AI agents parse these strings ( returns them verbatim). A consumer relying on the documented format to tokenise steps will break. Please update the Javadoc to document the current format, including which parts are conditional and under what conditions they are present. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
