Table header is optional in the spec, but required in practice

[uuf6429]

🤔 What's the problem you've observed?

Hi!

We've been having a bit of a discussion over Examples.tableHeader:

messages/jsonschema/GherkinDocument.json

Line 138 in e1537b0

"tableHeader": {

The spec defines it as optional since the beginning.

Whereas tableBody used to be optional and is now required since ce9f289#diff-69252728078b60bf59750707428b38924591264e92497fef90006dd33402e70aR112.

In practice the header is required to be able to replace placeholders effectively: https://cucumber.io/docs/gherkin/reference/#scenario-outline

As far as I can see there are no test fixtures on this case.

✨ Do you have a proposal for making it better?

1. Would be great to have some consensus:
   1. Should tableHeader also be required?
   2. If not, should we pick the first tableBody row as the header?
2. and additionally, a test/fixture to disambiguate the situation

📚 Any additional context?

No response

[mpkorstanje]

I've done a small investigation.

0. Scenario outlines without examples are quite tricky. If the Examples header is omitted, the scenario outline is effectively a scenario. Which is turned into an empty pickle - the empty pickle passes by default. If the Examples keyword is not omitted but all the examples are then there are no pickles to execute. This is currently covered by https://github.com/cucumber/gherkin/blob/main/testdata/good/incomplete_scenario_outline.feature

1. The JSON Schema was introduced as part of Json schema for message protocol common#1414. At the time we used a monorepo which we then later split up. Because the PR was not squashed we get to see all the gory details and because we then split the monorepo some context is missing. Long story short: while it looks like a change, tableBody was actually required in the JSON Schema from its introduction (see messages/jsonschema/GherkinDocument.json).

2. Cucumber Messages originally used Protobuf as its schema. The JSON Schema was a replacement for this. The expectations around tableBody and tableHeader didn't change. This can be seen in Json schema for message protocol common#1414 in gherkin/testdata/good/incomplete_scenario_outline.feature.ast.ndjson.

   For tableBody it would appear that because the original Protobuf implementation did not make a distinction between the empty list [] and null and always serialized either value to [] in json. So the tableHeader being an object became optional in the json schema, the tableBody being a list became required.

So should tableHeader also be required?

No. If the Examples section does not contain any rows then tableHeader is not included. It is not very pretty, and you could reasonably argue that the tableHeader should have been omitted from the schema in the first place. Yet this wasn't done and it is important that we specify how it works at present.

Should we pick the first tableBody row as the header?

Also no. If implemented correctly a Gherkin parser will populate the tableHeader before adding any entries to tableBody. So tableHeader == null/undefined always implies that tableBody is empty. But I don't think such a constraint can be reasonably be added to JSON schema.

And the correct way to get all table rows from the document would be something like

var example = ...

if example.tableHeader is null or undefined:
  return [];
else 
  return concatenate ( example.tableHeader,  example.tableBody )

I think that https://github.com/cucumber/gherkin/blob/main/testdata/good/incomplete_scenario_outline.feature and it's expectations are sufficient to cover all current expectations.

• https://github.com/cucumber/gherkin/blob/main/testdata/good/incomplete_scenario_outline.feature.ast.ndjson
• https://github.com/cucumber/gherkin/blob/main/testdata/good/incomplete_scenario_outline.feature.pickles.ndjson

Is there something you would add?

In practice the header is required to be able to replace placeholders effectively: https://cucumber.io/docs/gherkin/reference/#scenario-outline

To wit, table headers determine the replacement pattern. If there are no table headers, there are no replacement patterns and nothing should be replaced.

[mpkorstanje]

A possible good clarification would be to document the relationship between tableHeader and tableBody.

[uuf6429]

@mpkorstanje thanks for the extensive writeup!

But I don't think such a constraint can be reasonably be added to JSON schema.

As I understand and given your pseudocode, I imagine this should work and be reasonably understandable (more info):

{
  "type": "object",

  "properties": {
    "tableHeader": { ... },
    "tableBody": { ... },
  },

  "required": [],                   // neither are required by default

  "dependentRequired": {
    "tableHeader": ["tableBody"]    // table body is required when table header is provided
  }
}

Is there something you would add?

We could write a short description of the pseudocode you provided as the description of the tableHeader prop (this should be the low effort option, since it shouldn't affect anyone).

[mpkorstanje]

We could write a short description of the pseudocode you provided as the description of the tableHeader prop (this should be the low effort option, since it shouldn't affect anyone).

Agreed. Would you like to send a PR for that?

[uuf6429]

Agreed. Would you like to send a PR for that?

I could, but I think I would favour changing the schema first (it would also be more helpful in the long term e.g. in tooling). Any thoughts about using dependentRequired?

[mpkorstanje]

Assuming dependentRequired doesn't break the code gen in a horrible way I don't see a problem with that specific attribute.

However it does require tableBody to no longer be required which would be a breaking change. I'd like to avoid a breaking change.

I'm also not sure that the relationship is expressed correctly. The tableHeader is only required if there is a non-emtpy tableBody which isn't something that dependentRequired can express (I think).

[uuf6429]

With anyOf it's doable and is relatively easy to come up with, but unfortunately quite verbose.

All we need to do is to have each valid case as a separate one inside anyOf, but most importantly each case must not overlap with the others.

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "type": "object",

  "properties": {
    "some-common-property": {              // Just to illustrate that common properties can go here.
      "type": "string"
    }
  },

  "required": ["tableBody"],               // As is right now, in this case it is actually required like that.

  "unevaluatedProperties": false,          // This is so we can prohibit tableHeader in Case 1.

  "anyOf": [

    { // Case 1: No header, empty body.
      "properties": {
        "tableBody": {
          "$ref": "#/$defs/tableBody",
          "maxItems": 0                    // This means when the header is missing, body must be empty.
        }
      }
    },

    { // Case 2: Header, body.
      "properties": {
        "tableHeader": {
          "$ref": "#/$defs/tableHeader"
        },
        "tableBody": {
          "$ref": "#/$defs/tableBody"
        }
      },
      "required": ["tableHeader"]
    },

  ],

  "$defs": {                  // Avoid repeating those schemas - makes even more sense when used in practice.
    "tableHeader": {
      "type": "object"
    },
    "tableBody": {
      "type": "array"
    }
  }
}

You can try it out yourself here. Copy & paste that schema and try these cases:

• These should fail:

  {
    "tableBody": [{}]
  }

  {
  }

  {
    "tableHeader": {}
  }

• And these should pass:

  {
    "tableBody": []
  }

  {
    "tableHeader": {},
    "tableBody": [{}]
  }

  {
    "tableHeader": {},
    "tableBody": []
  }

[mpkorstanje]

Using anyOf would seem to break the code generation. So that makes it more than a quick fix.