Fix url handling #81
Fix url handling #81
Conversation
| config: self.0, | ||
| deepgram: self.0, |
There was a problem hiding this comment.
I changed this because I was getting confused about what a config is vs an options. It's a strictly internal change. Not visible to users.
…uble-escaping problem with a failing test.
jcdyer
force-pushed
the
fix-url-handling
branch
3 times, most recently
from
2e8b8cf to
e49900e
Compare
| /// # let mut need_token = std::env::var("DEEPGRAM_API_TOKEN").is_err(); | ||
| /// # if need_token { | ||
| /// # std::env::set_var("DEEPGRAM_API_TOKEN", "abc") | ||
| /// # } |
There was a problem hiding this comment.
I don't love this pattern. The #s ensure it doesn't show up in the docs, but it's still kind of ugly.
There was a problem hiding this comment.
What is the intent of those lines?
There was a problem hiding this comment.
I had a hard time fixing these imports in the examples in VSCode when they would break the build since there is no auto complete for them.
| // Add standard pre-recorded options | ||
| if let Some(options) = &self.builder.options { | ||
| let query_string = options_to_query_string(options); | ||
| let query_pairs: Vec<(Cow<str>, Cow<str>)> = query_string | ||
| .split('&') | ||
| .map(|s| { | ||
| let mut split = s.splitn(2, '='); | ||
| ( | ||
| Cow::from(split.next().unwrap_or_default()), | ||
| Cow::from(split.next().unwrap_or_default()), | ||
| ) | ||
| }) | ||
| .collect(); | ||
|
|
||
| for (key, value) in query_pairs { | ||
| pairs.append_pair(&key, &value); | ||
| } | ||
| } |
There was a problem hiding this comment.
This was the original logic which double-quoted inputs. So if the original option contained an &, L257 would quote it as %26 (as it should be), and then after L270, the % in %26 would get quoted to %25, and the final result would be %2526. The new logic unquotes parameters instead of just splitting on separators.
| &'a self, | ||
| options: Option<&'a Options>, | ||
| ) -> StreamRequestBuilder<'a> { | ||
| pub fn stream_request_with_options(&self, options: Options) -> StreamRequestBuilder<'_> { |
There was a problem hiding this comment.
Callout: I changed this public interface to take a crate::common::options::Options instead of an Option<&crate::common::options::Options>. It says it takes options, so only sometimes taking them is weird, especially if we also provide a version that doesn't take them, and since we can get the same behavior by passing an empty Options.
There was a problem hiding this comment.
Just checked: This function was not available in 0.5.0, so this isn't a breaking change.
Proposed changes
The current implementation double-escapes query parameters passed in
crate::common::Options. This uses proper formencoded parsing instead of string splitting, so the escaping only happens once.I also introduce a pub fn urlencoded to match the other api endpoints, and to help demonstrate the erroneous behavior.
Types of changes
Checklist
Further comments