From 955d5f44debdaf6f20bd780916c97d0cbcb75157 Mon Sep 17 00:00:00 2001 From: Julien Kronegg Date: Wed, 8 Oct 2025 22:10:47 +0200 Subject: [PATCH 1/2] feat: avoid unnecessary small things and corrected pom --- java/pom.xml | 1 + .../java/io/cucumber/gherkin/AstNode.java | 10 +++- .../gherkin/GherkinDocumentBuilder.java | 56 ++++++++++++------- .../gherkin/GherkinDocumentBuilderTest.java | 4 +- 4 files changed, 49 insertions(+), 22 deletions(-) diff --git a/java/pom.xml b/java/pom.xml index 031395101..cbefc6175 100644 --- a/java/pom.xml +++ b/java/pom.xml @@ -113,6 +113,7 @@ org.assertj assertj-core + test diff --git a/java/src/main/java/io/cucumber/gherkin/AstNode.java b/java/src/main/java/io/cucumber/gherkin/AstNode.java index dca8e9ba0..7875f732f 100644 --- a/java/src/main/java/io/cucumber/gherkin/AstNode.java +++ b/java/src/main/java/io/cucumber/gherkin/AstNode.java @@ -38,6 +38,14 @@ T getSingle(RuleType ruleType, T defaultResult) { return items == null ? defaultResult : items.get(0); } + @SuppressWarnings("unchecked") + T getSingle(RuleType ruleType) { + // if not null, then at least one item is present because + // the list was created in add(), so no need to check isEmpty() + List items = (List) subItems.get(ruleType); + return items == null ? null : items.get(0); + } + @SuppressWarnings("unchecked") List getItems(RuleType ruleType) { List items = (List) subItems.get(ruleType); @@ -48,7 +56,7 @@ List getItems(RuleType ruleType) { } Token getToken(TokenType tokenType) { - return requireNonNull(getSingle(tokenType.ruleType, null)); + return getSingle(tokenType.ruleType); } List getTokens(TokenType tokenType) { diff --git a/java/src/main/java/io/cucumber/gherkin/GherkinDocumentBuilder.java b/java/src/main/java/io/cucumber/gherkin/GherkinDocumentBuilder.java index a9ce1aaba..2c5168998 100644 --- a/java/src/main/java/io/cucumber/gherkin/GherkinDocumentBuilder.java +++ b/java/src/main/java/io/cucumber/gherkin/GherkinDocumentBuilder.java @@ -83,8 +83,8 @@ private Object getTransformedNode(AstNode node) { stepLine.matchedKeyword, stepLine.keywordType, stepLine.matchedText, - node.getSingle(RuleType.DocString, null), - node.getSingle(RuleType.DataTable, null), + node.getSingle(RuleType.DocString), + node.getSingle(RuleType.DataTable), idGenerator.newId() ); } @@ -116,7 +116,8 @@ private Object getTransformedNode(AstNode node) { ); } case ScenarioDefinition: { - AstNode scenarioNode = node.getSingle(RuleType.Scenario, null); + AstNode scenarioNode = node.getSingle(RuleType.Scenario); + //noinspection ConstantConditions // scenarioNode should never be null because of the grammar (see Parser) Token scenarioLine = scenarioNode.getToken(TokenType.ScenarioLine); return new Scenario( @@ -131,12 +132,22 @@ private Object getTransformedNode(AstNode node) { ); } case ExamplesDefinition: { - AstNode examplesNode = node.getSingle(RuleType.Examples, null); + AstNode examplesNode = node.getSingle(RuleType.Examples); + //noinspection ConstantConditions // examplesNode is never be null because of the grammar (see Parser) Token examplesLine = examplesNode.getToken(TokenType.ExamplesLine); - List rows = examplesNode.getSingle(RuleType.ExamplesTable, null); - TableRow tableHeader = rows != null && !rows.isEmpty() ? rows.get(0) : null; - List tableBody = rows != null && !rows.isEmpty() ? rows.subList(1, rows.size()) : Collections.emptyList(); + List rows = examplesNode.getSingle(RuleType.ExamplesTable); + TableRow tableHeader; + List tableBody; + // rows is null when a Scenario Outline has no Examples table + if (rows != null && !rows.isEmpty()) { + tableHeader = rows.get(0); + tableBody = rows.subList(1, rows.size()); + } else { + tableHeader = null; + tableBody = Collections.emptyList(); + } + //noinspection ConstantConditions // examplesLine is never be null because of the grammar (see Parser) return new Examples( examplesLine.location, getTags(node), @@ -146,7 +157,6 @@ private Object getTransformedNode(AstNode node) { tableHeader, tableBody, idGenerator.newId() - ); } case ExamplesTable: { @@ -162,14 +172,17 @@ private Object getTransformedNode(AstNode node) { return joinMatchedText(lineTokens, toIndex); } case Feature: { - AstNode header = node.getSingle(RuleType.FeatureHeader, new AstNode(RuleType.FeatureHeader)); - if (header == null) return null; + // when we transform a Feature, a FeatureHeader is always present + // and a FeatureLine are always present in the FeatureHeader + // because of the grammar (see Parser) + AstNode header = node.getSingle(RuleType.FeatureHeader); + //noinspection ConstantConditions // header is never be null because of the grammar (see Parser) List tags = getTags(header); Token featureLine = header.getToken(TokenType.FeatureLine); - if (featureLine == null) return null; List children = new ArrayList<>(); - Background background = node.getSingle(RuleType.Background, null); + Background background = node.getSingle(RuleType.Background); + // Background is an optional element of a Feature, so can be null if (background != null) { children.add(new FeatureChild(null, background, null)); } @@ -180,6 +193,7 @@ private Object getTransformedNode(AstNode node) { children.add(new FeatureChild(rule, null, null)); } + //noinspection ConstantConditions // featureLine is never be null because of the grammar (see Parser) return new Feature( featureLine.location, tags, @@ -191,15 +205,15 @@ private Object getTransformedNode(AstNode node) { ); } case Rule: { - AstNode header = node.getSingle(RuleType.RuleHeader, new AstNode(RuleType.RuleHeader)); - if (header == null) return null; + AstNode header = node.getSingle(RuleType.RuleHeader); + //noinspection ConstantConditions // header is never be null because of the grammar (see Parser) Token ruleLine = header.getToken(TokenType.RuleLine); - if (ruleLine == null) return null; List children = new ArrayList<>(); List tags = getTags(header); - Background background = node.getSingle(RuleType.Background, null); + Background background = node.getSingle(RuleType.Background); + // Background is an optional element of a Feature, so can be null if (background != null) { children.add(new RuleChild(background, null)); } @@ -208,6 +222,7 @@ private Object getTransformedNode(AstNode node) { children.add(new RuleChild(null, scenario)); } + //noinspection ConstantConditions // ruleLine is never be null because of the grammar (see Parser) return new Rule( ruleLine.location, tags, @@ -220,7 +235,7 @@ private Object getTransformedNode(AstNode node) { } case GherkinDocument: { - Feature feature = node.getSingle(RuleType.Feature, null); + Feature feature = node.getSingle(RuleType.Feature); return new GherkinDocument(uri, feature, comments); } @@ -293,9 +308,10 @@ private String getDescription(AstNode node) { } private List getTags(AstNode node) { - AstNode tagsNode = node.getSingle(RuleType.Tags, null); - if (tagsNode == null) + AstNode tagsNode = node.getSingle(RuleType.Tags); + if (tagsNode == null) {// tags are optional return Collections.emptyList(); + } List tokens = tagsNode.getTokens(TokenType.TagLine); List tags = new ArrayList<>(); @@ -309,7 +325,7 @@ private List getTags(AstNode node) { @Override public GherkinDocument getResult() { - return currentNode().getSingle(RuleType.GherkinDocument, null); + return currentNode().getSingle(RuleType.GherkinDocument); } } diff --git a/java/src/test/java/io/cucumber/gherkin/GherkinDocumentBuilderTest.java b/java/src/test/java/io/cucumber/gherkin/GherkinDocumentBuilderTest.java index 9053be7be..45fd49c28 100644 --- a/java/src/test/java/io/cucumber/gherkin/GherkinDocumentBuilderTest.java +++ b/java/src/test/java/io/cucumber/gherkin/GherkinDocumentBuilderTest.java @@ -87,7 +87,9 @@ void sets_empty_table_cells() { " |a||b|", "test.feature" ); - TableRow row = doc.getFeature().get().getChildren().get(0).getScenario().get().getSteps().get(0).getDataTable().get().getRows().get(0); + List children = doc.getFeature().get().getChildren(); + assertEquals(1, children.size()); + TableRow row = children.get(0).getScenario().get().getSteps().get(0).getDataTable().get().getRows().get(0); assertEquals("a", row.getCells().get(0).getValue()); assertEquals("", row.getCells().get(1).getValue()); assertEquals("b", row.getCells().get(2).getValue()); From 5b0fee72be64484a36bc4272b910a68625c346f9 Mon Sep 17 00:00:00 2001 From: Julien Kronegg Date: Sat, 11 Oct 2025 00:09:58 +0200 Subject: [PATCH 2/2] fix: added null checks to ensure the parser does not give bad data to the GherkinDocumentBuilder --- CHANGELOG.md | 3 ++ .../java/io/cucumber/gherkin/AstNode.java | 6 +++- .../gherkin/GherkinDocumentBuilder.java | 25 +++++---------- .../java/io/cucumber/gherkin/AstNodeTest.java | 22 +++++++++++++ .../io/cucumber/gherkin/TestDataTest.java | 32 +++++++++++++++++++ 5 files changed, 70 insertions(+), 18 deletions(-) create mode 100644 java/src/test/java/io/cucumber/gherkin/TestDataTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ea7a1d29..295c21959 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ This document is formatted according to the principles of [Keep A CHANGELOG](htt ### Added - [Java] Add OSGi metadata ([#485](https://github.com/cucumber/gherkin/pull/485)) +### Fixed +- [Java] Fixed `AstNode` conditions which never occur in `GherkinDocumentBuilder`. + ## [36.0.0] - 2025-10-09 ### Changed - [.NET, Elixir, Go, JavaScript, Java, Perl, Php, Ruby] Update dependency messages to v30 diff --git a/java/src/main/java/io/cucumber/gherkin/AstNode.java b/java/src/main/java/io/cucumber/gherkin/AstNode.java index 7875f732f..66858e0ea 100644 --- a/java/src/main/java/io/cucumber/gherkin/AstNode.java +++ b/java/src/main/java/io/cucumber/gherkin/AstNode.java @@ -46,6 +46,10 @@ T getSingle(RuleType ruleType) { return items == null ? null : items.get(0); } + T getRequiredSingle(RuleType ruleType) { + return requireNonNull(getSingle(ruleType)); + } + @SuppressWarnings("unchecked") List getItems(RuleType ruleType) { List items = (List) subItems.get(ruleType); @@ -56,7 +60,7 @@ List getItems(RuleType ruleType) { } Token getToken(TokenType tokenType) { - return getSingle(tokenType.ruleType); + return getRequiredSingle(tokenType.ruleType); } List getTokens(TokenType tokenType) { diff --git a/java/src/main/java/io/cucumber/gherkin/GherkinDocumentBuilder.java b/java/src/main/java/io/cucumber/gherkin/GherkinDocumentBuilder.java index 2c5168998..b9f764211 100644 --- a/java/src/main/java/io/cucumber/gherkin/GherkinDocumentBuilder.java +++ b/java/src/main/java/io/cucumber/gherkin/GherkinDocumentBuilder.java @@ -116,8 +116,7 @@ private Object getTransformedNode(AstNode node) { ); } case ScenarioDefinition: { - AstNode scenarioNode = node.getSingle(RuleType.Scenario); - //noinspection ConstantConditions // scenarioNode should never be null because of the grammar (see Parser) + AstNode scenarioNode = node.getRequiredSingle(RuleType.Scenario); Token scenarioLine = scenarioNode.getToken(TokenType.ScenarioLine); return new Scenario( @@ -132,13 +131,12 @@ private Object getTransformedNode(AstNode node) { ); } case ExamplesDefinition: { - AstNode examplesNode = node.getSingle(RuleType.Examples); - //noinspection ConstantConditions // examplesNode is never be null because of the grammar (see Parser) + AstNode examplesNode = node.getRequiredSingle(RuleType.Examples); Token examplesLine = examplesNode.getToken(TokenType.ExamplesLine); + // rows is null when a Scenario Outline has no Examples table List rows = examplesNode.getSingle(RuleType.ExamplesTable); TableRow tableHeader; List tableBody; - // rows is null when a Scenario Outline has no Examples table if (rows != null && !rows.isEmpty()) { tableHeader = rows.get(0); tableBody = rows.subList(1, rows.size()); @@ -147,7 +145,6 @@ private Object getTransformedNode(AstNode node) { tableBody = Collections.emptyList(); } - //noinspection ConstantConditions // examplesLine is never be null because of the grammar (see Parser) return new Examples( examplesLine.location, getTags(node), @@ -172,17 +169,13 @@ private Object getTransformedNode(AstNode node) { return joinMatchedText(lineTokens, toIndex); } case Feature: { - // when we transform a Feature, a FeatureHeader is always present - // and a FeatureLine are always present in the FeatureHeader - // because of the grammar (see Parser) - AstNode header = node.getSingle(RuleType.FeatureHeader); - //noinspection ConstantConditions // header is never be null because of the grammar (see Parser) + AstNode header = node.getRequiredSingle(RuleType.FeatureHeader); List tags = getTags(header); Token featureLine = header.getToken(TokenType.FeatureLine); List children = new ArrayList<>(); - Background background = node.getSingle(RuleType.Background); // Background is an optional element of a Feature, so can be null + Background background = node.getSingle(RuleType.Background); if (background != null) { children.add(new FeatureChild(null, background, null)); } @@ -193,7 +186,6 @@ private Object getTransformedNode(AstNode node) { children.add(new FeatureChild(rule, null, null)); } - //noinspection ConstantConditions // featureLine is never be null because of the grammar (see Parser) return new Feature( featureLine.location, tags, @@ -205,15 +197,14 @@ private Object getTransformedNode(AstNode node) { ); } case Rule: { - AstNode header = node.getSingle(RuleType.RuleHeader); - //noinspection ConstantConditions // header is never be null because of the grammar (see Parser) + AstNode header = node.getRequiredSingle(RuleType.RuleHeader); Token ruleLine = header.getToken(TokenType.RuleLine); List children = new ArrayList<>(); List tags = getTags(header); - Background background = node.getSingle(RuleType.Background); // Background is an optional element of a Feature, so can be null + Background background = node.getSingle(RuleType.Background); if (background != null) { children.add(new RuleChild(background, null)); } @@ -222,7 +213,6 @@ private Object getTransformedNode(AstNode node) { children.add(new RuleChild(null, scenario)); } - //noinspection ConstantConditions // ruleLine is never be null because of the grammar (see Parser) return new Rule( ruleLine.location, tags, @@ -236,6 +226,7 @@ private Object getTransformedNode(AstNode node) { } case GherkinDocument: { Feature feature = node.getSingle(RuleType.Feature); + // feature is null when the file is empty or contains only comments/whitespace, or no Cucumber feature return new GherkinDocument(uri, feature, comments); } diff --git a/java/src/test/java/io/cucumber/gherkin/AstNodeTest.java b/java/src/test/java/io/cucumber/gherkin/AstNodeTest.java index 83845a375..20f628a32 100644 --- a/java/src/test/java/io/cucumber/gherkin/AstNodeTest.java +++ b/java/src/test/java/io/cucumber/gherkin/AstNodeTest.java @@ -43,4 +43,26 @@ void getSingle_returns_first_item() { // Then assertEquals(item1, astNode.getSingle(Parser.RuleType.Step, "defaultValue")); } + + @Test + void getRequiredSingle_throws_exception_when_no_items() { + // Given a node + AstNode astNode = new AstNode(Parser.RuleType.Step); + + // When no subItem is present + + // Then + assertThrows(NullPointerException.class, () -> astNode.getRequiredSingle(Parser.RuleType.Scenario)); + } + + @Test + void getSingle_return_null_when_no_items() { + // Given a node + AstNode astNode = new AstNode(Parser.RuleType.Step); + + // When no subItem is present + + // Then + assertNull(astNode.getSingle(Parser.RuleType.Scenario)); + } } diff --git a/java/src/test/java/io/cucumber/gherkin/TestDataTest.java b/java/src/test/java/io/cucumber/gherkin/TestDataTest.java new file mode 100644 index 000000000..e5f0c844b --- /dev/null +++ b/java/src/test/java/io/cucumber/gherkin/TestDataTest.java @@ -0,0 +1,32 @@ +package io.cucumber.gherkin; + +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.stream.Stream; + +public class TestDataTest { + @Test + void testdata_features_are_parsed_without_NPE() throws IOException { + GherkinParser gherkinParser = GherkinParser.builder() + .idGenerator(new IncrementingIdGenerator()) + .build(); + try (Stream list = Stream.of( + Files.list(Paths.get("../testdata/good/")), + Files.list(Paths.get("../testdata/bad/"))) + .flatMap(s -> s)) { + list + .filter(path -> path.toString().endsWith(".feature")) + .forEach(source -> { + try { + gherkinParser.parse(source); + } catch (IOException e) { + throw new RuntimeException(e); + } + }); + } + } +}