Skip to content

[ENHANCEMENT](transportation) Make segment connectors required - #692

Merged
Victor Schappert (vcschapp) merged 1 commit into
vnextfrom
fix-segment-connectors-default
Aug 26, 2026
Merged

[ENHANCEMENT](transportation) Make segment connectors required#692
Victor Schappert (vcschapp) merged 1 commit into
vnextfrom
fix-segment-connectors-default

Conversation

@sethfitz

@sethfitz Seth Fitzsimmons (sethfitz) commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Major change release plan

A. Expected release date for this MAJOR change

Bundled into 2.0.0. Since it matches the published data, it doesn't strictly need to be treated as a breaking change.

B. Related MINOR change steps

  • None. The published data already satisfies the tightened constraint, so there is no intermediate step where producers must be given time to comply.

C. Public documentation and messaging plan

The schema reference regenerates from the model, and every Docusaurus segment example in this PR now carries a connectors array, so the visible change is that connectors moves into the required-property list for all three segment subtypes. This is effectively a fix for the docs to match what's published in the data.

Description

connectors was declared optional with an empty-list default that its own min_length=2 constraint rejects:

connectors: Annotated[
    list[ConnectorReference] | None,
    Field(min_length=2, description="..."),
    UniqueItemsConstraint(),
] = []          # violates min_length=2

Pydantic does not validate defaults unless validate_default=True, so the invalid value was accepted on the way in and refused on the way back out. A segment constructed without connectors dumped to connectors: [], and re-validating that dump failed — read → dump → read was not idempotent for any segment lacking connectors.

Rather than correct the default, this makes the field required. A segment is by definition physically connected to at least two connectors, the published data has always reflected that, and an optional field whose only legal values are arrays of two or more was never expressing the real constraint.

Changes:

  • connectors is now list[ConnectorReference] with no default, declared beside subtype in the model's required section.
  • The YAML schema gains connectors in its segment required list and drops the matching default: [] that sat beside minItems: 2.
  • Every valid segment example gains a two-connector array. Every segment counterexample that was not itself testing connectors gains one too, so each keeps failing only for the defect it names (checklist item 3). Both example trees are updated: reference/ (read by the test suite) and the root examples//counterexamples/ trees (the docs build symlinks examples/). theme-type-mismatch.json is deliberately untouched — it declares theme: buildings with type: segment and never reaches a segment arm.

Reference

  1. Fixes Segments with no connectors fail validation after a round trip #669
  2. The same contradiction in the pre-Pydantic schema: https://github.com/OvertureMaps/schema/blob/main/schema/transportation/segment.yaml#L61-L79 (noted by Victor Schappert (@vcschapp) on Segments with no connectors fail validation after a round trip #669)

Testing

No published data is affected. Every segment in the most recent release satisfies the tightened constraint — verified against release 2026-08-19.0 (350,469,378 segments across 128 partitions), which returned zero nulls, zero empty arrays, and zero single-element arrays for all three subtypes:

SELECT
  subtype,
  count(*)                               AS n_rows,
  count_if(connectors IS NULL)           AS null_connectors,
  count_if(cardinality(connectors) = 0)  AS empty_connectors,
  count_if(cardinality(connectors) = 1)  AS one_connector,
  count_if(cardinality(connectors) >= 2) AS two_or_more
FROM v2026_08_19_0
WHERE theme = 'transportation' AND "type" = 'segment'
GROUP BY GROUPING SETS ((subtype), ())
ORDER BY subtype;

This PR adds no new test, and the example updates are not proofs of the fix — they are the corpus catching up to a tightened schema. What pins the change is the regenerated JSON Schema baseline, which fails if connectors stops being required, and the round trip that motivated #669, which now succeeds because the field has no default at all rather than a corrected one.

Separately verified while investigating: across every field default in the whole model tree, the three segment arms and their shared TransportationSegment base were the only ones their own annotations reject. Nothing else in the schema had the defect.

Required-ness propagated to the generated PySpark layer, not just the JSON Schema: the regenerated transportation/segment.py now emits _connectors_check with check_required(F.col("connectors")). Control: optional access_restrictions still gets no such check.

JSON Schema baseline delta is exactly connectors joining required in all three arms. The connectors subschema itself is byte-identical to before, because the generator already emitted a non-nullable array and expressed optionality solely through required.

Full suite: 6215 passed.

Checklist

  1. Add relevant examples.
  2. Add relevant counterexamples.
  3. Update any counterexamples that became obsolete.
  4. Update in-schema documentation using plain English written in complete sentences, if an update is required. — The field description was already accurate; no wording change needed.
  5. Update Docusaurus documentation, if an update is required. — The embedded examples are updated; prose pages not reviewed.

Documentation website

Docs preview for this PR.

`connectors` was optional with a `[]` default that its own
`min_length=2` constraint rejected. Pydantic does not validate defaults
unless `validate_default=True`, so the invalid value was accepted on the
way in and refused on the way back out: a segment built without
connectors dumped to `connectors: []`, which re-validation rejected.

Every segment is physically connected to at least two connectors, and
the published data agrees -- across all 350,469,378 segments in release
2026-08-19.0 there is not one null, empty, or single-element
`connectors`.  So the field becomes required rather than
optional-with-a-default, and moves up beside `subtype` in the model's
required section. The YAML schema gains `connectors` in its segment
`required` list, dropping the matching `default: []` beside `minItems:
2`.

Every valid segment example gains a two-connector array, and so does
every segment counterexample that was not testing connectors, so each
keeps failing only for the defect it names. Both corpora are updated:
`reference/` (read by the test suite) and the root
`examples/`/`counterexamples/` trees (the docs site symlinks
`examples/`). `theme-type-mismatch.json` is left alone -- it declares
`theme: buildings` with `type: segment` and never reaches a segment arm.

Fixes #669

Signed-off-by: Seth Fitzsimmons <seth@mojodna.net>
@github-actions

Copy link
Copy Markdown

🗺️ Schema reference docs preview is live!

🌍 Preview https://staging.overturemaps.org/schema/pr/692/schema/index.html
🕐 Updated Aug 24, 2026 21:09 UTC
📝 Commit b5ecb2c
🔧 env SCHEMA_PREVIEW true

Note

♻️ This preview updates automatically with each push to this PR.

@sethfitz Seth Fitzsimmons (sethfitz) added the change type - major 🚨 Major schema change. See https://lf-overturemaps.atlassian.net/wiki/x/GgDa label Aug 24, 2026
@vcschapp

Copy link
Copy Markdown
Collaborator

⚠️ PR target check (advisory)

  • This PR targets main but has the change type - major 🚨 label. Major/breaking changes should typically target vnext instead.

Reviewer is the source of truth for change classification. This warning is advisory only.

We should discuss this. I'm not 100% clear on how we should handle the pre-2.0.0 breaking change collection.

@vcschapp

Copy link
Copy Markdown
Collaborator

I think it makes sense to target this to vnext. Then we should merge vnext down to main prior to doing the v2.0.0 release.

John McCall (@lowlydba) Do we have to do anything to keep vnext up-to-date with main before merging a PR to vnext?

@lowlydba

John McCall (lowlydba) commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Do we have to do anything to keep vnext up-to-date with main before merging a PR to vnext?

Victor Schappert (@vcschapp)

No, that's already automated: .github/workflows/rebase-vnext.yaml force-rebases vnext onto main on every push to main (via the overture-pull-requester app), and skips itself on the vnext->main release merge to avoid a no-op. Right now vnext and main are at the same commit (5dc32f39), so nothing to do before retargeting this to vnext.

If a rebase ever conflicts, the workflow opens an issue assigned to the triggering PR's author for manual resolution.

@vcschapp
Victor Schappert (vcschapp) changed the base branch from main to vnext August 26, 2026 15:53
@vcschapp
Victor Schappert (vcschapp) dismissed stale reviews from Dana Bauer (danabauer) and themself August 26, 2026 15:53

The base branch was changed.

@vcschapp
Victor Schappert (vcschapp) merged commit 20ed41c into vnext Aug 26, 2026
37 of 38 checks passed
@vcschapp
Victor Schappert (vcschapp) deleted the fix-segment-connectors-default branch August 26, 2026 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change type - major 🚨 Major schema change. See https://lf-overturemaps.atlassian.net/wiki/x/GgDa

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Segments with no connectors fail validation after a round trip

5 participants