CucumberND table headers might be missing

[uuf6429]

The following line:

Gherkin/src/Behat/Gherkin/Loader/CucumberNDJsonAstLoader.php

Line 160 in e016902

$table[$tableJson['tableHeader']['location']['line']] = array_column($tableJson['tableHeader']['cells'], 'value');

assumes that tableHeader is always set, but according to the spec schema, tableHeader is not required, and therefore optional.

I discovered this while implementing PHPStan fixes, using hand-written schema in PHPStan types DSL:

/**
 * @phpstan-type TExamples THasTags&array{
 *     location: TLocation,
 *     keyword: string,
 *     name: string,
 *     description: string,
 *     tableHeader?: TTableRow,              <-----
 *     tableBody: list<TTableRow>,
 *     id: string,
 * }
 */

What shall we do about this?

1. Make the PHPStan type mandatory (by removing the ?) and generally ignore the code.
2. Make PHPStan ignore that line (@phpstan-ignore-next-line)
3. Skip that line when the header is not set:

   if (isset($tableJson['tableHeader'])) {
       $table[$tableJson['tableHeader']['location']['line']] = array_column($tableJson['tableHeader']['cells'], 'value');
   }

4. Create an empty table row (representing the header) when not set. I tried to hack up some code, but it's not as easy as it sounds:
   • if we take the line (row index) as the first-body-row-line - 1, it might conflict with an existing line
   • what would the columns be? empty strings?
5. Something else...?

I think option 3 sounds reasonable, but we need to be sure it doesn't change behaviour.

---

In conclusion, we want to have this logic:

if (tableBody is empty) {         // Header is not important at this point.
  return                          // -> Example should be skipped.
}

if (tableHeader is missing) {     // Here it is implied that body is not empty.
  throw exception                 // -> This is not a valid or current case, we should
                                        be notified if this happens in the future.
}

load tableHeader and tableBody    // -> The normal/existing case.

[stof]

Well, is this property actually optional in the Cucumber format that we want to load here ? That's the first question. If it is not actually optional, we should fix the type (option 1). If it is indeed optional, we should fix the code (option 2 is bad in any case: if phpstan tells us that our code is broken, ignoring it still makes our code broken)

[uuf6429]

Well, is this property actually optional in the Cucumber format that we want to load here ?

I cannot fully answer that because this was the first time I'm looking at the code and I haven't in fact even used it before (as far as I know).

The link I posted seemed authoritative - or at least, that seemed to be the point of the https://github.com/cucumber/messages repo.

The generated typescript also seems to make example table headers optional: https://github.com/cucumber/messages/blob/e1537b07e511feb6405ed9aa00261ff79d8a9710/javascript/src/messages.ts#L174 as well as the spec here: https://github.com/cucumber/messages/blob/main/messages.md#examples

(honestly I don't fully understand the reasoning, it seems to me that examples must have a header - otherwise how would it map the values to placeholders? and additionally, how can it determine when a row is a header and when it isn't? afaik the top row was always considered as the header)

[uuf6429]

Making the PHPStan part required would technically break spec parity (option 1).

On the other hand, putting the header parsing in a condition (option 3) might put us in a bind, seeing as we do not distinguish between a header row and a table without a header (we just keep rows).

[ciaranmcnulty]

The cucumber parsers no longer populate TableHeader - I believe it's still in the JSON schema for BC reasons

If this class is only being used for parsing the testData (I can't recall) it's fine to assume it'll never be set.

[uuf6429]

@ciaranmcnulty at the moment it seems that we try to parse that unconditionally:

Gherkin/src/Loader/CucumberNDJsonAstLoader.php

Line 160 in d899102

$table[$tableJson['tableHeader']['location']['line']] = array_column($tableJson['tableHeader']['cells'], 'value');

By your comment, can you confirm that this code would trigger a Undefined array key "tableHeader" PHP warning? And if so, is the best course of action to put that behind an if (isset(...))?

[uuf6429]

If this class is only being used for parsing the testData (I can't recall) it's fine to assume it'll never be set.

I didn't think about it, but it seems to be the case - according to GitHub it is hardly ever used. Too bad it wasn't put inside tests/ - technically there's nothing telling our end users that it should not be used. 🤔

[acoulton]

Ahh - I have just encountered this in one of the cucumber examples we're not currently running, however the NDJSON is not as per the assumption in #294

In this incomplete_scenario_outline.feature there is a Scenario Outline "no steps, no table" which has no table at all.

That validly parses to this NDJSON - the relevant bit being:

{
  "scenario": {
    "description": "",
    "examples": [
      {
        "description": "",
        "id": "5",
        "keyword": "Examples",
        "location": {
          "column": 5,
          "line": 13
        },
        "name": "",
        "tableBody": [],
        "tags": []
      }
    ],
    "id": "6",
    "keyword": "Scenario Outline",
    "location": {
      "column": 3,
      "line": 11
    },
    "name": "no steps, no table",
    "steps": [],
    "tags": []
  }
}

So we can't just take the first row of the body - instead I think we need to ignore any "examples" entries where there is no header and the body is empty?

[uuf6429]

Nice catch!

we need to ignore any "examples" entries where there is no header and the body is empty?

OK, and then we can include this scenario too.

But the original ambiguity is from the ndjson files themselves - ideally we should find a case with no tableHeader but containing tableBody (and of course what is expected to happen then).

[acoulton]

There are other things blocking that scenario (I plan to work on that), but yes we will need to solve this too.

From everything I can see the valid cases are:

• tableHeader is missing, tableBody is empty = no table in feature file, do not create an ExampleTableNode
• tableHeader is present, tableBody is empty = feature file has a table with only heading (first row) - create an ExampleTableNode with that structure
• tableHeader is present, tableBody contains rows = normal case.

I don't see any case where tableBody is present and tableHeader is missing. I think we should treat that as invalid and throw an exception.

[uuf6429]

I don't see any case where tableBody is present and tableHeader is missing. I think we should treat that as invalid and throw an exception.

I'm preparing an issue about this in the cucumber/messages repo...might as well get an official answer at this point.

Thanks for looking into the other details.

[mpkorstanje]

The cucumber parsers no longer populate TableHeader - I believe it's still in the JSON schema for BC reasons

@ciaranmcnulty that doesn't seem to be the case. For example, the parsers in the Gherkin repo all produce the tableHeader. And this is present in the expectations.

https://github.com/search?q=repo%3Acucumber%2Fgherkin%20path%3A%2F%5Etestdata%5C%2Fgood%5C%2F%2F%20tableHeader&type=code

Could you clarify why you believe this?

[stof]

Based on cucumber/messages#298 (comment), I agree with @acoulton here. The case of tableHeader is missing with a non-empty tableBody should trigger an exception in our loader.