From b9475609b3d19f77b72bcbf9fed05db76001be5c Mon Sep 17 00:00:00 2001 From: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com> Date: Mon, 5 Oct 2026 15:00:06 -0400 Subject: [PATCH 1/2] add entity type as enum and clean empty types --- api/src/scripts/populate_db_gtfs.py | 54 +++++-- .../cascade_delete/test_cascade_delete.py | 3 +- .../populate_tests/test_data/sources_test.csv | 7 + .../populate_tests/test_populate.py | 40 +++++ docs/OperationsAPI.yaml | 147 ++++++++++++++++-- .../operations_api/.openapi-generator/FILES | 4 + liquibase/changelog.xml | 2 + liquibase/changes/fix_empty_entity_types.sql | 22 +++ 8 files changed, 255 insertions(+), 24 deletions(-) create mode 100644 liquibase/changes/fix_empty_entity_types.sql diff --git a/api/src/scripts/populate_db_gtfs.py b/api/src/scripts/populate_db_gtfs.py index 518cfd64e..a6feb5cc0 100644 --- a/api/src/scripts/populate_db_gtfs.py +++ b/api/src/scripts/populate_db_gtfs.py @@ -7,6 +7,7 @@ from scripts.load_dataset_on_create import publish_all from scripts.populate_db import DatabasePopulateHelper, set_up_configs +from shared.common.entity_type_enum import EntityType from shared.common.license_utils import assign_license_by_url from shared.database.database import generate_unique_id from shared.database_gen.sqlacodegen_models import ( @@ -104,25 +105,50 @@ def populate_location(self, session, feed, row, stable_id): ) feed.locations = [location] + def normalize_entity_types(self, row, stable_id) -> list[str]: + """ + Parse the `entity_type` cell into a list of valid entity type names. + + The cell is a `|` or `-` separated list. Tokens are trimmed and lowercased, blanks are + dropped and anything that is not a member of `EntityType` is discarded with a warning. + An unparseable cell therefore yields an empty list rather than a blank entity type: a + blank name is rejected by the API's `vp`/`tu`/`sa` enum and makes every response + carrying the feed fail. + """ + raw_value = self.get_safe_value(row, "entity_type", "") + valid_names = {entity_type.value for entity_type in EntityType} + entity_type_names = [] + for token in raw_value.replace("|", "-").split("-"): + name = token.strip().lower() + if not name: + continue + if name not in valid_names: + self.logger.warning(f"Skipping unknown entity type {token!r} for feed {stable_id}") + continue + if name not in entity_type_names: + entity_type_names.append(name) + return entity_type_names + def process_entity_types(self, session: "Session", feed: Gtfsrealtimefeed, row, stable_id): """ Process the entity types for the feed """ - entity_types = self.get_safe_value(row, "entity_type", "").replace("|", "-").split("-") - if len(entity_types) > 0: - if any(entity_types): - feed.entitytypes.clear() - for entity_type_name in entity_types: - entity_type = session.query(Entitytype).filter(Entitytype.name == entity_type_name).first() - - if not entity_type: - entity_type = Entitytype(name=entity_type_name) - if all(entity_type.name != entity.name for entity in feed.entitytypes): - feed.entitytypes.append(entity_type) - session.flush() - else: + entity_type_names = self.normalize_entity_types(row, stable_id) + if not entity_type_names: + # An empty cell leaves the stored entity types alone, like the other + # operator-owned columns in this file. self.logger.warning(f"Entity types array is empty for feed {stable_id}") - feed.entitytypes.clear() + return + + feed.entitytypes.clear() + for entity_type_name in entity_type_names: + entity_type = session.query(Entitytype).filter(Entitytype.name == entity_type_name).first() + + if not entity_type: + entity_type = Entitytype(name=entity_type_name) + if all(entity_type.name != entity.name for entity in feed.entitytypes): + feed.entitytypes.append(entity_type) + session.flush() def inherit_static_feed_locations(self, gtfs_rt_feed, matched_feeds): """ diff --git a/api/tests/integration/cascade_delete/test_cascade_delete.py b/api/tests/integration/cascade_delete/test_cascade_delete.py index 756dd7bf5..d4472270a 100644 --- a/api/tests/integration/cascade_delete/test_cascade_delete.py +++ b/api/tests/integration/cascade_delete/test_cascade_delete.py @@ -458,7 +458,8 @@ def test_delete_gtfsrealtimefeed_cascadeto_feedreference(test_database): def test_delete_gtfsrealtimefeed_cascadeto_entitytypes(test_database): with test_database.start_db_session() as session: gtfsrtfeed = Gtfsrealtimefeed(id="f1") - entitytype = Entitytype(name="type1") + # entitytype.name is constrained to the vp/tu/sa enum. + entitytype = Entitytype(name="vp") session.add_all([gtfsrtfeed, gtfsrtfeed]) gtfsrtfeed.entitytypes.append(entitytype) session.commit() diff --git a/api/tests/integration/populate_tests/test_data/sources_test.csv b/api/tests/integration/populate_tests/test_data/sources_test.csv index a485185e8..b00746bb5 100644 --- a/api/tests/integration/populate_tests/test_data/sources_test.csv +++ b/api/tests/integration/populate_tests/test_data/sources_test.csv @@ -28,3 +28,10 @@ mdb_source_id,data_type,entity_type,location.country_code,location.subdivision_n # Multi-parent regression fixture for #1567: inherit the union of locations from referenced static GTFS feeds. 99992,gtfs-rt,tu,,,,Location Lifecycle Test,TRUE,Multi-parent Locationless RT,,,40|50,https://example.com/gtfs-rt-multi,0,,,,,,,,,,active,,,,, + + +# An empty, padded/mixed-case, or partly invalid entity_type cell must never create a blank +# or unservable Entitytype row. +99993,gtfs-rt,,,,,Entity Type Test,TRUE,Blank Entity Type RT,,,50,https://example.com/gtfs-rt-blank-entity,0,,,,,,,,,,active,,,,, +99994,gtfs-rt, VP | sa ,,,,Entity Type Test,TRUE,Padded Entity Type RT,,,50,https://example.com/gtfs-rt-padded-entity,0,,,,,,,,,,active,,,,, +99995,gtfs-rt,tu-bogus,,,,Entity Type Test,TRUE,Unknown Entity Type RT,,,50,https://example.com/gtfs-rt-unknown-entity,0,,,,,,,,,,active,,,,, diff --git a/api/tests/integration/populate_tests/test_populate.py b/api/tests/integration/populate_tests/test_populate.py index aec769a9a..d2e00c7b3 100644 --- a/api/tests/integration/populate_tests/test_populate.py +++ b/api/tests/integration/populate_tests/test_populate.py @@ -333,3 +333,43 @@ def test_entity_types_overwrite(client: TestClient): assert response.status_code == 200 assert response.json()["entity_types"] == ["sa"] + + +@pytest.mark.parametrize( + "feed_id,expected_entity_types", + [ + ("mdb-99993", set()), + ("mdb-99994", {"vp", "sa"}), + ("mdb-99995", {"tu"}), + ], + ids=[ + "entity_types_blank_cell", + "entity_types_padded_and_mixed_case", + "entity_types_unknown_token_dropped", + ], +) +def test_entity_types_normalization(client: TestClient, feed_id: str, expected_entity_types: set): + """A blank, padded or partly invalid entity_type cell must never reach the API. + + A blank Entitytype name fails the vp/tu/sa enum in the generated models and turns every + response carrying the feed into a 500. + """ + response = client.request( + "GET", + f"/v1/gtfs_rt_feeds/{feed_id}", + headers=authHeaders, + ) + + assert response.status_code == 200 + assert set(response.json()["entity_types"]) == expected_entity_types + + +def test_no_blank_entity_types_in_db(test_database): + """The populate script must not create blank Entitytype rows.""" + from shared.database_gen.sqlacodegen_models import Entitytype + + with test_database.start_db_session() as session: + names = [name for (name,) in session.query(Entitytype.name).all()] + + assert all(name and name.strip() for name in names), f"Blank entity type names found: {names!r}" + assert set(names) <= {"vp", "tu", "sa"}, f"Unexpected entity type names: {names!r}" diff --git a/docs/OperationsAPI.yaml b/docs/OperationsAPI.yaml index 0897a49ec..ffd2cdf10 100644 --- a/docs/OperationsAPI.yaml +++ b/docs/OperationsAPI.yaml @@ -1683,7 +1683,7 @@ components: GtfsFeedContinuousCoverageResponse: type: object description: > - `latest_state` is the feed's latest dataset measured against the one before it; `latest_failure` is the same measurement at the criterion's last observed failure. Both have the structure of an `items[]` entry, and either can be null. Together they name at most four datasets, shared when the latest state is itself the failure. + `latest_state` is the feed's latest dataset measured against the one before it; `latest_failure` is the same at the criterion's last observed failure. Each carries both datasets of the comparison, and either can be null. required: - feed_id - items @@ -1696,9 +1696,9 @@ components: description: Unique identifier of the GTFS feed. example: mdb-123 latest_state: - $ref: "#/components/schemas/GtfsFeedContinuousCoverage" + $ref: "#/components/schemas/GtfsFeedContinuousCoverageBoundary" latest_failure: - $ref: "#/components/schemas/GtfsFeedContinuousCoverage" + $ref: "#/components/schemas/GtfsFeedContinuousCoverageBoundary" total: type: integer description: Total number of matching datasets regardless of limit and offset. @@ -1717,13 +1717,24 @@ components: One entry per dataset, ordered by downloaded_at from newest to oldest. The first entry of the unpaged list is the feed's current coverage; it is marked with `is_latest`. items: $ref: "#/components/schemas/GtfsFeedContinuousCoverage" + GtfsFeedContinuousCoverageBoundary: + type: object + description: > + Two successive datasets: `newer` and the one downloaded immediately before it. `older` is null when `newer` is the feed's first dataset. + required: + - newer + properties: + newer: + $ref: "#/components/schemas/GtfsFeedContinuousCoverage" + older: + $ref: "#/components/schemas/GtfsFeedContinuousCoverage" GtfsFeedContinuousCoverage: type: object description: > The coverage one dataset contributes, and how it lines up with the dataset downloaded just before it. - Three windows are reported. `service_window` is the service dates the validator derived from `calendar.txt` and `calendar_dates.txt`; `feed_info_window` is what the dataset's `feed_info.txt` declares; `coverage_window` is the one the calculation actually used, with `coverage_window_source` naming which of the two it came from. Any of them may be absent when the dataset did not supply the underlying files. + Three windows are reported. `service_window` is the service dates the validator derived from `calendar.txt` and `calendar_dates.txt`; `feed_info_window` is what the dataset's `feed_info.txt` declares; `coverage_window` is the one the criterion measures by - the declared window, falling back to the validated one - with `coverage_window_source` naming which of the two it came from. Any of them may be absent when the dataset did not supply the underlying files. required: - dataset_id - is_latest @@ -1752,14 +1763,12 @@ components: description: > Which input `coverage_window` was taken from. - * `service_dates` - the service dates derived by the validator from `calendar.txt` and - `calendar_dates.txt`. - * `feed_info` - the dates declared in `feed_info.txt`, used only when the service dates - are missing. + * `feed_info` - the dates declared in `feed_info.txt`. * `service_dates` - the service dates derived by the validator from `calendar.txt` and + `calendar_dates.txt`, used when the dataset declares no range. enum: - service_dates - feed_info - example: service_dates + example: feed_info within_max_coverage_window: type: boolean nullable: true @@ -2250,6 +2259,126 @@ components: commit_hash: type: string example: 8635fdac4fbff025b4eaca6972fcc9504bc1552d + GtfsFeedValidationReportsResponse: + type: object + description: > + The feed's validation history, one entry per dataset. `latest` is the entry for the feed's current dataset, whatever page or filter was requested, and is null when the feed has no validated dataset. + required: + - feed_id + - items + - total + - offset + - limit + properties: + feed_id: + type: string + description: Unique identifier of the GTFS feed. + example: mdb-123 + latest: + $ref: "#/components/schemas/GtfsFeedValidationReport" + total: + type: integer + description: Total number of matching datasets regardless of limit and offset. + example: 42 + offset: + type: integer + description: Offset of the first returned item. + example: 0 + limit: + type: integer + description: Maximum number of items returned. + example: 20 + items: + type: array + description: One entry per dataset, ordered by validated_at from newest to oldest. + items: + $ref: "#/components/schemas/GtfsFeedValidationReport" + GtfsFeedValidationReport: + type: object + description: > + The most recent validation report of one dataset. `total_*` counts every notice raised; `unique_*` counts the distinct codes behind them. + required: + - dataset_id + - is_latest + - notices + properties: + dataset_id: + type: string + description: Stable identifier of the validated dataset. + example: mdb-123-202604290029 + is_latest: + type: boolean + description: Whether this is the feed's latest dataset. + example: true + validated_at: + type: string + format: date-time + nullable: true + example: "2026-06-28T00:29:00Z" + validator_version: + type: string + nullable: true + example: 4.2.0 + total_error: + type: integer + nullable: true + example: 10 + total_warning: + type: integer + nullable: true + example: 20 + total_info: + type: integer + nullable: true + example: 30 + unique_error_count: + type: integer + nullable: true + example: 1 + unique_warning_count: + type: integer + nullable: true + example: 2 + unique_info_count: + type: integer + nullable: true + example: 3 + url_json: + type: string + nullable: true + description: JSON validation report URL. + url_html: + type: string + nullable: true + description: HTML validation report URL. + notices: + type: array + description: > + The notice codes raised, newest report only, ordered by severity then by count. Filtered by the `severity` query parameter when one is given. + items: + $ref: "#/components/schemas/GtfsFeedValidationNotice" + GtfsFeedValidationNotice: + type: object + required: + - code + - severity + - total + properties: + code: + type: string + description: Validator notice code. + example: invalid_phone_number + severity: + type: string + enum: + - ERROR + - WARNING + - INFO + example: ERROR + total: + type: integer + description: How many times this code was raised. + example: 10 ValidationReport: description: Validation report type: object diff --git a/functions-python/operations_api/.openapi-generator/FILES b/functions-python/operations_api/.openapi-generator/FILES index 4d34d9c8a..e317b1b7f 100644 --- a/functions-python/operations_api/.openapi-generator/FILES +++ b/functions-python/operations_api/.openapi-generator/FILES @@ -42,8 +42,12 @@ src/feeds_gen/models/gtfs_feed.py src/feeds_gen/models/gtfs_feed_availability_check.py src/feeds_gen/models/gtfs_feed_availability_response.py src/feeds_gen/models/gtfs_feed_continuous_coverage.py +src/feeds_gen/models/gtfs_feed_continuous_coverage_boundary.py src/feeds_gen/models/gtfs_feed_continuous_coverage_file.py src/feeds_gen/models/gtfs_feed_continuous_coverage_response.py +src/feeds_gen/models/gtfs_feed_validation_notice.py +src/feeds_gen/models/gtfs_feed_validation_report.py +src/feeds_gen/models/gtfs_feed_validation_reports_response.py src/feeds_gen/models/gtfs_rt_feed.py src/feeds_gen/models/import_early_access_invited_emails_request.py src/feeds_gen/models/import_early_access_invited_emails_response.py diff --git a/liquibase/changelog.xml b/liquibase/changelog.xml index 23c5d11a0..336fa821d 100644 --- a/liquibase/changelog.xml +++ b/liquibase/changelog.xml @@ -142,6 +142,8 @@ + + diff --git a/liquibase/changes/fix_empty_entity_types.sql b/liquibase/changes/fix_empty_entity_types.sql new file mode 100644 index 000000000..db62ffeff --- /dev/null +++ b/liquibase/changes/fix_empty_entity_types.sql @@ -0,0 +1,22 @@ +-- GTFS-RT feeds carrying a blank entity type make the API return HTTP 500. +-- DatabaseCatalogAPI.yaml restricts entity_types to vp/tu/sa and the generated Pydantic models +-- validate it, so a single blank name fails serialization for the feed and for every unrelated +-- feed on the same /v1/search page. +-- +-- EntityTypeFeed.entity_name references EntityType(name) with no ON DELETE rule (only feed_id +-- cascades), so the join rows have to go first. + +DELETE FROM EntityTypeFeed +WHERE btrim(entity_name) = ''; + +DELETE FROM EntityType +WHERE btrim(name) = ''; + +-- Mirrors the entity_types enum in docs/DatabaseCatalogAPI.yaml and +-- api/src/shared/common/entity_type_enum.py, so no writer can reintroduce an unservable value. +ALTER TABLE EntityType + ADD CONSTRAINT entitytype_name_valid + CHECK (name IN ('vp', 'tu', 'sa')); + +-- feedsearch aggregates entity_name into its `entities` column, which /v1/search returns verbatim. +REFRESH MATERIALIZED VIEW feedsearch; From dc79ec33080edcf30a82ce95fc955fbdde1bdb63 Mon Sep 17 00:00:00 2001 From: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com> Date: Mon, 5 Oct 2026 17:03:11 -0400 Subject: [PATCH 2/2] enhance comments --- api/src/scripts/populate_db_gtfs.py | 11 ++--------- .../cascade_delete/test_cascade_delete.py | 1 - .../populate_tests/test_data/sources_test.csv | 3 +-- .../integration/populate_tests/test_populate.py | 6 +----- liquibase/changes/fix_empty_entity_types.sql | 14 +++----------- 5 files changed, 7 insertions(+), 28 deletions(-) diff --git a/api/src/scripts/populate_db_gtfs.py b/api/src/scripts/populate_db_gtfs.py index a6feb5cc0..acdb34ed7 100644 --- a/api/src/scripts/populate_db_gtfs.py +++ b/api/src/scripts/populate_db_gtfs.py @@ -107,13 +107,7 @@ def populate_location(self, session, feed, row, stable_id): def normalize_entity_types(self, row, stable_id) -> list[str]: """ - Parse the `entity_type` cell into a list of valid entity type names. - - The cell is a `|` or `-` separated list. Tokens are trimmed and lowercased, blanks are - dropped and anything that is not a member of `EntityType` is discarded with a warning. - An unparseable cell therefore yields an empty list rather than a blank entity type: a - blank name is rejected by the API's `vp`/`tu`/`sa` enum and makes every response - carrying the feed fail. + Parse the `|` or `-` separated `entity_type` cell into a list of valid entity type names. """ raw_value = self.get_safe_value(row, "entity_type", "") valid_names = {entity_type.value for entity_type in EntityType} @@ -135,8 +129,7 @@ def process_entity_types(self, session: "Session", feed: Gtfsrealtimefeed, row, """ entity_type_names = self.normalize_entity_types(row, stable_id) if not entity_type_names: - # An empty cell leaves the stored entity types alone, like the other - # operator-owned columns in this file. + # An empty cell leaves the stored entity types alone. self.logger.warning(f"Entity types array is empty for feed {stable_id}") return diff --git a/api/tests/integration/cascade_delete/test_cascade_delete.py b/api/tests/integration/cascade_delete/test_cascade_delete.py index d4472270a..7b9eed922 100644 --- a/api/tests/integration/cascade_delete/test_cascade_delete.py +++ b/api/tests/integration/cascade_delete/test_cascade_delete.py @@ -458,7 +458,6 @@ def test_delete_gtfsrealtimefeed_cascadeto_feedreference(test_database): def test_delete_gtfsrealtimefeed_cascadeto_entitytypes(test_database): with test_database.start_db_session() as session: gtfsrtfeed = Gtfsrealtimefeed(id="f1") - # entitytype.name is constrained to the vp/tu/sa enum. entitytype = Entitytype(name="vp") session.add_all([gtfsrtfeed, gtfsrtfeed]) gtfsrtfeed.entitytypes.append(entitytype) diff --git a/api/tests/integration/populate_tests/test_data/sources_test.csv b/api/tests/integration/populate_tests/test_data/sources_test.csv index b00746bb5..0c50376bc 100644 --- a/api/tests/integration/populate_tests/test_data/sources_test.csv +++ b/api/tests/integration/populate_tests/test_data/sources_test.csv @@ -30,8 +30,7 @@ mdb_source_id,data_type,entity_type,location.country_code,location.subdivision_n 99992,gtfs-rt,tu,,,,Location Lifecycle Test,TRUE,Multi-parent Locationless RT,,,40|50,https://example.com/gtfs-rt-multi,0,,,,,,,,,,active,,,,, -# An empty, padded/mixed-case, or partly invalid entity_type cell must never create a blank -# or unservable Entitytype row. +# Blank, padded/mixed-case and partly invalid entity_type cells. 99993,gtfs-rt,,,,,Entity Type Test,TRUE,Blank Entity Type RT,,,50,https://example.com/gtfs-rt-blank-entity,0,,,,,,,,,,active,,,,, 99994,gtfs-rt, VP | sa ,,,,Entity Type Test,TRUE,Padded Entity Type RT,,,50,https://example.com/gtfs-rt-padded-entity,0,,,,,,,,,,active,,,,, 99995,gtfs-rt,tu-bogus,,,,Entity Type Test,TRUE,Unknown Entity Type RT,,,50,https://example.com/gtfs-rt-unknown-entity,0,,,,,,,,,,active,,,,, diff --git a/api/tests/integration/populate_tests/test_populate.py b/api/tests/integration/populate_tests/test_populate.py index d2e00c7b3..954ca30b8 100644 --- a/api/tests/integration/populate_tests/test_populate.py +++ b/api/tests/integration/populate_tests/test_populate.py @@ -349,11 +349,7 @@ def test_entity_types_overwrite(client: TestClient): ], ) def test_entity_types_normalization(client: TestClient, feed_id: str, expected_entity_types: set): - """A blank, padded or partly invalid entity_type cell must never reach the API. - - A blank Entitytype name fails the vp/tu/sa enum in the generated models and turns every - response carrying the feed into a 500. - """ + """A blank, padded or partly invalid entity_type cell must never reach the API.""" response = client.request( "GET", f"/v1/gtfs_rt_feeds/{feed_id}", diff --git a/liquibase/changes/fix_empty_entity_types.sql b/liquibase/changes/fix_empty_entity_types.sql index db62ffeff..8806b31cc 100644 --- a/liquibase/changes/fix_empty_entity_types.sql +++ b/liquibase/changes/fix_empty_entity_types.sql @@ -1,10 +1,5 @@ --- GTFS-RT feeds carrying a blank entity type make the API return HTTP 500. --- DatabaseCatalogAPI.yaml restricts entity_types to vp/tu/sa and the generated Pydantic models --- validate it, so a single blank name fails serialization for the feed and for every unrelated --- feed on the same /v1/search page. --- --- EntityTypeFeed.entity_name references EntityType(name) with no ON DELETE rule (only feed_id --- cascades), so the join rows have to go first. +-- Blank entity types make every API response carrying the feed fail the vp/tu/sa enum. +-- EntityTypeFeed.entity_name has no ON DELETE rule, so the join rows go first. DELETE FROM EntityTypeFeed WHERE btrim(entity_name) = ''; @@ -12,11 +7,8 @@ WHERE btrim(entity_name) = ''; DELETE FROM EntityType WHERE btrim(name) = ''; --- Mirrors the entity_types enum in docs/DatabaseCatalogAPI.yaml and --- api/src/shared/common/entity_type_enum.py, so no writer can reintroduce an unservable value. ALTER TABLE EntityType ADD CONSTRAINT entitytype_name_valid CHECK (name IN ('vp', 'tu', 'sa')); --- feedsearch aggregates entity_name into its `entities` column, which /v1/search returns verbatim. -REFRESH MATERIALIZED VIEW feedsearch; +REFRESH MATERIALIZED VIEW CONCURRENTLY feedsearch;