-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
feat(lavalink)!: Lavalink refactor to v4 API #2322
base: main
Are you sure you want to change the base?
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.
Please remove all unwraps that are not provably unreachable or document those functions as panicking on . I would also like struct fields and enum variants to be alphabetically sorted everywhere.
Lastly, if it's not too much work, it would be great to complete the Lavalink v4 support in one go.
So for the panics, you just mean send them up as a result correct? Sorry for the structs change. I was following the same order as the lavalink documentation to ensure i had all the new fields. I can revert those to alphabetical order. Didn't catch that before. I personally don't have the time currently to finish the v4 implementation. I can maybe work on that at a later time but constrained at the moment. It will just take quite some time to verify the changes is the problem. The missing pieces are really just the extra stuff such as lavalink version, more filters (i haven't needed or used them), session info which isn't required, and i think route planning which i haven't needed either and quite frankly not sure how that all works. Unless someone else can help i won't be able to get that together. |
Yes
That's okay, We can defer the v4 additions to another PR. |
@vilgotf I think I have address all the open items. Please let me know if i missed anything or you want further changes. I appreciate the feedback! Also what is up with the book and docs jobs failing? I didn't touch any of the other ones and it seems to be broken. Not sure if there is a fix for these or not? |
The book stuff is maybe fixed by #2326, I don't really know whats up with the docs though. |
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.
Looks better, but there's still some ways to go before landing this.
/// The guild ID of the player. | ||
pub guild_id: Id<GuildMarker>, | ||
/// Op code for this websocket event. | ||
pub op: Opcode, |
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 believe this should be an associated constant rather than part of the deserialized payload. Also, you can tag the IncomingEvent
enum based on this field, which is what you appear to desire.
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.
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.
Not sure i follow what you want here. See the example payload in the tests in the model.rs
:
r#"{"op":"playerUpdate","guildId":"987654321","state":{"time":1710214147839,"position":534,"connected":true,"ping":0}}"#,
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'll fix this in a follow up PR
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 believe this should be done as part of this PR, don't think a follow-up would be merged any time soon seeing how we're progressing on this one...
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 so much for your work on this and sorry for the long wait. I pushed some changes (hope you don't mind) to help get this closer to the finish line. I don't expect to request any more changes after this.
fn connect_request(state: &NodeConfig) -> Result<ClientBuilder, NodeError> { | ||
let mut builder = ClientBuilder::new() | ||
.uri(&format!("ws://{}", state.address)) | ||
.uri(&format!("ws://{}/v4/websocket", state.address)) |
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.
Since the crate supports TLS, this should use wss
conditionally. Considering you did not add TLS support for the HTTP part, I assume all of that is entirely untested if you left this as-is? Perhaps just remove TLS?
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.
see other comment about http2 support. Clarify that and I can then address this.
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.
@Gelbpunkt I looked into this a bit more. I was trying to enable tls support but this uri isn't allowed to be tls in the twilight-http. Should we just drop this implementation use and use the hyper client ourselves here?
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.
How is twilight-http relevant at all here? This is the websocket connection part. Generally speaking though I think you won't be able to avoid forcing the user to specify whether TLS should be used for a particular node
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.
The ClientBuilder is a struct used within the twilight http crate. I don't know why we have the socket connection using this crate and not using the hyper builder directly.
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.
No, that's a tokio-websockets
struct
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.
ah sorry for the confusion... missed that. I was using my editor to find the definition and it matched the name of the twilight-http. Didn't double check the scope of it.
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.
Alright so here is the deal. I did the rebase but the docs broke and can't reproduce locally. Also I have no idea how to make the TLS happen here. I am trying and running into all sorts of errors.
I get corrupted messages on the twilight side and and a bunch of invalid strings from the lavalink side. It is as if the websocket isn't upgrading to TLS correctly. I can't just simply change from ws to wss is what I am saying. I don't have time to debug this issue. I would like to make this fix later if possible as a issue ticket.
I think the prior behavior of this client was just on the ws not the upgraded TLS socket based on the diff.
.uri(&format!("ws://{}", state.address))
How would you like to proceed? I need help if we want this change in this PR.
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.
Please add a per-node parameter that toggles the use of TLS
@@ -488,15 +509,18 @@ impl Connection { | |||
|
|||
let (to_node, from_lavalink) = mpsc::unbounded_channel(); | |||
let (to_lavalink, from_node) = mpsc::unbounded_channel(); | |||
let lavalink_http = HyperClient::builder(TokioExecutor::new()).build_http(); |
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.
Why keep TLS features and then make this plaintext-only? Also, why is there a HTTP2 feature? HTTP2 requires negotiation in the TLS handshake or prior knowledge, neither of which I see you doing
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.
This was a change that @vilgotf did. That is why I was so confused what you were referring to.
Before:
let lavalink_http = HyperClient::builder(TokioExecutor::new())
.http2_only(cfg!(feature = "http2"))
.build_http();
Please tell me what we want to do here. I would like to close this PR soon.
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.
This is enabled in the feature flags. The legacy client isn't clear but it is enabled here: https://docs.rs/hyper-util/latest/hyper_util/client/legacy/struct.Builder.html
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.
So this is still open but hinges on how to proceed.
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.
As I mention earlier, please add a TLS parameter to the Lavalink node that determines whether TLS (and therefore HTTP2) should be used.
The only major issue I have remaining is TLS and HTTP2. Either we support it, or we don't. But having TLS features for websockets which are unused, and HTTP2 features for HTTP which are unused is a no go. My wild guess would be that 99% of people run lavalink without TLS, so we can just remove the features? On the other hand, TLS support is perhaps like 10 lines of code, so it might be best to keep it (and properly implement it!). |
@vilgotf thank you for the help on updating the branch. Been super busy so haven't had a chance to respond here. I resolved your issues I believe. @Gelbpunkt Can you be a bit more specific? I don't follow the comments. I think some of the confusion is from @vilgotf changes that were done. Please clarify how i should proceed. I would really like to close the PR out soon. |
@vilgotf and @Gelbpunkt I know it has been a bit but would like to close this. I don't have much time lately so haven't made any progress. What needs to happen to close this? I tried to change the ws to wss as suggested and that is a bigger change than we think. That is using the The deadline for v3 support is coming to a close soon (November 1, 2024) |
I think the best way forward now is to rebase the PR and look over all pending, unresolved review comments to see which ones still apply or need clarification. I haven't replied to all of them in time in the past, but I'm happy to revisit them now to get this merged. Regarding TLS, twilight-http plays no part in the websocket connection. You will need to add a parameter at node creation for whether TLS should be used in order to decide the scheme to use for connecting. Thanks again for trying to get this merged, it's a big PR which makes this a bit more troublesome for everyone involved, but I think we're approaching the goal soon. |
… the version 4 api. Fixed the example for now and updated tests
…amed to lavalink-protocol-http2.
…e in the connection object to avoid continuously connecting / creating the client.
Updating some grammar errors, formatting, spelling, and some code reduction.
…ttp on lavalink connection
…eature flag `http-support`
Authorization is to be redacted. In this case, the request is not that interesting so the entire event is removed
Added the ability to either include the identifier or the encoded track on and update. Each let you do different things for the server.
… basic example for a bit more documentation
This reverts commit 1aa6b9b.
18d981e
to
9d7ad7b
Compare
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.
Alright so I took a look at getting the the websocket to use tls. This is not a straight forward change. The previous behavior is to us the non tls socket. So no behavioral changes happen. I would like to just make an issue ticket and move on with this. I know this isn't ideal but the leg work to get the upgraded socket is not straight forward an I do not have time.
I did as suggested and rebased. Everything seems to still be working in my bot that I am testing on. I resolved all the open issues that weren't resolved that should be closed now.
Please have another look. Thank you so much for the reviews and help! Sorry for being pushy and trying to close this off. Just limited on time and testing this is a bit of a setup and pain.
FYI: I don't know why the doc is failing. I can't reproduce locally on my branch.
fn connect_request(state: &NodeConfig) -> Result<ClientBuilder, NodeError> { | ||
let mut builder = ClientBuilder::new() | ||
.uri(&format!("ws://{}", state.address)) | ||
.uri(&format!("ws://{}/v4/websocket", state.address)) |
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.
Alright so here is the deal. I did the rebase but the docs broke and can't reproduce locally. Also I have no idea how to make the TLS happen here. I am trying and running into all sorts of errors.
I get corrupted messages on the twilight side and and a bunch of invalid strings from the lavalink side. It is as if the websocket isn't upgrading to TLS correctly. I can't just simply change from ws to wss is what I am saying. I don't have time to debug this issue. I would like to make this fix later if possible as a issue ticket.
I think the prior behavior of this client was just on the ws not the upgraded TLS socket based on the diff.
.uri(&format!("ws://{}", state.address))
How would you like to proceed? I need help if we want this change in this PR.
@@ -488,15 +509,18 @@ impl Connection { | |||
|
|||
let (to_node, from_lavalink) = mpsc::unbounded_channel(); | |||
let (to_lavalink, from_node) = mpsc::unbounded_channel(); | |||
let lavalink_http = HyperClient::builder(TokioExecutor::new()).build_http(); |
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.
So this is still open but hinges on how to proceed.
@@ -53,6 +57,7 @@ async fn main() -> anyhow::Result<()> { | |||
let http = HttpClient::new(token.clone()); | |||
let user_id = http.current_user().await?.model().await?.id; | |||
|
|||
// The client is [`Lavalink`](crate::client::Lavalink) that forwards the required events from Discord. |
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.
It's nice that you are trying to document what is going on here, but this is not a doc comment and won't get rendered anywhere. I'd try and rephrase it as a comment rather than documentation
// We read the [Voice State and Voice Server Updates](https://discord.com/developers/docs/topics/gateway-events#voice) from discord to format the data to send to a Lavalink `VoiceUpdate` Event. | ||
// There is a lower level [node](crate::node) that processes this for you. It isn't recommended to use this but rather the lavalink struct with the players. If you don't find functionality please open up and issue to expose what you need. | ||
// See the command functions for where we use the players. |
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.
same here
default = ["http-support", "rustls-native-roots"] | ||
http-support = ["dep:percent-encoding"] | ||
default = ["http2", "rustls-native-roots"] | ||
http2 = ["hyper/http2", "hyper-util/http2"] |
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.
HTTP2 as a separate feature makes little sense because it is pointless without prior knowledge, which I assume is virtually never the case. I'd add this implicitly to the TLS features.
Currently some [Filters](crate::model::outgoing::Filters) are not yet supported. | ||
Some endpoints such as [Lavalink Info] and [Update Session] have also not yet | ||
been implemented. Please reach out and open an issue for any missing feature you | ||
would like to use. The Lavalink V4 port did not add support for any new features | ||
not previously found in V3. |
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 feel like this is supposed to go inside the changelog, not the README
/// Information about a playlist from a search result. | ||
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] | ||
#[non_exhaustive] | ||
#[serde(rename_all = "camelCase")] | ||
pub struct PlaylistInfo { | ||
/// The name of the playlist, if available. | ||
pub name: Option<String>, | ||
/// The name of the playlist |
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.
/// The name of the playlist | |
/// The name of the playlist. |
@@ -488,15 +509,18 @@ impl Connection { | |||
|
|||
let (to_node, from_lavalink) = mpsc::unbounded_channel(); | |||
let (to_lavalink, from_node) = mpsc::unbounded_channel(); | |||
let lavalink_http = HyperClient::builder(TokioExecutor::new()).build_http(); |
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.
As I mention earlier, please add a TLS parameter to the Lavalink node that determines whether TLS (and therefore HTTP2) should be used.
}; | ||
} | ||
|
||
tracing::error!("no session id is found. Session id should have been provided from the websocket connection already."); |
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.
tracing::error!("no session id is found. Session id should have been provided from the websocket connection already."); | |
tracing::error!("no session id found"); |
let (method, url) = self.get_outgoing_endpoint_based_on_event(&outgoing)?; | ||
let payload = serde_json::to_string(&outgoing).expect("serialization cannot fail"); | ||
|
||
let authority = url.authority().expect("Authority comes from endpoint"); |
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.
let authority = url.authority().expect("Authority comes from endpoint"); | |
let authority = url.authority().expect("authority comes from endpoint"); |
fn connect_request(state: &NodeConfig) -> Result<ClientBuilder, NodeError> { | ||
let mut builder = ClientBuilder::new() | ||
.uri(&format!("ws://{}", state.address)) | ||
.uri(&format!("ws://{}/v4/websocket", state.address)) |
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.
Please add a per-node parameter that toggles the use of TLS
@@ -652,9 +752,14 @@ async fn reconnect( | |||
"key": config.address, | |||
"timeout": resume.timeout, | |||
}); | |||
let msg = Message::text(serde_json::to_string(&payload).unwrap()); | |||
let msg = Message::text( | |||
serde_json::to_string(&payload).expect("Serialize can't panic here."), |
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.
serde_json::to_string(&payload).expect("Serialize can't panic here."), | |
serde_json::to_string(&payload).expect("serialization cannot fail"), |
I appreciate the feedback @Gelbpunkt. If this TLS / HTTP2 is a deal breaker then I am going to close this PR. I don't have time to make this work. As i stated, that isn't supported right now and this is just a port over to v4 api. The TLS issue is a feature request in my opinion. I simply don't have time to develop and test this huge change. It isn't just a simple switch URL and be done. Someone else can come through and pick that up. I won't close the branch on my fork so anyone should be able to clone that and use this as a starting point. |
Yes, it is exactly just that. If you're keen on dropping the PR I can implement it as well, it's not a big deal. |
If you have time to help implement that then please do. Once you get it to a point you are happy, let me know and I can give it a test on my bot. More than happy to have you just push to this branch. I can run through the changes once we get to a good point. |
Here are the changes required to work against the new lavalink API: https://lavalink.dev/api/index.html.
I didn't fully implement all the API since some of it is new in
v4
. I only maintained the compatibility of the current system that was built onv3
. I put the structs / enums under deserialization and serialization tests with json i received when testing the bot and with some in the documentation. I also tested against my bot and seems to work just as well as before.Major changes were:
/v4/...
There are a few other things i want to change but they aren't critical. I want to clean up the http since it has some model structs that could make more sense in model. The http also just generates the body and parts for the http requests for some endpoints. I think the player needs to hold the client and make these calls for the user. There is too much detail exposed in my opinion. That will be a different day and effort but wanting to jot that down now.
Please let me know what I need to change and can do those and test with the basic bot. I updated the documentation with what i thought was relevant.
This covers the issue: #2192