Add see_other? method to GdsApi::Response #1322
Conversation
KludgeKML
changed the title
- We want to know (in frontend apps) whether a returned content item is the content item we originally asked for, or a redirect. content-store only returns 303 see_other redirects. - 301s and 302s might be returned from systems in front of content store, (for instance the content item at "/" - the homepage - currently has a 301 in its history), but we shouldn't handle those yet, so we don't need a more general redirect method.
KludgeKML
force-pushed
the
add-redirect-query
branch
from
5b7f6f5 to
3b96973
Compare
KludgeKML
changed the title
|
Code change looks good - though does seem a little non-exhaustive. Though overall, we tend to deal with the redirects at the router side and intend for content store ones to be transparent, so could we dig into a bit more of the problem you're facing to validate whether this is the most appropriate route? |
|
Main problem we're looking at solving is that with some prefix routes we get redirects for non-valid paths that end up being rendered by a different controller. (so for instance, asking for /government/get-involved/take-part/blah should really return a 404, but content-store gives us a 303 to /government/get-involved which is then hidden by the adaptor following the redirect - we're routing based on the initial path, not the redirected path, so we end up trying to render the get-involved content item using the take-part controller, which ends up in a crash.) This isn't really a security issue, but it does mean we get a lot of sentry alerts instead of ignorable 404s. We could do what content-store does internally, and just say "does the base path we got from content store match the path we asked for?", but then we get stuck with what we should do with that (I personally think we should be 404-ing these rather than redirecting them to get-involved, but content-store seems to sort of want us to redirect them). |
|
Sure, I can see the challenge there - and would agree about them 404ing. So I guess the intention of the original /government/get-involved prefix route was so that all of the 404 handling and stuff could happen in app without the content item - as I I guess it was just a way of getting the request to Whitehall frontend which then matched the actual route or Whitehall returned a 404. What I'd first question is do we still actually need a prefix route for /government/get-involved. It seems like something that made sense when we had all the take part pages rendered by Whitehall but these all seem to have individual content items now (e.g: https://www.gov.uk/api/content/government/get-involved/take-part/national-citizen-service) |
|
Ah, so maybe we can forget about this PR, and just do this: alphagov/whitehall#9908 ? |
|
Yeah I think that'd approach would be more in tune with GOV.UK's precedent for this type of handling. The challenge though is to have confidence that it's not going to break any URIs that relied on the prefix route - though hopefully that isn't too hard to work out from the rendering app? I'll review your other PR on the assumption you've validated that. |
|
Closing in favour of this: alphagov/whitehall#9908 |
To correctly handle some responses from content store, frontend needs to know whether the returned response from content store included following a 303 ("see other") redirect (in which case frontend should also redirect). Content store only returns 303s, so we add a
see_other?method to the GdsApi::Response method which queries the underlying model's history array (if there's anything with a 303 in there, we have followed a see_other redirect).We don't want to handle other types of redirect yet (they're probably already okay to ignore), so make this specific to 303.
This repo is owned by the publishing platform team. Please let us know in #govuk-publishing-platform when you raise any PRs.