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

Updated the headers map in WebRequest and WebResponse to accept multiple headers with the same name #1150

Merged
merged 4 commits into from
Nov 15, 2024

Conversation

B1rtek
Copy link
Contributor

@B1rtek B1rtek commented Nov 14, 2024

Because there can be multiple headers with the same name, for example Set-Cookie (which was the main motivation for this change)
Also it's probably terrible idk (although the game didn't explode when I tried to use it and it actually worked)

loader/include/Geode/utils/web.hpp Outdated Show resolved Hide resolved
@B1rtek B1rtek requested review from Fleeym and matcool November 14, 2024 09:05
loader/src/utils/web.cpp Outdated Show resolved Hide resolved
loader/src/utils/web.cpp Outdated Show resolved Hide resolved
@B1rtek B1rtek requested a review from matcool November 14, 2024 21:58
@Fleeym
Copy link
Contributor

Fleeym commented Nov 14, 2024

So as a little personal recap:

  • m_headers is now std::unordered_map<std::string, std::vector<std::string>>
  • you can still use res->header(name), and if the key has multiple headers it returns the first one(?)
  • you have res->headersWithName(name), which returns the full vector of values under that header key (vec can be empty)

if that's all then this lgtm, but maybe we could change headersWithName to something else, current name just doesn't sound right

@B1rtek
Copy link
Contributor Author

B1rtek commented Nov 14, 2024

m_headers is now std::unordered_map<std::string, std::vector<std::string>>

yes

you can still use res->header(name), and if the key has multiple headers it returns the first one(?)

yes, it returns the first one, I'm not sure whether it should be this or the last one, because with the previous version of the map technically the last one would be returned as the previous ones were overwritten, I'd say that it probably doesn't matter for all mods written until this point because nobody yet complained that they don't get the correct one (I think)

you have res->headersWithName(name), which returns the full vector of values under that header key (vec can be empty)

if the vec is empty it'll return std::nullopt because that means that the map of headers doesn't have a key with this header's name

As for the name I didn't really know what to call it, I'd propose maybe changing it to getListOfHeadersNamed/getListOfHeadersWithName (that's long but would definitely please Java devs), or renaming header() to getHeader() and the new one to getHeaders() but that would probably be confusing
edit: getAllHeadersNamed()?

@B1rtek
Copy link
Contributor Author

B1rtek commented Nov 15, 2024

I renamed it to getAllHeadersNamed()

@matcool matcool merged commit 64d9a28 into geode-sdk:main Nov 15, 2024
@B1rtek
Copy link
Contributor Author

B1rtek commented Nov 15, 2024

🎉

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

Successfully merging this pull request may close these issues.

3 participants