Conversation
a02ec8e to
3244d7a
Compare
| return exact_match if exact_match | ||
|
|
||
| if route_matches.present? | ||
| exact_route_match || best_prefix_match |
There was a problem hiding this comment.
Could we end up up favouring a live root edition when there's a draft based on whatever other ordering we do here? Is that correct?
There was a problem hiding this comment.
question: could exact_route_match now return two matches, one from each content store?
21d7f42 to
e34f034
Compare
84a66bf to
6995f80
Compare
When introducing support for drafts, using the default draft edition factory will become an issue
This was missed in 7ece3e9
So that it doesn't conflict with the have_links matcher in the other dataloader's spec
We're working towards supporting draft content via GraphQL. It seems reasonable to have this scenario covered for the #live_content controller action, whether we choose to extend this endpoint or create a separate one.
The `live_edition` and `draft_edition` (alias `edition`) spec factories were both defaulting to a user_facing_version of 1. Since a document's live and draft editions are required to have different user_facing_versions, this new default of 2 enables a spec example to create both states of editions for a single document without introducing the noise of explicitly setting this attribute for one or both of them. We can rethink this if it begins to make other tests more awkward in future. Note: that the unpublished_edition factory needed updating here too. This was to prevent its default user_facing_version from changing since it inherits from the factory we've changed.
This replaces the content_store parameter. The behaviour of this dataloader is the same between the old default of `content_store: "live"` and the new default of `with_drafts: false`. `content_store: "draft"` won't behave like `with_drafts: true`, though, since live editions can now be included in a draft edition's links (if a more suitable draft version doesn't exist). In the near future the flag will be set in the query. For now we omit the argument when called in the link methods defined via `Types::BaseObject`, thereby defaulting to false. Co-authored-by: Ynda Jas <yndajas@gmail.com>
This replaces the content_store parameter. The behaviour of this dataloader is the same between the old default of `content_store: "live"` and the new default of `with_drafts: false`. `content_store: "draft"` won't behave like `with_drafts: true`, though, since live editions can now be included in a draft edition's links (if a more suitable draft version doesn't exist). In the near future the flag will be set in the query. For now we omit the argument when called in the link methods defined via `Types::BaseObject` and `Types::EditionQuery`, thereby defaulting to false. Co-authored-by: Ynda Jas <yndajas@gmail.com>
Draft editions can legitimately have null first_published_at and/or public_updated_at. They follow different rules, but I've found Edition::Timestamps and UpdateExistingDraftEdition to be useful reading on the topic
The `content_store` variable isn't currently being used and we're dropping support for it. Its replacement (`with_drafts`) is coming soon
As with the dataloader sources, this content_store setting was never used and it defaulted to "live". A query with `content_store: "live"` always returned a live edition or no edition at all. That's the same behaviour as `with_drafts: false`. A query with `content_store: "draft"` always returned a draft edition or no edition. `with_drafts: true` doesn't behave like that because it can return a draft edition or a live edition or no edition, with that order of precedence. In Content Store land, there's a draft content item (i.e. in Draft Content Store) for every published content item (i.e. in Live Content Store). This `with_drafts` fallback behaviour is intended to mimick the user-visible results of that. Unlike the dataloader sources, which have to consider a linked-to edition's state, QueryType looks up the root edition by base_path, which will only have 0 or 1 exact matches for each of `content_store: "live"` and `content_store: "draft"`.
Until recently, the dataloaders would only link editions together that had matching `content_store`s. Since introducing `with_drafts` to the dataloaders, we've been omitting the argument in the link methods defined in `Types::BaseObject` and thereby defaulting to `false`, only including live linked editions. Now, using the query's `with_drafts` setting in the dataloaders, when `with_drafts: true`, any draft edition can link to a live edition and vice versa, at any level of nesting in the generated response.
We're working towards supporting draft content via GraphQL using the `with_drafts` flag. Any GraphQL queries we generate in future should have support for this variable to allow drafts to be served.
We're working towards supporting draft content via GraphQL using the `with_drafts` flag. Including this variable in a query enables clients to set the variable and it ultimately ends up being used by the QueryType and dataloaders.
All existing uses of this class specify content_store: "live". Note: the line that changed in the controller action resulted in an entry in the Brakeman ignore file needing updating. I ran `brakeman -I` to remove the old version and introduce the new version.
This flag replaces the content_store parameter. With `content_store: "live"` this service always returned a live edition or no edition at all. That's the same behaviour as `with_drafts: false`. With `content_store: "draft"`, this service always returned a draft edition or no edition. `with_drafts: true` doesn't behave like that exactly, it can return a draft edition or a live edition or no edition, with that order of precedence. I don't think there's any need to compare a published edition with an unpublished edition at any point in this service. For base_paths that match edition records exactly, there can't be more than 1 record with that same base_path and `content_store: "live"`. So I believe we're expecting 0 or 1 draft editions and 0 or 1 live editions. This service then does its own sorting once it gets into the routes and redirects, so I *think* any more sorting by state or content_store precedence is moot. I'm hoping that the matching rules should take precedence over them, anyway, but it might be possible that we need to modify this code to group pairs of sibling draft and live editions? Co-authored-by: Ynda Jas <yndajas@gmail.com>
In a subsequent commit we'll introduce a draft content action which will duplicate most of the behaviour of the live content action, so this makes it easier to extend to other actions by wrapping most of the behaviour in shared examples
Which behaves a lot like the /graphql/content live content endpoint, only with `with_drafts: true`. I've left all the duplication here because it's tricky to see where to begin with that. If we settle on having 2 separate controller actions like this, then we can spend the time to make it easier and less error prone to work with. One consequence of the duplication is that it includes another file access Brakeman warning to ignore (as the code that we duplicated did). I've introduced a spec example here that demonstrates how this endpoint's use of with_drafts=true enables a live edition to link to a draft edition and vice versa. I've put it in its own separate integration spec because it seems less focussed on endpoint behaviour than the controller specs since it covers QueryType and dataloader behaviour. Co-authored-by: Ynda Jas <yndajas@gmail.com>
These tests provide us with reassurance that we're in line with Content Store other than in a few known scenarios. The new examples added by considering drafts all pass, suggesting there are no draft-specific divergences from Content Store (unless any of our existing ignore rules are masking novel diffs)
These tests provide us with reassurance that we're in line with Content Store other than in a few known scenarios. The new examples added by considering drafts all pass, suggesting there are no draft-specific divergences from Content Store (unless any of our existing ignore rules are masking novel diffs)
For the GraphQL link expansion integration tests, in CI we have one job for each of the four specs to speed things up. However, when introducing drafts the number of examples in each spec increases substantially, leading to each of the specs taking longer than the job for the main test suite, and about two minutes longer for the different content ID specs We can double down on the parallelisation by introducing the parallel_rspec gem, which by default runs four examples at a time (if the machine has enough cores - GitHub Actions does by default). This doesn't neatly divide the run time by four but it does bring the GraphQL link expansion integration test run times in line with or below the main test suite The main test suite fails when running examples in parallel, so we can't currently use this to speed up the overall CI run time In order to run the examples in parallel, the gem creates three additional databases. When doing this preparation it tries to drop the initial database, which fails if the initial database is using the default name of 'postgres' we think because the postgres server relies on this default-named database. Therefore for these jobs we use a non-default name for the initial database (the specific name of which doesn't matter beyond not being 'postgres')
6995f80 to
5162df3
Compare
| context "when with_drafts=false" do | ||
| it "doesn't return links to drafts" do | ||
| target_edition_1 = create(:draft_edition, title: "edition 1, draft") | ||
| target_document = create(:document) | ||
| target_edition_2 = create(:live_edition, title: "edition 2, live", document: target_document) | ||
| create(:draft_edition, title: "edition 3, draft", document: target_document) | ||
|
|
||
| source_edition = create(:live_edition, | ||
| links_kind => [ | ||
| { link_type: "test_link", target_content_id: target_edition_1.content_id }, | ||
| { link_type: "test_link", target_content_id: target_document.content_id }, | ||
| ]) | ||
|
|
||
| expected_titles = [target_edition_2.title] | ||
| expect(source_edition).to have_links("test_link").with_titles(expected_titles) | ||
| end | ||
| end |
There was a problem hiding this comment.
suggestion: the precedence aspect of this test might be clearer if separated from the other one, e.g. if the precedence was described as it "when drafts=false, it returns links to live editions despite drafts being available".
There was a problem hiding this comment.
Or maybe the precedence aspect of this test is already covered by defaults to including a (live) 'en' link if the locale-matching one is draft below.
| expect(target_edition).to have_reverse_links("test_link").with_titles(expected_titles) | ||
| end | ||
|
|
||
| it "returns reverse links to live editions when drafts aren't available" do |
nacnudus
left a comment
There was a problem hiding this comment.
It's patronising of me to praise this, but I will anyway.
There might be a corner case in the edition finder service, when there are editions in both content stores that don't have an exactly-matching base path, but do have an exactly-matching route. I don't understand why there would be such things, so I've left it as a question.
| return exact_match if exact_match | ||
|
|
||
| if route_matches.present? | ||
| exact_route_match || best_prefix_match |
There was a problem hiding this comment.
question: could exact_route_match now return two matches, one from each content store?
Introduces support for draft content in GraphQL. This involves changes to the GraphQL queries, the dataloaders and their SQL queries, the GraphQL controller, a few GraphQL types, and the edition finder service. Various refactors are included along the way. More detail in the individual commit messages
The draft content endpoint will not yet be exposed via GDS API Adapters. We need to think about authentication/authorisation and probably do some diffing (though the link expansion precedence tests introduced in #3876 and #3883 give us some confidence that we're broadly in line with Content Store for link expansion except in a few known scenarios). We'll also need to add support in frontend apps once supported by GDS API Adapters
Closes #3803