Add support for HTTP 307/308 redirect status codes #14092
Conversation
| @@ -53,6 +53,16 @@ class RedirectController { | |||
| redirect(action:"${prefix}oo") | |||
| } | |||
|
|
|||
| def toActionTemporaryRedirect() { | |||
| request.setAttribute('statusConfig', [tempRedirect: true]) | |||
There was a problem hiding this comment.
The existing code seems to add a dedicated parameter for these purposes. Can you please update your PR to do so? i.e. ARGUMENT_PERMANENT - there should be one for ARGUMENT_TEMPORARY
|
@jdaugherty I've updated the implementation as suggested to use a dedicated argument instead of request attributes. Now the implementation changes:
// For 307 Temporary Redirect // For 308 Permanent Redirect |
|
@dakshmehta007 Can you please also submit a pull request to update the documentation? (https://github.com/apache/grails-doc/blob/7.0.x/src/en/ref/Controllers/redirect.adoc) |
| @@ -39,6 +39,7 @@ class ResponseRedirector { | |||
|
|
|||
| public static final String ARGUMENT_PERMANENT = "permanent" | |||
| public static final String ARGUMENT_ABSOLUTE = "absolute" | |||
| public static final String ARGUMENT_TEMPORARY = "temporary" // Add new constant | |||
There was a problem hiding this comment.
Drop the comment to be consistent with the current code
| @@ -104,13 +105,13 @@ class ResponseRedirector { | |||
| namedParameters.put(LinkGenerator.ATTRIBUTE_PARAMS, configuredParams + webRequest.originalParams) | |||
| } | |||
| } | |||
| redirectResponse(linkGenerator.getServerBaseURL(), linkGenerator.link(namedParameters), request, response, permanent, absolute) | |||
| redirectResponse(linkGenerator.getServerBaseURL(), linkGenerator.link(namedParameters), request, response, permanent, absolute, arguments) | |||
There was a problem hiding this comment.
Lets match the existing style here and not pass the map?
| int status = permanent ? HttpServletResponse.SC_MOVED_PERMANENTLY : HttpServletResponse.SC_MOVED_TEMPORARILY | ||
|
|
||
| // Update to use arguments instead of request attributes | ||
| boolean temporary = Boolean.TRUE == arguments.get(ARGUMENT_TEMPORARY) |
There was a problem hiding this comment.
This may be a string value (like permanent). Can you please parse this in the prior method to be consistent and handle the same edge cases as permanent.
| boolean temporary = Boolean.TRUE == arguments.get(ARGUMENT_TEMPORARY) | ||
| int status | ||
| if (permanent) { | ||
| status = temporary ? HttpServletResponse.SC_PERMANENT_REDIRECT : HttpServletResponse.SC_MOVED_PERMANENTLY |
There was a problem hiding this comment.
Instead of the name temporary, wouldn't 'moved' be a better name? Where permanent decides between temporary/ permanent and moved decides between the MOVED value or not.
|
@jdaugherty, here is the pull request you requested (apache/grails-doc#966) in the grails-doc repository. |
Added support for HTTP 307/308 status codes in the ResponseRedirector by:
Fixes #11625
Extending the redirect logic to check for a
tempRedirectflag in the statusConfigUsing appropriate status codes based on permanent + tempRedirect flags:
Added test cases to verify both temporary (307) and permanent (308) redirect behavior
Usage##