From 615fe10670ab5726d29022f79ac6459c8c19ee73 Mon Sep 17 00:00:00 2001 From: James Date: Tue, 22 Nov 2022 08:38:34 +0000 Subject: [PATCH] geojsontojson: Include name in organisation and phase references https://github.com/Open-Telecoms-Data/lib-cove-ofds/issues/33 And * check id is a string * reorder and comment code to make it easier to read --- CHANGELOG.md | 2 + libcoveofds/geojson.py | 107 +++++++++++------- .../organisations_1.expected.json | 24 ++-- .../organisations_1.meta.expected.json | 15 +++ .../organisations_1.nodes.geo.json | 12 +- .../organisations_1.spans.geo.json | 6 +- ...organisations_inconsistent_1.expected.json | 18 ++- ...isations_inconsistent_1.meta.expected.json | 12 ++ .../geojson_to_json/phases_1.expected.json | 12 +- .../phases_1.meta.expected.json | 9 ++ .../phases_inconsistent_1.expected.json | 12 +- .../phases_inconsistent_1.meta.expected.json | 9 ++ 12 files changed, 167 insertions(+), 71 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa4197c..41c5e2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - add more information to errors - Add unique ID checks - Add path to output +- GeoJSON to JSON: + - Include name in organisation and phase references ## [0.4.0] - 2022-11-09 diff --git a/libcoveofds/geojson.py b/libcoveofds/geojson.py index 2baba01..a7a9875 100644 --- a/libcoveofds/geojson.py +++ b/libcoveofds/geojson.py @@ -1,5 +1,6 @@ import copy from collections import defaultdict +from typing import Optional from json_merge_patch import create_patch as json_diff_function @@ -231,11 +232,11 @@ def _process_network(self, geojson_feature_node_or_span: dict) -> None: ): out: list = [] for phase_reference in contract["relatedPhases"]: - phase_id = self._process_phase( + phase_data = self._process_phase( network.get("id"), phase_reference ) - if phase_id: - out.append({"id": phase_id}) + if phase_data: + out.append(phase_data) else: out.append(phase_reference) contract["relatedPhases"] = out @@ -255,15 +256,15 @@ def _process_node(self, geojson_feature_node: dict) -> None: # sort organisations for field in ["physicalInfrastructureProvider", "networkProvider"]: if isinstance(node.get(field), dict) and node.get(field): - organisation_id = self._process_organisation(network_id, node[field]) - if organisation_id: - node[field] = {"id": organisation_id} + organisation_data = self._process_organisation(network_id, node[field]) + if organisation_data: + node[field] = organisation_data # sort phase if isinstance(node.get("phase"), dict) and node.get("phase"): - phase_id = self._process_phase(network_id, node["phase"]) - if phase_id: - node["phase"] = {"id": phase_id} + phase_data = self._process_phase(network_id, node["phase"]) + if phase_data: + node["phase"] = phase_data if geojson_feature_node.get("geometry"): node["location"] = geojson_feature_node["geometry"] @@ -285,15 +286,15 @@ def _process_span(self, geojson_feature_span: dict) -> None: # sort organisations for field in ["physicalInfrastructureProvider", "networkProvider", "supplier"]: if isinstance(span.get(field), dict) and span.get(field): - organisation_id = self._process_organisation(network_id, span[field]) - if organisation_id: - span[field] = {"id": organisation_id} + organisation_data = self._process_organisation(network_id, span[field]) + if organisation_data: + span[field] = organisation_data # sort phase if isinstance(span.get("phase"), dict) and span.get("phase"): - phase_id = self._process_phase(network_id, span["phase"]) - if phase_id: - span["phase"] = {"id": phase_id} + phase_data = self._process_phase(network_id, span["phase"]) + if phase_data: + span["phase"] = phase_data if geojson_feature_span.get("geometry"): span["route"] = geojson_feature_span["geometry"] @@ -303,38 +304,58 @@ def _process_span(self, geojson_feature_span: dict) -> None: self._networks[network_id]["spans"].append(span) - def _process_phase(self, network_id: str, phase: dict) -> str: + def _process_phase(self, network_id: str, phase: dict) -> Optional[dict]: phase_id = phase.get("id") - if phase_id: - if phase_id in self._networks[network_id]["phases"]: - # Is it inconsistent with what we have seen before? - if json_diff_function( - self._networks[network_id]["phases"][phase_id], phase - ): - self._inconsistent_phase_ids_by_network_id[network_id].add(phase_id) - else: - self._networks[network_id]["phases"][phase_id] = phase - return phase_id - return "" + # If no id, can't do anything. TODO log somewhere? + if not phase_id or not isinstance(phase_id, str): + return None + # Check data + if phase_id in self._networks[network_id]["phases"]: + # Is it inconsistent with what we have seen before? + if json_diff_function( + self._networks[network_id]["phases"][phase_id], phase + ): + self._inconsistent_phase_ids_by_network_id[network_id].add(phase_id) + else: + # Not seen this before; store it + self._networks[network_id]["phases"][phase_id] = phase + # Make output + out: dict = {"id": phase_id} + # Take name from data on network, not data that is passed to this function. + # This means that if inconsistent names are in input, we'll have consistent names in the output. + name = self._networks[network_id]["phases"][phase_id].get("name") + if name: + out["name"] = name + return out - def _process_organisation(self, network_id: str, organisation: dict) -> str: + def _process_organisation( + self, network_id: str, organisation: dict + ) -> Optional[dict]: organisation_id = organisation.get("id") - if organisation_id: - if organisation_id in self._networks[network_id]["organisations"]: - # Is it inconsistent with what we have seen before? - if json_diff_function( - self._networks[network_id]["organisations"][organisation_id], - organisation, - ): - self._inconsistent_organisation_ids_by_network_id[network_id].add( - organisation_id - ) - else: - self._networks[network_id]["organisations"][ + # If no id, can't do anything. TODO log somewhere? + if not organisation_id or not isinstance(organisation_id, str): + return None + # Check data + if organisation_id in self._networks[network_id]["organisations"]: + # Is it inconsistent with what we have seen before? + if json_diff_function( + self._networks[network_id]["organisations"][organisation_id], + organisation, + ): + self._inconsistent_organisation_ids_by_network_id[network_id].add( organisation_id - ] = organisation - return organisation_id - return "" + ) + else: + # Not seen this before; store it + self._networks[network_id]["organisations"][organisation_id] = organisation + # Make output + out: dict = {"id": organisation_id} + # Take name from data on network, not data that is passed to this function. + # This means that if inconsistent names are in input, we'll have consistent names in the output. + name = self._networks[network_id]["organisations"][organisation_id].get("name") + if name: + out["name"] = name + return out def get_json(self) -> dict: out: dict = {"networks": []} diff --git a/tests/fixtures/geojson_to_json/organisations_1.expected.json b/tests/fixtures/geojson_to_json/organisations_1.expected.json index 51d04c2..7c0a2d2 100644 --- a/tests/fixtures/geojson_to_json/organisations_1.expected.json +++ b/tests/fixtures/geojson_to_json/organisations_1.expected.json @@ -8,10 +8,12 @@ "id": "1", "name": "Accra", "physicalInfrastructureProvider": { - "id": "GH-RGD-CS111111111" + "id": "GH-RGD-CS111111111", + "name": "FibreCo" }, "networkProvider": { - "id": "GH-RGD-CS222222222" + "id": "GH-RGD-CS222222222", + "name": "FastWeb" }, "location": { "type": "Point", @@ -26,10 +28,12 @@ "name": "Kumasi", "status": "operational", "physicalInfrastructureProvider": { - "id": "GH-RGD-CS111111111" + "id": "GH-RGD-CS111111111", + "name": "FibreCo" }, "networkProvider": { - "id": "GH-RGD-CS222222222" + "id": "GH-RGD-CS222222222", + "name": "FastWeb" }, "location": { "type": "Point", @@ -47,10 +51,12 @@ "start": "1", "end": "2", "physicalInfrastructureProvider": { - "id": "GH-RGD-CS111111111" + "id": "GH-RGD-CS111111111", + "name": "FibreCo" }, "networkProvider": { - "id": "GH-RGD-CS222222222" + "id": "GH-RGD-CS222222222", + "name": "FastWeb" }, "route": { "type": "LineString", @@ -114,11 +120,13 @@ "organisations": [ { "id": "GH-RGD-CS111111111", - "name": "FibreCo" + "name": "FibreCo", + "country": "IE" }, { "id": "GH-RGD-CS222222222", - "name": "FastWeb" + "name": "FastWeb", + "country": "IE" } ] } diff --git a/tests/fixtures/geojson_to_json/organisations_1.meta.expected.json b/tests/fixtures/geojson_to_json/organisations_1.meta.expected.json index bcfe7c0..5a3514a 100644 --- a/tests/fixtures/geojson_to_json/organisations_1.meta.expected.json +++ b/tests/fixtures/geojson_to_json/organisations_1.meta.expected.json @@ -24,12 +24,18 @@ "/networks/nodes/physicalInfrastructureProvider/id": { "count": 2 }, + "/networks/nodes/physicalInfrastructureProvider/name": { + "count": 2 + }, "/networks/nodes/networkProvider": { "count": 2 }, "/networks/nodes/networkProvider/id": { "count": 2 }, + "/networks/nodes/networkProvider/name": { + "count": 2 + }, "/networks/nodes/location": { "count": 2 }, @@ -63,12 +69,18 @@ "/networks/spans/physicalInfrastructureProvider/id": { "count": 1 }, + "/networks/spans/physicalInfrastructureProvider/name": { + "count": 1 + }, "/networks/spans/networkProvider": { "count": 1 }, "/networks/spans/networkProvider/id": { "count": 1 }, + "/networks/spans/networkProvider/name": { + "count": 1 + }, "/networks/spans/route": { "count": 1 }, @@ -86,6 +98,9 @@ }, "/networks/organisations/name": { "count": 2 + }, + "/networks/organisations/country": { + "count": 2 } }, "inconsistent_phases_by_network_id": {}, diff --git a/tests/fixtures/geojson_to_json/organisations_1.nodes.geo.json b/tests/fixtures/geojson_to_json/organisations_1.nodes.geo.json index bdb5ccf..5301718 100644 --- a/tests/fixtures/geojson_to_json/organisations_1.nodes.geo.json +++ b/tests/fixtures/geojson_to_json/organisations_1.nodes.geo.json @@ -15,11 +15,13 @@ "name": "Accra", "physicalInfrastructureProvider": { "id": "GH-RGD-CS111111111", - "name": "FibreCo" + "name": "FibreCo", + "country": "IE" }, "networkProvider": { "id": "GH-RGD-CS222222222", - "name": "FastWeb" + "name": "FastWeb", + "country": "IE" }, "network": { "id": "a096d627-72e1-4f9b-b129-951b1737bff4", @@ -42,11 +44,13 @@ "status": "operational", "physicalInfrastructureProvider": { "id": "GH-RGD-CS111111111", - "name": "FibreCo" + "name": "FibreCo", + "country": "IE" }, "networkProvider": { "id": "GH-RGD-CS222222222", - "name": "FastWeb" + "name": "FastWeb", + "country": "IE" }, "network": { "id": "a096d627-72e1-4f9b-b129-951b1737bff4", diff --git a/tests/fixtures/geojson_to_json/organisations_1.spans.geo.json b/tests/fixtures/geojson_to_json/organisations_1.spans.geo.json index d0ef682..7c8af00 100644 --- a/tests/fixtures/geojson_to_json/organisations_1.spans.geo.json +++ b/tests/fixtures/geojson_to_json/organisations_1.spans.geo.json @@ -100,11 +100,13 @@ }, "physicalInfrastructureProvider": { "id": "GH-RGD-CS111111111", - "name": "FibreCo" + "name": "FibreCo", + "country": "IE" }, "networkProvider": { "id": "GH-RGD-CS222222222", - "name": "FastWeb" + "name": "FastWeb", + "country": "IE" }, "network": { "id": "a096d627-72e1-4f9b-b129-951b1737bff4", diff --git a/tests/fixtures/geojson_to_json/organisations_inconsistent_1.expected.json b/tests/fixtures/geojson_to_json/organisations_inconsistent_1.expected.json index 51d04c2..5ea7df8 100644 --- a/tests/fixtures/geojson_to_json/organisations_inconsistent_1.expected.json +++ b/tests/fixtures/geojson_to_json/organisations_inconsistent_1.expected.json @@ -8,10 +8,12 @@ "id": "1", "name": "Accra", "physicalInfrastructureProvider": { - "id": "GH-RGD-CS111111111" + "id": "GH-RGD-CS111111111", + "name": "FibreCo" }, "networkProvider": { - "id": "GH-RGD-CS222222222" + "id": "GH-RGD-CS222222222", + "name": "FastWeb" }, "location": { "type": "Point", @@ -26,10 +28,12 @@ "name": "Kumasi", "status": "operational", "physicalInfrastructureProvider": { - "id": "GH-RGD-CS111111111" + "id": "GH-RGD-CS111111111", + "name": "FibreCo" }, "networkProvider": { - "id": "GH-RGD-CS222222222" + "id": "GH-RGD-CS222222222", + "name": "FastWeb" }, "location": { "type": "Point", @@ -47,10 +51,12 @@ "start": "1", "end": "2", "physicalInfrastructureProvider": { - "id": "GH-RGD-CS111111111" + "id": "GH-RGD-CS111111111", + "name": "FibreCo" }, "networkProvider": { - "id": "GH-RGD-CS222222222" + "id": "GH-RGD-CS222222222", + "name": "FastWeb" }, "route": { "type": "LineString", diff --git a/tests/fixtures/geojson_to_json/organisations_inconsistent_1.meta.expected.json b/tests/fixtures/geojson_to_json/organisations_inconsistent_1.meta.expected.json index 7079725..44f7def 100644 --- a/tests/fixtures/geojson_to_json/organisations_inconsistent_1.meta.expected.json +++ b/tests/fixtures/geojson_to_json/organisations_inconsistent_1.meta.expected.json @@ -24,12 +24,18 @@ "/networks/nodes/physicalInfrastructureProvider/id": { "count": 2 }, + "/networks/nodes/physicalInfrastructureProvider/name": { + "count": 2 + }, "/networks/nodes/networkProvider": { "count": 2 }, "/networks/nodes/networkProvider/id": { "count": 2 }, + "/networks/nodes/networkProvider/name": { + "count": 2 + }, "/networks/nodes/location": { "count": 2 }, @@ -63,12 +69,18 @@ "/networks/spans/physicalInfrastructureProvider/id": { "count": 1 }, + "/networks/spans/physicalInfrastructureProvider/name": { + "count": 1 + }, "/networks/spans/networkProvider": { "count": 1 }, "/networks/spans/networkProvider/id": { "count": 1 }, + "/networks/spans/networkProvider/name": { + "count": 1 + }, "/networks/spans/route": { "count": 1 }, diff --git a/tests/fixtures/geojson_to_json/phases_1.expected.json b/tests/fixtures/geojson_to_json/phases_1.expected.json index 08ac39a..ae85171 100644 --- a/tests/fixtures/geojson_to_json/phases_1.expected.json +++ b/tests/fixtures/geojson_to_json/phases_1.expected.json @@ -10,7 +10,8 @@ "description": "Contract for the construction of phase 1 of the NextGen network.", "relatedPhases": [ { - "id": "1" + "id": "1", + "name": "NextGen Phase 1" } ] } @@ -20,7 +21,8 @@ "id": "1", "name": "Accra", "phase": { - "id": "1" + "id": "1", + "name": "NextGen Phase 1" }, "location": { "type": "Point", @@ -35,7 +37,8 @@ "name": "Kumasi", "status": "operational", "phase": { - "id": "1" + "id": "1", + "name": "NextGen Phase 1" }, "location": { "type": "Point", @@ -53,7 +56,8 @@ "start": "1", "end": "2", "phase": { - "id": "1" + "id": "1", + "name": "NextGen Phase 1" }, "route": { "type": "LineString", diff --git a/tests/fixtures/geojson_to_json/phases_1.meta.expected.json b/tests/fixtures/geojson_to_json/phases_1.meta.expected.json index 0a2b59b..7166692 100644 --- a/tests/fixtures/geojson_to_json/phases_1.meta.expected.json +++ b/tests/fixtures/geojson_to_json/phases_1.meta.expected.json @@ -27,6 +27,9 @@ "/networks/contracts/relatedPhases/id": { "count": 1 }, + "/networks/contracts/relatedPhases/name": { + "count": 1 + }, "/networks/nodes": { "count": 1 }, @@ -42,6 +45,9 @@ "/networks/nodes/phase/id": { "count": 2 }, + "/networks/nodes/phase/name": { + "count": 2 + }, "/networks/nodes/location": { "count": 2 }, @@ -75,6 +81,9 @@ "/networks/spans/phase/id": { "count": 1 }, + "/networks/spans/phase/name": { + "count": 1 + }, "/networks/spans/route": { "count": 1 }, diff --git a/tests/fixtures/geojson_to_json/phases_inconsistent_1.expected.json b/tests/fixtures/geojson_to_json/phases_inconsistent_1.expected.json index 08ac39a..ae85171 100644 --- a/tests/fixtures/geojson_to_json/phases_inconsistent_1.expected.json +++ b/tests/fixtures/geojson_to_json/phases_inconsistent_1.expected.json @@ -10,7 +10,8 @@ "description": "Contract for the construction of phase 1 of the NextGen network.", "relatedPhases": [ { - "id": "1" + "id": "1", + "name": "NextGen Phase 1" } ] } @@ -20,7 +21,8 @@ "id": "1", "name": "Accra", "phase": { - "id": "1" + "id": "1", + "name": "NextGen Phase 1" }, "location": { "type": "Point", @@ -35,7 +37,8 @@ "name": "Kumasi", "status": "operational", "phase": { - "id": "1" + "id": "1", + "name": "NextGen Phase 1" }, "location": { "type": "Point", @@ -53,7 +56,8 @@ "start": "1", "end": "2", "phase": { - "id": "1" + "id": "1", + "name": "NextGen Phase 1" }, "route": { "type": "LineString", diff --git a/tests/fixtures/geojson_to_json/phases_inconsistent_1.meta.expected.json b/tests/fixtures/geojson_to_json/phases_inconsistent_1.meta.expected.json index 7a9a9f5..a5a7dd4 100644 --- a/tests/fixtures/geojson_to_json/phases_inconsistent_1.meta.expected.json +++ b/tests/fixtures/geojson_to_json/phases_inconsistent_1.meta.expected.json @@ -27,6 +27,9 @@ "/networks/contracts/relatedPhases/id": { "count": 1 }, + "/networks/contracts/relatedPhases/name": { + "count": 1 + }, "/networks/nodes": { "count": 1 }, @@ -42,6 +45,9 @@ "/networks/nodes/phase/id": { "count": 2 }, + "/networks/nodes/phase/name": { + "count": 2 + }, "/networks/nodes/location": { "count": 2 }, @@ -75,6 +81,9 @@ "/networks/spans/phase/id": { "count": 1 }, + "/networks/spans/phase/name": { + "count": 1 + }, "/networks/spans/route": { "count": 1 },