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 cda0656a513c3ddc8afe660b9921234d7d727b7d Author: Christofer Dutz <[email protected]> AuthorDate: Fri Aug 14 11:03:53 2026 +0200 fix(plc4j/s7): Addressed an observed asymmetry in tag-handling with S7 tags fixes: #2388 --- .../plc4x/java/s7/tag/S7StringFixedLengthTag.java | 6 +-- .../plc4x/java/s7/tag/S7StringVarLengthTag.java | 6 +-- .../java/org/apache/plc4x/java/s7/tag/S7Tag.java | 14 +++++- .../apache/plc4x/java/s7/tag/S7StringTagTest.java | 55 ++++++++++++++++++++++ 4 files changed, 74 insertions(+), 7 deletions(-) diff --git a/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringFixedLengthTag.java b/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringFixedLengthTag.java index 713bd05825..845dd85ccc 100644 --- a/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringFixedLengthTag.java +++ b/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringFixedLengthTag.java @@ -145,8 +145,8 @@ public class S7StringFixedLengthTag extends S7Tag { return new S7StringFixedLengthTag(dataType, memoryArea, blockNumber, byteOffset, bitOffset, numElements, stringLength); } - - return null; - } + + throw new PlcInvalidTagException("Unable to parse address: " + address); + } } diff --git a/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringVarLengthTag.java b/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringVarLengthTag.java index eced0465f6..fa9b52aa3d 100644 --- a/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringVarLengthTag.java +++ b/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringVarLengthTag.java @@ -121,8 +121,8 @@ public class S7StringVarLengthTag extends S7Tag { return new S7StringVarLengthTag(dataType, memoryArea, blockNumber, byteOffset, bitOffset, numElements); } - - return null; - } + + throw new PlcInvalidTagException("Unable to parse address: " + address); + } } diff --git a/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7Tag.java b/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7Tag.java index 5ed2e5072b..e21c8fb652 100644 --- a/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7Tag.java +++ b/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7Tag.java @@ -156,7 +156,9 @@ public class S7Tag implements PlcTag, Serializable { } public static boolean matches(String tagString) { - return DATA_BLOCK_ADDRESS_PATTERN.matcher(tagString).matches() || + return S7StringFixedLengthTag.matches(tagString) || + S7StringVarLengthTag.matches(tagString) || + DATA_BLOCK_ADDRESS_PATTERN.matcher(tagString).matches() || DATA_BLOCK_SHORT_PATTERN.matcher(tagString).matches() || PLC_PROXY_ADDRESS_PATTERN.matcher(tagString).matches() || ADDRESS_PATTERN.matcher(tagString).matches(); @@ -164,6 +166,16 @@ public class S7Tag implements PlcTag, Serializable { public static S7Tag of(String tagString) { Matcher matcher; + // STRING/WSTRING addresses carry their length - "%DB69:68:STRING(20)" - which none of the + // patterns below accept. Delegate those to the dedicated subtypes, the same way + // S7PlcTagHandler.parseTag() does, so both entry points parse the same address space and + // the declared length isn't lost (it decides the string layout when reading/writing). + if (S7StringFixedLengthTag.matches(tagString)) { + return S7StringFixedLengthTag.of(tagString); + } + if (S7StringVarLengthTag.matches(tagString)) { + return S7StringVarLengthTag.of(tagString); + } if ((matcher = DATA_BLOCK_ADDRESS_PATTERN.matcher(tagString)).matches()) { String dataTypeName = matcher.group(DATA_TYPE); if("RAW_BYTE_ARRAY".equals(dataTypeName)) { diff --git a/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/tag/S7StringTagTest.java b/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/tag/S7StringTagTest.java index 5c61224cb1..d4788e3a0e 100644 --- a/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/tag/S7StringTagTest.java +++ b/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/tag/S7StringTagTest.java @@ -110,6 +110,61 @@ class S7StringTagTest { assertEquals(TransportSize.STRING, tag.getDataType()); } + /** + * S7Tag.of() has to accept everything S7PlcTagHandler.parseTag() accepts, STRING lengths + * included - see GH-2388. Otherwise an address that works via addTagAddress(...) blows up + * when the very same string is handed to S7Tag.of(...). + */ + @Test + void s7TagOfParsesFixedLengthStrings() { + S7Tag longForm = S7Tag.of("%DB1.DB0:STRING(80)"); + assertInstanceOf(S7StringFixedLengthTag.class, longForm); + assertEquals(80, ((S7StringFixedLengthTag) longForm).getStringLength()); + + // the reporter's address + S7Tag shortForm = S7Tag.of("%DB69:68:STRING(20)"); + assertInstanceOf(S7StringFixedLengthTag.class, shortForm); + assertEquals(20, ((S7StringFixedLengthTag) shortForm).getStringLength()); + assertEquals(69, shortForm.getBlockNumber()); + assertEquals(68, shortForm.getByteOffset()); + + S7Tag withCount = S7Tag.of("%DB1.DB0:WSTRING(40)[3]"); + assertInstanceOf(S7StringFixedLengthTag.class, withCount); + assertEquals(40, ((S7StringFixedLengthTag) withCount).getStringLength()); + assertEquals(3, withCount.getNumberOfElements()); + } + + @Test + void s7TagMatchesAcceptsStringLengths() { + assertTrue(S7Tag.matches("%DB69:68:STRING(20)")); + assertTrue(S7Tag.matches("%DB1.DB0:WSTRING(40)[3]")); + assertFalse(S7Tag.matches("not-a-tag")); + } + + /** + * Both entry points must return equal tags for the same address. + */ + @Test + void s7TagOfAgreesWithTagHandler() { + S7PlcTagHandler handler = new S7PlcTagHandler(); + for (String address : new String[]{ + "%DB69:68:STRING(20)", + "%DB1.DB0:STRING(80)", + "%DB1.DB0:STRING(40)[3]", + "%DB1.DB0:STRING", + "%DB1:0:STRING", + "%MW0:INT", + "%DB1.DBX0.0:BOOL"}) { + assertEquals(handler.parseTag(address), S7Tag.of(address), address); + } + } + + @Test + void s7TagOfStillRejectsGarbage() { + assertThrows(org.apache.plc4x.java.api.exceptions.PlcInvalidTagException.class, + () -> S7Tag.of("not-a-tag")); + } + @Test void tagHandlerRoutesToCorrectTagClass() { S7PlcTagHandler handler = new S7PlcTagHandler();
