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.

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

Ability to add custom headers and query params #153

Open
u382514 opened this issue Mar 30, 2022 · 2 comments
Open

Ability to add custom headers and query params #153

u382514 opened this issue Mar 30, 2022 · 2 comments

Comments

@u382514
Copy link

u382514 commented Mar 30, 2022

On the initial connect/upgrade we need the ability to add custom headers (think IAM/Sigv4 for Neptune or custom auth outside of basic) and the ability to add query params to the URL.

websocket::ClientBuilder has 'custom_headers' on the builder to do this and connect_async_with_tls_connector can this by supplying a full request.

Looks like adding query params can be completed by enhancing the websocket_url function.

Is there any reason we shouldn't go ahead with adding these features?

@wolf4ood
Copy link
Owner

Hi @u382514

yes i guess if we take additional parameter to the ConnectionOptions like additional headers/query params we can then pass them to the underlying ClientBuilder

In case i can help on this :)

@u382514
Copy link
Author

u382514 commented Mar 31, 2022

@wolf4ood I think we should go one step further if you agree (while still keeping everything backwards compatible). The ability to add a URL (url::Url) to the connection options which would parse out the URL given and any query params it has. We can then backfill the host, port, query, path and credentials in the connection options for information purposes. Of course everything will continue to be backwards compatible by simply leaving url set to None and utilizing host, port, query, path and credentials params as usual.

Notes:

  • We will continue to set the protocol based on SSL option and ignore the protocol in URL. That way if they put in something like HTTPS we'll change it to WSS/WS respectively.
  • The websocket crates seem to set the Host header during its build and will overwrite any Host header provided in the custom headers. Usually this isn't a problem, however anybody using bastions with forwarding will possibly have issues if they don't know that. We can make a note of that in the documentation.
  • Query params in most packages allow for duplicate keys so we'll have to account for that in the Query struct to make it consistent.
  • Headers will also be a custom struct with helper functions to convert since some packages use HeaderMap while others use Headers and even their own structs.
  • Would like to change the websocket_url function to parse via URL and return the uri from the results rather than the format! macro. We can take advantage of URLs error handling for bad input. I don't think this would break anything from a backwards compatibility standpoint.

Let me know your thoughts.

pub struct ConnectionOptions {
    pub(crate) url: Option<Url>, // new variable
    pub(crate) host: String,
    pub(crate) port: u16,
    pub(crate) path: String, // new variable
    pub(crate) query: Option<Query>, // a new struct with some helper functions due to conversions
    pub(crate) headers: Option<Headers>, // a new struct with some helper functions due to conversions
    pub(crate) pool_size: u32,
    pub(crate) credentials: Option<Credentials>,
    pub(crate) ssl: bool,
    pub(crate) tls_options: Option<TlsOptions>,
    pub(crate) serializer: GraphSON,
    pub(crate) deserializer: GraphSON,
}

... 

fn default() -> ConnectionOptions {
        ConnectionOptions {
            url: None, 
            host: String::from("localhost"),
            port: 8182,
            query: None,
            path: String::from("/gremlin"),
            headers: None,
            pool_size: 10,
            credentials: None,
            ssl: false,
            tls_options: None,
            serializer: GraphSON::V3,
            deserializer: GraphSON::V3,
        }
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants