-
Notifications
You must be signed in to change notification settings - Fork 106
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
[AUD-1375] Prepare User and Track APIs for Type Generation #2778
Conversation
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.
Thanks for doing this!
Some notes:
- I don't think we want to drop the trending version everywhere - as it is used when migrating default trending versions for a consistent user experience
- One things that's not consistent everywhere is the responses in the ns.doc - in some places we have it and in other we don't (Note that it's not too useful even when we have it bc it's the same everywhere)
- Since user_id was converted to id in the url params, do we want to update track_id to id in its respective route param for consistency? I don't it doesn't conflict so I'm fine either way
class DescriptiveArgument(reqparse.Argument): | ||
""" | ||
A version of reqparse.Argument that takes an additional "description" param. | ||
The "description" is used in the Swagger JSON generation and takes priority over "help". |
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.
nice!
|
||
|
||
@full_ns.route( | ||
"/recommended", | ||
defaults={"version": DEFAULT_TRENDING_VERSIONS[TrendingType.TRACKS].name}, | ||
strict_slashes=False, | ||
) | ||
@full_ns.route("/recommended/<string:version>") | ||
class FullRecommendedTrack(Resource): | ||
class FullRecommendedTracks(Resource): |
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.
I don't think we want to drop the ability to specify the version - it should help with migrations of default trending versions
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.
I added FullRecommendedTracksWithVersion
to replace this route here which the same thing. It needs to be separate so that they can have different operation IDs (we can't have two routes with the same ID)
@@ -838,7 +824,7 @@ def get(self, user_id): | |||
) | |||
|
|||
|
|||
@full_ns.route("/content_node/<string:replica_type>") | |||
@full_ns.route("/content_node/<string:replica_type>", doc=False) |
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.
@jowlee Thoughts on leaving this out of the typegen/documentation? (and/or any other endpoints that might be worth hiding?)
(addressed inline - it shouldn't be missing, just moved)
Yeah I noticed this too - should we just remove it? I don't think it's adding any value...
No strong feelings here, I left it to reduce footprint of this change but could easily go the other way. I would have preferred to keep them |
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.
Re: Added caching in some places, double check that's ok?
- do we know which endpoints we added caching for? Would like to double check this is safe
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.
Added comments where cache was added
Removed added caching
Description
Preview API docs: http://34.66.46.158:4567/#audius-api-docs
Users
<string:user_id>
to<string:id>
in the route params for users so that it doesn't conflict with theuser_id
query param and cause problems with the swagger.json outputcurrent_user_parser
,pagination_parser
, andpagination_with_current_user_parser
which can be used between different responses so we don't have to keep redefining the same parameters, and added help text explaining what the parameters areTracks
expect
/by_id
and the "explore" endpoints. I have some hot takes here, mainly as the explore endpoints are dependent on the user:/by_id
endpoint, we should update the root/tracks
endpoint to take IDstracks/under_the_radar
isn't actually "Under the Radar" (yet) it's just a feed. It should be/my/feed
* or/users/{id}/followees/activity
(and be authed?)/my/smart-playlists/under-the-radar
*tracks/best_new_releases
should be/my/smart-playlists/new-releases
* (and be authed?)tracks/most_loved
should be/my/smart-playlists/most-loved
* (and be authed?)*For all of these, could also go for
/users/{id}
instead of/my
or even just dropping themy
, but I do like the idea of making it authed (no need to allow users to see each other's unless shared?) and grouping the smart playlists together. Open to ideas, but I don't think they should be on the root/tracks
route and I prefer hyphens for routes over snake casingNotes/Qs/Callouts
Limits are defaulted differently on some endpoints. Can we standardize? If not, I can do a pass and override as necessary.Did a pass and ensured default limits stay the same.Added caching in some places, double check that's ok?Removed this@record_metrics
?copy()
s but wasn't sure if it was necessary. Might still be good to have for future maintenance?Get Track
(v1/full) still usesget_unlisted_track
. When is Marcus gonna clean this up??? 👀@record_metrics
@api.doc
@api.expect
@api.marshal_with
@cache
Tests
❗ There should be no functional changes from this PR ❗ - Everything I touched should only impact documentation. If you think something I did will affect functionality, let me know so I can make sure it gets tested. I do want to avoid testing each API individually 😨
How will this change be monitored? Are there sufficient logs?