-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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.
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
refactor: Removes the deprecated redirect endpoint #26377
refactor: Removes the deprecated redirect endpoint #26377
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #26377 +/- ##
==========================================
- Coverage 69.57% 69.56% -0.01%
==========================================
Files 1893 1892 -1
Lines 74220 74162 -58
Branches 8263 8263
==========================================
- Hits 51635 51592 -43
+ Misses 20504 20489 -15
Partials 2081 2081
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michael-s-molina although the redirect endpoint is deprecated and the functionality for creating /r
links no longer exists, I wonder if we should try to preserve these URLs (for some period) by:
- Migrate the records from the
url
to thekey_value
table where theresource
column isredirect
and thevalue
column is a JSON key/value pair mappingurl.id
tourl.url
. These records could have an expiration of 1–2 years given the somewhat transient nature of URLs. - Keep the
/r
link endpoint which—albeit inefficiently—scans thekey_value
table (by leveraging JSON SQL UDFs) where theresource
column isredirect
and thevalue
column contains the/r
ID as they key. - Remove the deprecated RESTful HTTP endpoints in a far future version.
Alternatively, if code bloat is less of a concern, we could maybe punt this PR (as is) until v6.0 (or later) given the increased likelihood that the legacy redirects would be unused.
Note both options suffer from the same achilles heel in the sense that there’s no guarantee that an institution upgrade cadence will reflect that of the release cycle, hence users may feel short changed when the redirects no longer exist.
@@ -112,6 +112,7 @@ def test_delete_tags_command(self): | |||
TaggedObject.object_id == example_dashboard.id, | |||
Tag.type == TagType.custom, | |||
) | |||
.order_by(Tag.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this change have to do with the redirect logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing. It was something I caught while running the tests. It's a flaky test given that order is not guaranteed.
If we're going to keep the |
I don't have a strong opinion on whether to keep the urls, but I did notice that it doesn't look like the endpoint was ever marked as deprecated. Not a blocker but something to keep in mind for future. |
@eschutho I updated the PR description with more context. Let me know if that's sufficient to resolve your concern. |
Nevermind, found it :) Didn't realize it was just /r/id |
d7a8417
to
4fb63fc
Compare
|
||
|
||
def upgrade(): | ||
module.downgrade() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting approach
SUMMARY
As part of the 4.0 approved initiatives, this PR removes the deprecated Redirect API that supported short URLs (
/r
) and theurl
metadata table used to store them that was used before the permalink feature. Users lost the ability to generate R links ~1.5 years ago which seems sufficient time to remove the endpoint. It looks like we forgot to officially deprecate the endpoint inconfig.py
when working on 3.0 but #19078 has the following message inUPDATING.md
which is more relevant from a communication standpoint as opposed to the# deprecated
comment inconfig.py
:TESTING INSTRUCTIONS
CI should be sufficient for merging this PR. We'll do a complete testing of 4.0 after all approved proposals are merged.
ADDITIONAL INFORMATION