This is an automated email from the ASF dual-hosted git repository. chrisdutz pushed a commit to branch develop in repository https://gitbox.apache.org/repos/asf/plc4x.git
commit 751b74634c3603f35cf8d03ef6896d9307bac15a Author: Christofer Dutz <[email protected]> AuthorDate: Sun Aug 16 18:03:20 2026 +0200 feat(plc4j/spi): Warn about connection-string parameters nothing reads The configuration factory ignores any parameter it has no field for, so a misspelt or misplaced one looked accepted while silently leaving the default in place. Writing 'remote-slot=2' instead of 'cotp.remote-slot=2' is the case that keeps costing people an afternoon - the connection comes up on the default slot and nothing says why it is talking to the wrong CPU (GH-2247). DriverBase now compares the supplied parameter names against what the driver, the transport, the audit log and the connection control options declare, and warns about the remainder. Where a known parameter is an obvious near-miss it is suggested: a matching name under a different prefix wins outright, since a missing prefix is the mistake that actually happens, and a short edit distance catches ordinary typos. Nothing close enough means no suggestion rather than an invented one. WARN Connection string parameter 'remote-slot' is not known to driver 's7' and is ignored - did you mean 'cotp.remote-slot'? mistake that actually happens, and a short edit distance catches ordinary typos. Nothing close enough means no suggestion rather than an invented one. WARN Connection string parameter 'remote-slot' is not known to driver 's7' and is ignored - did you mean 'cotp.remote-slot'? The full list of accepted parameters is logged at DEBUG, where someone debugging can ask for it, rather than in the warning. This is deliberately a warning and not an exception: a driver may read further prefixed configurations this code cannot see, and failing the someone debugging can ask for it, rather than in the warning. This is deliberately a warning and not an exception: a driver may read further prefixed configurations this code cannot see, and failing the connection over a false positive would break working setups. To check message - none of them trips it. ConfigurationFactory.parameterNames exposes the existing query parser so the check reads the connection string the same way the code that consumes it does. --- .../java/spi/config/ConfigurationFactory.java | 12 ++ .../apache/plc4x/java/spi/drivers/DriverBase.java | 162 +++++++++++++++++ .../drivers/DriverBaseUnknownParameterTest.java | 196 +++++++++++++++++++++ 3 files changed, 370 insertions(+) diff --git a/plc4j/spi/config/src/main/java/org/apache/plc4x/java/spi/config/ConfigurationFactory.java b/plc4j/spi/config/src/main/java/org/apache/plc4x/java/spi/config/ConfigurationFactory.java index 06226daaa7..d0e267593a 100644 --- a/plc4j/spi/config/src/main/java/org/apache/plc4x/java/spi/config/ConfigurationFactory.java +++ b/plc4j/spi/config/src/main/java/org/apache/plc4x/java/spi/config/ConfigurationFactory.java @@ -301,6 +301,18 @@ public class ConfigurationFactory { return Enum.valueOf((Class<Enum>) field.getType(), valueString); } + /** + * The parameter names a connection string carries, in the order they appear. Exposed so callers + * can compare what was supplied against what the configurations actually declare - the factory + * silently ignores anything it has no field for, which makes a typo look like it was accepted. + * + * @param paramString the parameter part of a connection string (may be null or blank). + * @return the parameter names, never null. + */ + public static Set<String> parameterNames(String paramString) { + return splitQuery(paramString).keySet(); + } + /** * <a href="https://stackoverflow.com/questions/13592236/parse-a-uri-string-into-name-value-collection/13592567#13592567";>...</a> */ diff --git a/plc4j/spi/drivers/src/main/java/org/apache/plc4x/java/spi/drivers/DriverBase.java b/plc4j/spi/drivers/src/main/java/org/apache/plc4x/java/spi/drivers/DriverBase.java index b9d21e2598..5c3039a3c8 100644 --- a/plc4j/spi/drivers/src/main/java/org/apache/plc4x/java/spi/drivers/DriverBase.java +++ b/plc4j/spi/drivers/src/main/java/org/apache/plc4x/java/spi/drivers/DriverBase.java @@ -29,6 +29,7 @@ import org.apache.plc4x.java.api.metadata.PlcDriverMetadata; import org.apache.plc4x.java.api.types.OptionType; import org.apache.plc4x.java.spi.config.Configuration; import org.apache.plc4x.java.spi.config.ConfigurationFactory; +import org.apache.plc4x.java.spi.config.annotations.ComplexConfigurationParameter; import org.apache.plc4x.java.spi.config.annotations.ConfigurationParameter; import org.apache.plc4x.java.spi.config.annotations.Description; import org.apache.plc4x.java.spi.config.annotations.Required; @@ -68,6 +69,8 @@ import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; +import static java.util.stream.Collectors.toList; + /** * Abstract base class for PLC4X drivers. * <p> @@ -214,6 +217,11 @@ public abstract class DriverBase implements PlcDriver { // Create and initialize the audit log. AuditLogConfiguration auditLogConfiguration = configurationFactory.createPrefixedConfiguration( AuditLogConfiguration.class, "log", paramString); + + // Point out parameters nothing will ever read. The configuration factory ignores what it has + // no field for, so a misspelt or misplaced parameter looks accepted and silently leaves the + // default in place - 'remote-slot=2' instead of 'cotp.remote-slot=2' being the classic case. + warnAboutUnknownParameters(paramString, transportCode, transportConfigType); this.auditLog = AuditLog.builder() .withSource(getProtocolCode()) .withConfiguration(auditLogConfiguration) @@ -309,6 +317,160 @@ public abstract class DriverBase implements PlcDriver { }; } + /** + * Logs a warning for every parameter in the connection string that none of the configurations + * involved declares, listing what was expected. + * <p> + * This is deliberately a warning and not an exception: the driver, the transport, the audit log + * and the connection control options are the configurations this class knows about, but a driver + * is free to read further prefixed configurations of its own, and those parameters would look + * unknown from here. Failing the connection over that would break working setups; saying + * something turns a silent misconfiguration into a visible one. + */ + private void warnAboutUnknownParameters(String paramString, String transportCode, + Class<? extends TransportConfiguration> transportConfigType) { + List<String> unknown = findUnknownParameters( + paramString, getConfigurationClass(), transportConfigType, transportCode); + if (unknown.isEmpty()) { + return; + } + Set<String> known = knownParameterNames(getConfigurationClass(), transportConfigType, transportCode); + for (String name : unknown) { + String suggestion = suggestionFor(name, known); + if (suggestion == null) { + log.warn("Connection string parameter '{}' is not known to driver '{}' and is ignored.", + name, getProtocolCode()); + } else { + log.warn("Connection string parameter '{}' is not known to driver '{}' and is ignored - " + + "did you mean '{}'?", name, getProtocolCode(), suggestion); + } + } + if (log.isDebugEnabled()) { + List<String> sorted = new ArrayList<>(known); + Collections.sort(sorted); + log.debug("Parameters driver '{}' accepts over transport '{}': {}", + getProtocolCode(), transportCode, sorted); + } + } + + /** + * The known parameter the given unknown one was most likely meant to be, or {@code null} when + * nothing is close enough to be worth suggesting. + * <p> + * A missing or wrong prefix is the mistake that actually happens - {@code remote-slot} for + * {@code cotp.remote-slot} - so a name that matches some known parameter's last segment wins + * outright. Otherwise a short edit distance catches ordinary typos. + */ + static String suggestionFor(String unknown, Set<String> known) { + String unknownLeaf = leafOf(unknown); + List<String> byLeaf = known.stream() + .filter(candidate -> !candidate.equals(unknown) && leafOf(candidate).equals(unknownLeaf)) + .sorted() + .collect(toList()); + if (!byLeaf.isEmpty()) { + return byLeaf.get(0); + } + + // Allow roughly one edit per four characters, so short names don't match everything. + int budget = Math.min(3, Math.max(1, unknown.length() / 4)); + String best = null; + int bestDistance = Integer.MAX_VALUE; + for (String candidate : known) { + int distance = editDistance(unknown, candidate); + if (distance <= budget && ((best == null) || (distance < bestDistance) + || ((distance == bestDistance) && (candidate.compareTo(best) < 0)))) { + best = candidate; + bestDistance = distance; + } + } + return best; + } + + /** The part after the last dot - the parameter name without any prefix. */ + private static String leafOf(String name) { + int lastDot = name.lastIndexOf('.'); + return (lastDot < 0) ? name : name.substring(lastDot + 1); + } + + /** + * Levenshtein distance. Hand-rolled to keep the SPI free of another dependency; the strings + * involved are parameter names, so the quadratic cost is irrelevant. + */ + private static int editDistance(String left, String right) { + int[] previous = new int[right.length() + 1]; + int[] current = new int[right.length() + 1]; + for (int j = 0; j <= right.length(); j++) { + previous[j] = j; + } + for (int i = 1; i <= left.length(); i++) { + current[0] = i; + for (int j = 1; j <= right.length(); j++) { + int substitution = previous[j - 1] + (left.charAt(i - 1) == right.charAt(j - 1) ? 0 : 1); + current[j] = Math.min(substitution, Math.min(previous[j] + 1, current[j - 1] + 1)); + } + int[] swap = previous; + previous = current; + current = swap; + } + return previous[right.length()]; + } + + /** + * The parameters in {@code paramString} that none of the configurations involved declares, in the + * order they were supplied. Package-private so it can be tested without a registered transport. + */ + static List<String> findUnknownParameters(String paramString, Class<?> driverConfigClass, + Class<?> transportConfigClass, String transportCode) { + Set<String> supplied = ConfigurationFactory.parameterNames(paramString); + if (supplied.isEmpty()) { + return Collections.emptyList(); + } + Set<String> known = knownParameterNames(driverConfigClass, transportConfigClass, transportCode); + return supplied.stream().filter(name -> !known.contains(name)).collect(toList()); + } + + private static Set<String> knownParameterNames(Class<?> driverConfigClass, Class<?> transportConfigClass, + String transportCode) { + Set<String> known = new HashSet<>(); + collectParameterNames(driverConfigClass, "", known); + collectParameterNames(transportConfigClass, transportCode + ".", known); + collectParameterNames(AuditLogConfiguration.class, "log.", known); + collectParameterNames(ConnectionControlConfiguration.class, "", known); + return known; + } + + /** + * Adds every parameter name the given configuration class declares to {@code names}, prefixed + * with {@code prefix}. Complex parameters contribute their nested names under their own prefix, + * so an OPC UA {@code encoding.*} parameter is recognised rather than reported as unknown. + */ + private static void collectParameterNames(Class<?> configurationClass, String prefix, Set<String> names) { + if (configurationClass == null) { + return; + } + Class<?> current = configurationClass; + while (current != null && current != Object.class) { + for (Field field : current.getDeclaredFields()) { + if (Modifier.isStatic(field.getModifiers())) { + continue; + } + ConfigurationParameter parameterAnnotation = field.getAnnotation(ConfigurationParameter.class); + if (parameterAnnotation != null && !parameterAnnotation.value().isEmpty()) { + names.add(prefix + parameterAnnotation.value()); + continue; + } + ComplexConfigurationParameter complexAnnotation = + field.getAnnotation(ComplexConfigurationParameter.class); + if (complexAnnotation != null) { + String nestedPrefix = complexAnnotation.prefix().isEmpty() + ? prefix : prefix + complexAnnotation.prefix() + "."; + collectParameterNames(field.getType(), nestedPrefix, names); + } + } + current = current.getSuperclass(); + } + } + /** * Walks the full class hierarchy of the given configuration class and returns one * {@link Option} for every field annotated with {@link ConfigurationParameter}. diff --git a/plc4j/spi/drivers/src/test/java/org/apache/plc4x/java/spi/drivers/DriverBaseUnknownParameterTest.java b/plc4j/spi/drivers/src/test/java/org/apache/plc4x/java/spi/drivers/DriverBaseUnknownParameterTest.java new file mode 100644 index 0000000000..684e0a76ba --- /dev/null +++ b/plc4j/spi/drivers/src/test/java/org/apache/plc4x/java/spi/drivers/DriverBaseUnknownParameterTest.java @@ -0,0 +1,196 @@ +/* + * 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 + * + * https://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.plc4x.java.spi.drivers; + +import org.apache.plc4x.java.spi.config.Configuration; +import org.apache.plc4x.java.spi.config.annotations.ComplexConfigurationParameter; +import org.apache.plc4x.java.spi.config.annotations.ConfigurationParameter; +import org.apache.plc4x.java.spi.transports.api.config.TransportConfiguration; +import org.junit.jupiter.api.Test; + +import java.util.List; +import java.util.Set; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The configuration factory ignores any parameter it has no field for, so a misspelt or misplaced + * parameter used to look accepted while silently leaving the default in place - writing + * {@code remote-slot=2} instead of {@code cotp.remote-slot=2} being the case that keeps costing + * people an afternoon (GH-2247). These parameters are now reported. + */ +class DriverBaseUnknownParameterTest { + + private static List<String> unknownIn(String paramString) { + return DriverBase.findUnknownParameters( + paramString, DriverConfig.class, TransportConfig.class, "cotp"); + } + + @Test + void acceptsParametersTheDriverDeclares() { + assertEquals(List.of(), unknownIn("controller-type=S7_300&read-timeout=5000")); + } + + @Test + void acceptsTransportParametersUnderTheirPrefix() { + assertEquals(List.of(), unknownIn("cotp.remote-slot=2&cotp.local-tsap=512")); + } + + /** + * The case from the issue: a transport parameter written without its prefix reaches nothing. + */ + @Test + void reportsATransportParameterWithoutItsPrefix() { + assertEquals(List.of("remote-slot"), unknownIn("controller-type=S7_200&remote-slot=2")); + } + + @Test + void reportsAMisspeltParameter() { + assertEquals(List.of("controler-type"), unknownIn("controler-type=S7_300")); + } + + @Test + void reportsSeveralInTheOrderTheyWereSupplied() { + assertEquals(List.of("transport-protocol", "remote-rack"), + unknownIn("transport-protocol=pniop&remote-rack=0&controller-type=S7_200")); + } + + /** + * Parameters inherited from a superclass count - transport configurations are usually a + * protocol-specific subclass of a shared base. + */ + @Test + void acceptsInheritedTransportParameters() { + assertEquals(List.of(), unknownIn("cotp.remote-tsap=512")); + } + + /** + * A complex parameter contributes its nested names under its own prefix. Warning about these + * would be a false positive, which is the failure mode that would make the whole check a + * nuisance - OPC UA has exactly this shape with its {@code encoding.*} options. + */ + @Test + void acceptsNestedComplexParameters() { + assertEquals(List.of(), unknownIn("encoding.charset=utf-8")); + } + + /** + * The options the SPI itself consumes are not the driver's, but they are still legitimate. + */ + @Test + void acceptsSpiOwnedParameters() { + assertEquals(List.of(), unknownIn("allow-unsupported-transport=true")); + } + + @Test + void saysNothingAboutAnEmptyParameterString() { + assertEquals(List.of(), unknownIn(null)); + assertEquals(List.of(), unknownIn("")); + } + + /** + * A driver that declares no configuration at all must not turn every parameter into a warning + * beyond the ones that are genuinely unknown. + */ + @Test + void handlesADriverWithoutAConfigurationClass() { + List<String> unknown = DriverBase.findUnknownParameters( + "allow-unsupported-transport=true&nonsense=1", null, TransportConfig.class, "cotp"); + assertEquals(List.of("nonsense"), unknown); + } + + @Test + void keepsTheValueOutOfTheReportedName() { + assertTrue(unknownIn("nonsense=secret-value").contains("nonsense"), + "the name is reported, not the value"); + assertEquals(List.of("nonsense"), unknownIn("nonsense=secret-value")); + } + + private static final Set<String> KNOWN = Set.of( + "controller-type", "read-timeout", "allow-unsupported-transport", + "cotp.remote-slot", "cotp.remote-rack", "cotp.local-tsap", "log.audit-log-file"); + + /** + * The mistake that actually happens: the right name, without its transport prefix. + */ + @Test + void suggestsThePrefixedNameForAnUnprefixedOne() { + assertEquals("cotp.remote-slot", DriverBase.suggestionFor("remote-slot", KNOWN)); + } + + @Test + void suggestsTheRightNameForATypo() { + assertEquals("controller-type", DriverBase.suggestionFor("controler-type", KNOWN)); + } + + /** + * Nothing is close to this, and inventing a suggestion would be worse than none. + */ + @Test + void suggestsNothingWhenNothingIsClose() { + assertNull(DriverBase.suggestionFor("transport-protocol", KNOWN)); + } + + /** + * A short name must not match half the parameter list just because everything is within a + * couple of edits of it. + */ + @Test + void doesNotStretchForShortNames() { + assertNull(DriverBase.suggestionFor("x", KNOWN)); + } + + @Test + void suggestsDeterministicallyWhenSeveralMatchEquallyWell() { + Set<String> ambiguous = Set.of("cotp.remote-slot", "tcp.remote-slot"); + assertEquals("cotp.remote-slot", DriverBase.suggestionFor("remote-slot", ambiguous)); + assertEquals("cotp.remote-slot", DriverBase.suggestionFor("remote-slot", ambiguous)); + } + + public static class EncodingConfig implements Configuration { + @ConfigurationParameter("charset") + protected String charset = "utf-8"; + } + + public static class DriverConfig implements Configuration { + @ConfigurationParameter("controller-type") + protected String controllerType = "ANY"; + + @ConfigurationParameter("read-timeout") + protected int readTimeout = 10000; + + @ComplexConfigurationParameter(prefix = "encoding", defaultOverrides = {}, requiredOverrides = {}) + protected EncodingConfig encoding; + } + + public static class BaseTransportConfig implements TransportConfiguration { + @ConfigurationParameter("remote-tsap") + protected int remoteTsap; + } + + public static class TransportConfig extends BaseTransportConfig { + @ConfigurationParameter("remote-slot") + protected int remoteSlot; + + @ConfigurationParameter("local-tsap") + protected int localTsap; + } +}
