fix: don't overwrite an Urgency header supplied via the headers option - #980
Open
Jaybhade wants to merge 1 commit into
Open
fix: don't overwrite an Urgency header supplied via the headers option#980Jaybhade wants to merge 1 commit into
Jaybhade wants to merge 1 commit into
Conversation
generateRequestDetails() copied the `headers` option into the request
headers and then unconditionally reassigned `Urgency` to the value of
the `urgency` option, which defaults to "normal". A caller asking for
`headers: { Urgency: 'high' }` therefore had it silently downgraded to
"normal" on the wire, with no error and no warning.
The sibling `Topic` header did not have this problem, because it is only
written when the `topic` option is actually set.
Seed the default urgency into the headers object before the `headers`
option is applied, and only write the option-derived value when one was
supplied. That gives Urgency the same precedence as Topic: the explicit
option wins, an extra header is used when no option is given, and
"normal" remains the default when neither is.
The existing 'Extra headers' test asserted this exact behaviour, but
passed "normal" as the header value -- the same value the overwrite put
back -- so it could never fail. It now uses "high".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
generateRequestDetails()copies theheadersoption into the outgoing request headers, and then unconditionally reassignsUrgencyfrom theurgencyoption — which defaults tonormal. So whatever the caller put inheadersis thrown away:No error and no warning; the notification just goes out at normal urgency. The sibling
Topicheader doesn't have this problem, because it is only written when thetopicoption is actually set —headers: { Topic: 'mytopic' }survives.This seeds the default urgency into the headers object before the
headersoption is applied, and only writes the option-derived value when one was supplied.Urgencythen gets exactly the same precedenceTopicalready has:urgencyoption wins when it is given,Urgencyentry inheadersis used,normal.One note on the existing coverage: the
Extra headerstest already asserted this, but passed'normal'as the header value — the same value the overwrite put back — so the assertion could never fail. It now uses'high', which fails on master and passes here. I also added two tests pinning the default and the option-wins precedence.Two things I deliberately left alone, happy to fold either in if you'd rather:
headers: { Urgency }plusurgencydoesn't trip it (same forTopic/topic). Making it case-insensitive would start throwing for combinations that are accepted today, which felt like a separate decision from this bug.headersare passed through as-is, so anUrgencyset that way isn't checked against the four allowed values — consistent with howTopicbehaves today.Verification:
npm run lintis clean, and the full non-browser suite passes (79 tests acrosstest-generate-request-details,test-vapid-helper,test-set-vapid-details,test-encryption-helper,testSetGCMAPIKey,testSendNotificationandtest-cli).