Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: make 302 the default status_code for redirect responses #2189

Merged
merged 15 commits into from
Sep 7, 2023
Merged

feat: make 302 the default status_code for redirect responses #2189

merged 15 commits into from
Sep 7, 2023

Conversation

cofin
Copy link
Member

@cofin cofin commented Aug 21, 2023

Fixes #2138

Pull Request Checklist

  • New code has 100% test coverage
  • (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR

Description

This PR will set the Redirect status code to 302 if it is left unset.

Close Issue(s)

Closes #2138

sfermigier and others added 5 commits August 20, 2023 10:43
@sfermigier @provinzkraut
Fix: better error message.
db39820
@gsakkis @provinzkraut
feat: Ensures deterministic ordering of handlers (#2152)
135b80e 
Ensure deterministic handler order
@abdulhaq-e @Goldziher @provinzkraut
feat: Support custom status code for openapi exceptions (#1949)
e75e584 
* Support custom status code for openapi exceptions

* add docs for custom exceptions in OpenAPI schemas

---------

Co-authored-by: Na'aman Hirschfeld <nhirschfeld@gmail.com>
@provinzkraut
docs(openapi): Fix stdlib link (#2177)
6d77c51 
doc(openapi): fix stdlib link

Signed-off-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
@cofin
Enhancement: make 302 the default status_code for Redirect
15d24c9 
Fixes #2138
@cofin cofin requested review from a team as code owners August 21, 2023 00:52
@cofin cofin changed the title Enhancement: make 302 the default status_code for Redirect feat: make 302 the default status_code for Redirect Aug 21, 2023
@cofin cofin changed the title feat: make 302 the default status_code for Redirect feat: make 302 the default status_code for redirect responses Aug 21, 2023
guacs
guacs reviewed Aug 21, 2023
View reviewed changes
Copy link
Member

@guacs guacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the default in ASGIRedirectResponse be changed as well?

Base automatically changed from v2.1 to main August 21, 2023 14:32
@cofin
Copy link
Member Author

cofin commented Aug 22, 2023

Shouldn't the default in ASGIRedirectResponse be changed as well?

I wasn't exactly sure here. I see this line in the ASGIRedirectResponse.__init__ method, which is why I didn't add initially:

status_code = status_code or HTTP_307_TEMPORARY_REDIRECT

@provinzkraut what do you think?

@provinzkraut
Copy link
Member

provinzkraut commented Aug 22, 2023 •
edited
Loading

Hmm. It might be a bit unexpected if the two different redirects available use different defaults for the status code. I do believe though that the 307 in the ASGIRedirectResponse is a bug, and it should be 302 as well.

But this raises the question whether this ought to be considered a breaking change. It'd be more of a breaking bugfix really 🤔

@guacs
Copy link
Member

guacs commented Aug 23, 2023

I agree that having different defaults for the different redirects seems a bit confusing.

On the topic of the breaking change, it seems this change is the one that will actually document that the default is 302, and so the previous use of 307 was undocumented and could be considered an implementation detail. I don't know...I may be stretching this a bit too much :P

@cofin
Copy link
Member Author

cofin commented Aug 23, 2023

I agree that having different defaults for the different redirects seems a bit confusing.

On the topic of the breaking change, it seems this change is the one that will actually document that the default is 302, and so the previous use of 307 was undocumented and could be considered an implementation detail. I don't know...I may be stretching this a bit too much :P

Well, it is true. The original default was undocumented. However, i think it could be considered breaking if anyone's existing code is set to look for the 307 instead of the 302.

I'm fine with any approach y'all are good with on this.

@provinzkraut
Copy link
Member

However, i think it could be considered breaking if anyone's existing code is set to look for the 307 instead of the 302.

I think we should treat this as such, which means this would have to be postponed to the next major version.

@guacs
Copy link
Member

guacs commented Aug 23, 2023

Then maybe we should document that the current default is 307?

@cofin
Copy link
Member Author

cofin commented Aug 23, 2023

So how do we handle these types of changes, do we need to create the next major version branch now?

@provinzkraut
Copy link
Member

So how do we handle these types of changes, do we need to create the next major version branch now?

Well.. I don't really want to have another branch like that for a year. I'd say we just put this into the 3.0 milestone and then address it when the time comes?

@guacs
Copy link
Member

guacs commented Aug 23, 2023

So how do we handle these types of changes, do we need to create the next major version branch now?

Well.. I don't really want to have another branch like that for a year. I'd say we just put this into the 3.0 milestone and then address it when the time comes?

Does that mean this PR and the associated bug has to be kept open as well?

@provinzkraut
Copy link
Member

Does that mean this PR and the associated bug has to be kept open as well?

I'd close the PR but keep the issue. Wdyt?

@abdulhaq-e
Copy link
Member

Merge + feature flag is a good combo imo.

@provinzkraut
Copy link
Member

Merge + feature flag is a good combo imo.

The issue I see with that is that the feature flag implementation would require more code than the actual change. It's a really trivial change, so imo we should just postpone it.

Goldziher
Goldziher reviewed Aug 24, 2023
View reviewed changes
docs/usage/responses.rst Outdated Show resolved Hide resolved
@cofin @Goldziher
Update docs/usage/responses.rst
b508568 
Co-authored-by: Na'aman Hirschfeld <nhirschfeld@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented Aug 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@github-actions
Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/2189

@Goldziher Goldziher merged commit 3b0becb into main Sep 7, 2023
15 of 16 checks passed
@Goldziher Goldziher deleted the cofin/issue2138 branch September 7, 2023 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guacs guacs guacs left review comments

@Goldziher Goldziher Goldziher approved these changes

@JacobCoffee JacobCoffee JacobCoffee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancement: make 302 the default status_code for Redirect
8 participants
@cofin @provinzkraut @guacs @abdulhaq-e @Goldziher @JacobCoffee @sfermigier @gsakkis