-
Notifications
You must be signed in to change notification settings - Fork 586
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
Add custom headers to HttpsCallableReference #6587
base: main
Are you sure you want to change the base?
Conversation
8305f35
to
6c079bb
Compare
* @param name Name of HTTP header | ||
* @param value Value of HTTP header | ||
*/ | ||
public fun addHeader(name: String, value: String): HttpsCallableReference { |
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.
Shall we also add a addHeaders(headers: Map<String, String>)
function?
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.
Sounds good!
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.
Done.
Oops, I missed the PR description before reviewing the code.
That's a really good point! It was actually suggested internally that we provide headers in |
6c079bb
to
2dc0e55
Compare
40fae11
to
738de3d
Compare
738de3d
to
6e1cbc3
Compare
@thatfiredev I already went forward and implemented this. Actually it makes a lot more sense this way because the header values are now handled like the timeout option, for example. They are kept in the internal |
Implements the
addHeader
functionality as mentioned in this issue.Unfortunately I was not able to test these changes because deploying Firebase Functions requires a Blaze (pay-as-you-go) plan for a Firebase project. It's not possible to use a free (Spark) plan (and I can't use the production business project for these tests). I kindly ask you to run the tests for me. I prepared a test function
headersTest
in this PR.Also, currently we don't support multiple headers with the same name, which is possible by HTTP standards.
If we want to support this, we have to replace the internal collection holding headers by a Multimap (
Map<String, Set<String>>
).One more thing:
Currently headers can only be added to an instance of
HttpsCallableReference
, meaning on a per-function basis. If we want to support global headers and minimize repetition, we could extendHttpsCallOptions
to also hold headers, since options can be reused and passed to several functions. Headers would then have the following precedencemeaning any global headers are overwritten by instance headers with same names, lastly internal headers will overwrite any existing headers with the same names.
What do you think?