Use directives in graphql client #440
Conversation
|
Linting will fail until thoth/thoth-app/src/component/mod.rs Line 1 in 849756d |
Now cherry-picked on 57108eb |
| type Error = ThothError; | ||
|
|
||
| fn try_from(q: QueryConfiguration) -> ThothResult<Self> { | ||
| match q.specification { |
There was a problem hiding this comment.
This is neat, but we risk later updating one of the specifications so that it requires something omitted from the query, and then forgetting to update the corresponding parameters here.
For example, we might decide to add chapter data (where available) to one of the ONIX outputs, and not realise that the without_relations() toggle would mean no chapters were ever fetched. (Updating the unit tests would not catch this.)
It would be safer if we could also add something to the individual specification modules to encode what is/isn't being fetched here, and trigger some kind of error if the developer tried to access unfetched elements. (Optionally also if fetched elements were no longer being used, to keep the queries minimal 🙂 )
Is the actual query logged anywhere? That could help with debugging/give reassurance that the right parameters were being requested. Seemed a bit opaque when I tested it.
There was a problem hiding this comment.
Yes, that's a fair point. This change basically allows us to keep having a single struct that we can use (thoth_client::Work). The safer option you propose would entail having different queries, with associated structs, so that each specification depended on its particular one. This would indeed be preferred, but would require a major rewrite of both thoth-client and thoth-export-server...
There was a problem hiding this comment.
I wasn't thinking of going that far - more like adding some list of fetched/unfetched elements in to each of the modules which would break if not updated by hand, as a reminder to update the TryFrom. At the very least, obvious commenting on the use thoth_client:: statements. It's going to be far too easy to unwittingly break behaviour otherwise, with the TryFrom being subtle and separated from the main logic.
There was a problem hiding this comment.
It's not that different from before, the specification modules have always being agnostic to the query. The best option is to make use of the type system, creating separate queries with associated structs for each specification that needs to be different, when we have the time. For the time being, this is currently breaking the distribution of publisher files, so I'm going to merge it
| MetadataSpecification::CsvThoth(_) => match q.request { | ||
| SpecificationRequest::ByWork => Ok(QueryParameters::new().with_all()), | ||
| SpecificationRequest::ByPublisher => { | ||
| Ok(QueryParameters::new().with_all().without_relations()) |
There was a problem hiding this comment.
Will this always output a "relations" header for which all the cells are empty? To avoid possible confusion, could we output something like [omitted] in these cells (or just remove the header)?
There was a problem hiding this comment.
Yes, it does. It'd be great to flag it somehow, but this would require writing a separate CSV implementation just to handle it, which seems overkill
Fixes #438
thoth-clientqueries now accept parameters to toggle the inclusion of certain fragments (using limits:limit: 9999orlimit: 0)thoth-export-servernow live inthoth-export-server/src/specification_query.rsand can choose what parameters will be passed ontothoth-clientbased on the type of specification and wether the specification is being output for a single or multiple works