8191: Data adapter - #3
Conversation
One record per data set in config/sources.yaml, DCAT-AP field names. SourceCatalog validates lazily and fails loudly on incomplete records.
Removes ENTER_MTM_SPATIALMAPS_HANDICAP_PARKING_SOURCE; the feed URL, CRS and model now come from the manifest entry, and the entity type and URN prefix both derive from its model field. Rewrote the test, which called a constructor signature that no longer existed and read a fixture file not in the repo.
rimi-itk
left a comment
rimi-itk
left a comment
There was a problem hiding this comment.
This looks very good. Some refactoring can make it even gooder. In general we should be careful not to add too much login in commands and create services that can handle the job.
| - **Containers are mandatory.** The template's PHP runs ahead of developer | ||
| hosts, so dependency management, console commands and tests cannot run | ||
| natively. Most likely source of first-run confusion. | ||
| - A web server, database and mail catcher are provisioned and never used. |
There was a problem hiding this comment.
We should remove any unused services.
There was a problem hiding this comment.
Agreed. Removed this statement.
| input declares its own CRS; reprojection happens at the boundary between reading | ||
| an input and building an entity, and nowhere else. | ||
|
|
||
| - **The specification is unambiguous.** RFC 7946 §4: "The coordinate reference |
There was a problem hiding this comment.
If we need ADRs we should write them ourselves. Much of the text makes no sense to me.
There was a problem hiding this comment.
I agree that they could be even shorter and more concise. I think that some of the considerations behind the decisions makes a stronger argument towards the decided architecture. People in the future may use this project as inspiration for working with brokers, so i think that the proper documentation earns its place.
| ``` shell | ||
| task import # list the available sources | ||
| task import -- mtm_spatialmaps-handicap-parking # import one | ||
| task import -- mtm_spatialmaps-handicap-parking --dry-run --limit 5 # print the payload instead | ||
| task broker:entities -- OnStreetParking 10 # read back what landed | ||
| ``` |
There was a problem hiding this comment.
I thinks we should tell how to run these commands with docker compose rather than with a task. We (currently) mostly use task for local development and for productions one should use docker compose (and we'll use itkdev-docker-compose-server, but that's our choice and caused by our hosting setup).
| task import # list the available sources | ||
| task import -- mtm_spatialmaps-handicap-parking # import one | ||
| task import -- mtm_spatialmaps-handicap-parking --dry-run --limit 5 # print the payload instead | ||
| task broker:entities -- OnStreetParking 10 # read back what landed |
There was a problem hiding this comment.
This task makes sense only during local development, right? I would remove it and add a curl example on how to talk to the broker (and then refer to reader to the NGSI-LD API documentation elsewhere).
There was a problem hiding this comment.
I have removed the broker:entities task for now, as it was kinda hacky.
I think the import tasks are pretty nifty when working with new datasets, so i want to keep them for now..
| * Parsed on first use rather than during container warm-up, so a malformed | ||
| * entry fails the import that needs it instead of every cache clear. |
There was a problem hiding this comment.
I think we should handle this as any other Symfony config, i.e. read and parse and validate it during container warm-up and define the sources a real config (cf. https://symfony.com/doc/current/components/config.html)
| // A JSON document may legally be a scalar. Every feed we consume is a | ||
| // list or an object, and a scalar here means the location is wrong | ||
| // rather than that the feed is empty. |
There was a problem hiding this comment.
We should handle HTTP errors in a better way (than just throwing \RuntimeExceptions) and maybe even report them back to the caller.
| use Symfony\Component\Console\Tester\CommandTester; | ||
| use Symfony\Component\HttpClient\MockHttpClient; | ||
|
|
||
| class ImportCommandTest extends TestCase |
There was a problem hiding this comment.
This should probably be refactored to be a DataSourceImporterTest, i.e. something the tests an import service rather than a command container too much logic.
Co-authored-by: Mikkel Ricky <rimi@aarhus.dk>
Co-authored-by: Mikkel Ricky <rimi@aarhus.dk>
Co-authored-by: Mikkel Ricky <rimi@aarhus.dk>
Co-authored-by: Mikkel Ricky <rimi@aarhus.dk>
Co-authored-by: Mikkel Ricky <rimi@aarhus.dk>
Co-authored-by: Mikkel Ricky <rimi@aarhus.dk>
Replaces the hand-written field checks with a config tree, and reads the whole manifest during container warm-up, so a malformed entry fails the build instead of the one import that happens to select it.
Catalog, Descriptor, Schema and Validator now sit together, which drops the repeated Source prefix. The cache warmer became Validator: it caches nothing, and exists so a malformed record fails the build. Schema no longer implements ConfigurationInterface — that is a bundle extension contract, not a description of a committed data file — so the catalogue hands the built tree to Processor::process() instead.
…nfig/sources.yaml
| /** | ||
| * Identifier used to select this source on the command line. | ||
| */ | ||
| public function key(): string; |
There was a problem hiding this comment.
I wonder if using https://symfony.com/doc/current/service_container/subscribers_locators.html#injecting-a-service-locator would be better suited for this, i.e. something like
arguments:
- !service_locator
mtm_spatialmaps-handicap-parking: '@App\Source\MtmSpatialMaps\HandicapParking'
…where we define the key in the config rather than in the class.
2 participants
Link to ticket
https://leantime.itkdev.dk/#/tickets/showTicket/8191
Description
Screenshot of the result
N/A
Checklist