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.

Sign up for GitHub

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

Jump to bottom

fix: allow stream protocols to return headers with multiple values #14887

Merged
merged 4 commits into from Oct 25, 2018
Merged

fix: allow stream protocols to return headers with multiple values #14887

merged 4 commits into from Oct 25, 2018

Conversation

ibash
Copy link
Contributor

@ibash ibash commented Sep 30, 2018

Description of Change

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes #14778

cc @sofianguy

Checklist
Release Notes

Notes: Fixed returning headers with multiple values for stream protocols.

@ibash ibash requested a review from a team September 30, 2018 22:52
@welcome
Copy link

welcome bot commented Sep 30, 2018

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@ibash
Copy link
Contributor Author

ibash commented Oct 7, 2018

@MarshallOfSound updated! did you want me to dedupe the method as well?

@ibash
Copy link
Contributor Author

ibash commented Oct 10, 2018

Bump @MarshallOfSound

ckerr
ckerr requested changes Oct 10, 2018
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's great to see first time submitters come up with solid code and tests. Thanks for the PR!

atom/common/native_mate_converters/net_converter.cc Outdated Show resolved Hide resolved
atom/common/native_mate_converters/net_converter.cc Outdated Show resolved Hide resolved
@ibash
Copy link
Contributor Author

ibash commented Oct 13, 2018

@ckerr back to you!

@MarshallOfSound
Copy link
Member

@ibash you have a consistently failing test here, once CI is green we can take another look at this

@ibash
Copy link
Contributor Author

ibash commented Oct 19, 2018 •
edited

@MarshallOfSound fixed! (not sure how I missed that before, sorry about that!)

ckerr
ckerr approved these changes Oct 23, 2018
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These CI failures seem unrelated to the PR itself, e.g. cannot open display: :99.0.

I'd prefer to see CI green and have raised an issue in the Slack channel, but am OK with this landing regardless

@MarshallOfSound
Copy link
Member

MarshallOfSound commented Oct 23, 2018 •
edited

@ckerr I'm rerunning the linux CI from the beginning, something probably broke in the middle and is influencing the partial reruns.

🔗 https://circleci.com/workflow-run/e97ae132-c6fc-477f-a971-79d64643f753

@jkleinsc
Copy link
Contributor

@ibash can you rebase this PR with the latest from master? The CI tests are failing due to an issue that was resolved in master.

@ibash
fix: allow stream protocols to return headers with multiple values
b15fd19 
This allows stream protocols to return headers with multiple values as
an array of values.

Fixes #14778
@ibash
Prefer ConvertFromV8
5cb932b
@ibash
Cleanup header conversion
9108958 
1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)
@ibash
Fix broken test
463fbc3 
Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.
@ibash
Copy link
Contributor Author

ibash commented Oct 23, 2018 via email

@ibash ibash mentioned this pull request Oct 24, 2018
Open
@ckerr
Copy link
Member

ckerr commented Oct 24, 2018

@MarshallOfSound OK to merge?

@MarshallOfSound MarshallOfSound merged commit 3b6f0d8 into electron:master Oct 25, 2018
@release-clerk
Copy link

release-clerk bot commented Oct 25, 2018

Release Notes Persisted

Fixed returning headers with multiple values for stream protocols.

@welcome
Copy link

welcome bot commented Oct 25, 2018

Congrats on merging your first pull request! 🎉🎉🎉

@ibash
Copy link
Contributor Author

ibash commented Nov 25, 2018

@MarshallOfSound any opposition to backporting this to 3 and 4?

@ibash ibash mentioned this pull request Dec 28, 2018
Closed
miniak pushed a commit that referenced this pull request May 1, 2019
@ibash @miniak
fix: allow stream protocols to return headers with multiple values (#…
c0533d5 
…14887)

* fix: allow stream protocols to return headers with multiple values

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes #14778

* Prefer ConvertFromV8

* Cleanup header conversion

1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)

* Fix broken test

Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.
@miniak miniak mentioned this pull request May 1, 2019
Merged
5 tasks
@trop
Copy link
Contributor

trop bot commented May 1, 2019

A maintainer has manually backported this PR to "4-1-x", please check out #18094

1 similar comment
@trop
Copy link
Contributor

trop bot commented May 1, 2019

A maintainer has manually backported this PR to "4-1-x", please check out #18094

@trop
Copy link
Contributor

trop bot commented May 1, 2019

A maintainer has manually backported this PR to "4-1-x", please check out #18094

@trop trop bot added the in-flight/4-1-x label May 1, 2019
MarshallOfSound pushed a commit that referenced this pull request May 1, 2019
@miniak @MarshallOfSound
fix: allow stream protocols to return headers with multiple values (#…
ac714a1 
…14887) (#18094)

* fix: allow stream protocols to return headers with multiple values

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes #14778

* Prefer ConvertFromV8

* Cleanup header conversion

1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)

* Fix broken test

Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr approved these changes

@MarshallOfSound MarshallOfSound MarshallOfSound approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Converter<net::HttpResponseHeaders*>::FromV8 does not allow multiple headers with the same name
4 participants
@ibash @MarshallOfSound @jkleinsc @ckerr