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

feat(service-worker): bypass on queryparam/header (#21191) #30010

Conversation

@petersalomonsen
Copy link
Contributor

commented Apr 21, 2019

issue #21191

added ngsw-bypass header and queryparameter
for bypassing serviceworker on specific requests

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: 21191

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@petersalomonsen petersalomonsen requested a review from angular/fw-service-worker as a code owner Apr 21, 2019

@googlebot googlebot added the cla: yes label Apr 21, 2019

@petersalomonsen petersalomonsen force-pushed the petersalomonsen:21191-serviceworker-bypass branch 2 times, most recently from 8e339fd to b7bc822 Apr 21, 2019

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

@gkalpak I believe everything from #21191 is covered now. Let me know if anything else is needed for this Pull Request.

@ngbot ngbot bot added this to the needsTriage milestone Apr 22, 2019

@gkalpak
Copy link
Member

left a comment

Thx for taking this on, @petersalomonsen. I have left some minor comments, but generally this LGTM.
Also, can you please update the commit message to follow our guidelines more closely. E.g. something like:

feat(service-worker): support bypassing SW with specific header/query param

Add support for bypassing the ServiceWorker for a request by using the ngsw-bypass header or query
parameter.

Fixes #21191

@@ -92,6 +92,15 @@ to refresh the resource in the background. This way, broken
unhashed resources do not remain in the cache beyond their
configured lifetimes.

### Bypass the service worker

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 23, 2019

Member

I would move this to the end of the "Service worker and caching of app resources" section.
I, also, think that "Bypassing" sounds more consistent with the other headings.

This comment has been minimized.

Copy link
@petersalomonsen

petersalomonsen Apr 23, 2019

Author Contributor

This is done.

@@ -176,6 +176,11 @@ export class Driver implements Debuggable, UpdateSource {
*/
private onFetch(event: FetchEvent): void {
const req = event.request;

if (req.headers.has('ngsw-bypass') || req.url.indexOf('ngsw-bypass=true') > -1) {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 23, 2019

Member
  1. I suggest handling both the header and the query param similarly: Either check for presense only or check for a value of 'true'.
    One problem with requiring 'true' is that it implies that adifferent value (e.g. 'false') disables the by-passing, which in turn raises the question of which values are valid, how to handle invalid values (if any), whether the header or the query param has precedence, etc. Therefore, I might lean towards checking for presense only (and making it clear in the docs).

  2. The query param check needs to be stricter. E.g. extend adapter.parseUrl() to also return .search and check something like requestUrlObj.search.slice(1).toLowerCase().split('&').includes('ngsw-bypass' /*or ngsw-bypass=true*/).

This comment has been minimized.

Copy link
@petersalomonsen

petersalomonsen Apr 23, 2019

Author Contributor

Thanks for the review @gkalpak , and good feedback. I see your point, but just want to check with you before proceeding with stricter checks.

If acceptable I'd like to keep it as simple as possible (hopefully lower risk of bugs), and as CPU efficient as possible ( less expensive to search for a string than splitting, converting to lowercase, using regex etc ).

So I've modified the docs, so that it's clearer on the criteria for bypassing the SW. Now it says you can have either the request header with any value, or the query parameter with value true. I think adding the value requirement reduces the risks of collision with ngsw-bypass other places in the URL (like in the path, or other parameters with different postfix). Checking for & means that you can't have the parameter directly after ? so I'd prefer not checking for &.

So my point here is that keeping it simple makes it easier for others to understand, and it's also faster code, while making it more complex also could make it more vulnerable and in that case more difficult to debug.

What do you think?

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 23, 2019

Member

I'm afraid I will have to disagree 😁

Simpler code is great but only if the resulting requirements are simple. I would aim for simpler (and more intuitive) requirements first. I still stand by my earlier suggestion to:

  1. Have consistency between header and query param values.
  2. Correctly detect query param.
  3. Be case insensitive (which again keeps consistency between header and query param).

To address some of your concerns directly:

I'd like to keep it as simple as possible (hopefully lower risk of bugs) [...].

Keeping it too simple would indeed lower the risk of bugs in our code, but increase the risk of bugs in app code.

I'd like to keep it [...] as CPU efficient as possible (less expensive to search for a string than splitting, converting to lowercase, using regex etc).

I wouldn't be so worried about computational complexity, because this is not a hot path and the other operations involved are much more expensive anyway.

This comment has been minimized.

Copy link
@petersalomonsen

petersalomonsen Apr 23, 2019

Author Contributor

point taken :-) I'll try to implement it as you suggest. hopefully tomorrow.

This comment has been minimized.

Copy link
@petersalomonsen

petersalomonsen Apr 24, 2019

Author Contributor

@gkalpak now only checking for presence of the ngsw-bypass query param, and it's case insensitive. I added the search property to the result of adapter.parseUrl() as you suggested.

Hopefully closer now to what you have in mind now, maybe you want some more test cases? I added some more.


expect(await makeRequest(scope, '/bar.txt?ngsw-bypass=false')).toBe('this is bar');
server.assertSawRequestFor('/bar.txt');
});

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 23, 2019

Member

Depending on how we decide to handle header/query param values, we need more checks to cover for other cases. E.g.:

  • Lower/Upper/Mixed case.
  • Falsy value/No value.
  • Having more headers/query params.
  • Having similarly named headers/query params (e.g. ?no-ngsw-bypass=true).
  • Having both the header and the query param.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 24, 2019

Member

Thx for adding the extra tests. They look great.

Can you please add a couple more:

  • Upper/Mixed case headers.
  • Falsy value/No value (both headers and query param).

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 24, 2019

Member

I'd like to see one more test with an empty query param (e.g. ?ngsw-bypass=&foo=ngsw-bypass).

@petersalomonsen petersalomonsen force-pushed the petersalomonsen:21191-serviceworker-bypass branch 2 times, most recently from c326079 to c7b357a Apr 23, 2019

@gkalpak
Copy link
Member

left a comment

Thx, @petersalomonsen. Looking much better already 😉
I left some more suggestions (but we are close) 😇

handling the request. An example is when uploading files, where
you will not see progress events if the request is going through
the service worker. To bypass the serviceworker you can set `ngsw-bypass`
as a request header, or as a query parameter with any value.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 24, 2019

Member

"with any value" suggests that there must be a value, which is not true 😁

@@ -176,9 +176,16 @@ export class Driver implements Debuggable, UpdateSource {
*/
private onFetch(event: FetchEvent): void {
const req = event.request;

This comment has been minimized.

Copy link
@gkalpak
const scopeUrl = this.scope.registration.scope;
const requestUrlObj = this.adapter.parseUrl(req.url, scopeUrl);

if (req.headers.has('ngsw-bypass') ||
requestUrlObj.search.slice(1).toLowerCase().split('&').find(

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 24, 2019

Member
  • Since most of the time .search will be empty, you might want to short-circuit the more complex operations if .search is falsy.
  • Since we don't need the query param (just want to test for presence), .some() is preferrable.
const scopeUrl = this.scope.registration.scope;
const requestUrlObj = this.adapter.parseUrl(req.url, scopeUrl);

if (req.headers.has('ngsw-bypass') ||
requestUrlObj.search.slice(1).toLowerCase().split('&').find(
param => param.split('=')[0] === 'ngsw-bypass')) {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 24, 2019

Member

Since you are concerned about performance, it might be faster to avoid splitting each key-value pair (e.g. use param === 'ngsw-bypass' || param.startsWith('ngsw-bypass=')).

Or even using a RegExp (and avoiding all the slicing/lower-casing/splitting/searching in array): /[?&]ngsw-bypass(?:[=&]|$)/i.test(requestUrlObj.search)

UPDATE:
It turns out, avoiding splitting key-value pairs is twice as fast and using a RegExp is 7x faster: Benchmark
(Again, not that all this should have any noticeable impact on the overall operation 😁)

This comment has been minimized.

Copy link
@petersalomonsen

petersalomonsen Apr 24, 2019

Author Contributor

You are doing all the work here @gkalpak :-) I'm learning a lot, so very happy to be part of the process here! Very nice benchmark ( I suspected regexp was faster, but didn't really benchmark as you did ). I copied your RegExp which seems to do the job.

@@ -184,14 +184,15 @@ export class SwTestHarness implements ServiceWorkerGlobalScope, Adapter, Context
}, new MockHeaders());
}

parseUrl(url: string, relativeTo?: string): {origin: string, path: string} {
parseUrl(url: string, relativeTo?: string): {origin: string, path: string, search: string} {
const parsedUrl: URL = (typeof URL === 'function') ?
new URL(url, relativeTo) :
require('url').parse(require('url').resolve(relativeTo || '', url));

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 24, 2019

Member

AFAICT, Node's url.parse(...) will set .search to null (instead of an empty string).
So, we either need to normalize null to an empty string or type the return value as search: null | string.

This comment has been minimized.

Copy link
@petersalomonsen

petersalomonsen Apr 24, 2019

Author Contributor

I chose to return an empty string instead of changing the return type, since the use of url.parse(...) only applies to older nodejs versions (todays version has URL type).


expect(await makeRequest(scope, '/bar.txt?ngsw-bypass=false')).toBe('this is bar');
server.assertSawRequestFor('/bar.txt');
});

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 24, 2019

Member

Thx for adding the extra tests. They look great.

Can you please add a couple more:

  • Upper/Mixed case headers.
  • Falsy value/No value (both headers and query param).

@petersalomonsen petersalomonsen force-pushed the petersalomonsen:21191-serviceworker-bypass branch 2 times, most recently from f802ad9 to a33c301 Apr 24, 2019

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Also added tests for no value and mixed case headers. For the mixed case headers test, I had to modify the MockHeaders class to store and look up lower case keys. Testing https://developer.mozilla.org/en-US/docs/Web/API/Headers/has seems to be case insensitive, so MockHeaders should now behave more like that - though an iteration on the MockHeaders keys will only show lowercase keys, which might be an issue? (UPDATE: not an issue, this is how it is in the browser too - iterating through header keys gives lower case results).

@gkalpak
Copy link
Member

left a comment

A couple of minor comments. LGTM otherwise 👍

const parsedUrl: URL = (typeof URL === 'function') ?
new URL(url, relativeTo) :
require('url').parse(require('url').resolve(relativeTo || '', url));

return {
origin: parsedUrl.origin || `${parsedUrl.protocol}//${parsedUrl.host}`,
path: parsedUrl.pathname,
search: parsedUrl.search ? parsedUrl.search : ''

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 24, 2019

Member

Nit: Could be simplified to parsedUrl.search || ''.

This comment has been minimized.

Copy link
@petersalomonsen

petersalomonsen Apr 24, 2019

Author Contributor

Good point. Did that now.

await makeRequest(scope, '/bar.txt?ngsw-bypaSS=something');
server.assertNoRequestFor('/bar.txt');

await makeRequest(scope, '/bar.txt?testparam=test&ngsw-bypASS=anything');

This comment has been minimized.

Copy link
@gkalpak
await makeRequest(scope, '/bar?testparam=test&ngsw-bypass&testparam2');
server.assertNoRequestFor('/bar');


This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 24, 2019

Member

Nit: Unnecessary newline.

This comment has been minimized.

Copy link
@petersalomonsen

petersalomonsen Apr 24, 2019

Author Contributor

Fixed


expect(await makeRequest(scope, '/bar.txt?ngsw-bypass=false')).toBe('this is bar');
server.assertSawRequestFor('/bar.txt');
});

This comment has been minimized.

Copy link
@gkalpak

gkalpak Apr 24, 2019

Member

I'd like to see one more test with an empty query param (e.g. ?ngsw-bypass=&foo=ngsw-bypass).

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Also added one more test with empty query param as you asked for.

@googlebot

This comment has been minimized.

Copy link

commented Apr 25, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Apr 25, 2019

@gkalpak
Copy link
Member

left a comment

Thx, @petersalomonsen 👍
I have added a fixup commit with some suggested fixes. LMK if it looks good to you, so I can go ahead and mark this for merging.

For posterity:

I was pondering whether we should also recognize an x--prefixed header, since that seems to be a common requirement for non-standard headers. But the current recommendation is discouraging the x- prefix.

It is easy to address this (in a non-breaking way) later, so let's re-consider, if this turns out to be a problem. Another approach could be to have the SW remove the header or query param altogether, but that would make the requests differ depending on whether the SW is enabled/disabled. so probably not a good idea.

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Looks good @gkalpak . Also tested building the serviceworker package and using the resulting ngsw-worker.js and it works as it should.

Not too worried about using a x- prefixed header, I think ngsw- is probably unique enough. Modifying the request and removing headers / query params is not really needed / wanted as I see it, but I often tend to go for the least comprehensive solutions (which is sometimes good, but not always) :-)

@ngbot

This comment has been minimized.

Copy link

commented Apr 25, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "cla/google" is failing
    failure status "ci/angular: size" is failing
    pending missing required labels: cla: yes
    pending forbidden labels detected: PR action: cleanup, cla: no

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@gkalpak

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

merge-assistance: No idea what is causing the ci/angular: size check failure or how to fix.

@IgorMinar
Copy link
Member

left a comment

Lgtm. Thanks

@IgorMinar IgorMinar added cla: yes and removed cla: no labels Apr 25, 2019

@googlebot

This comment has been minimized.

Copy link

commented Apr 25, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@gkalpak

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

For posterity:

As @alexeagle's suggested, I have compared the core/hello_world/bundle.br artifacts from master and this PR and there are identical.

petersalomonsen and others added some commits Apr 25, 2019

feat(service-worker): support bypassing SW with specific header/query…
… param

Add support for bypassing the ServiceWorker for a request by using the
ngsw-bypass header or query parameter.

Fixes #21191

@gkalpak gkalpak force-pushed the petersalomonsen:21191-serviceworker-bypass branch from ac929e8 to 9f6307d Apr 25, 2019

@googlebot

This comment has been minimized.

Copy link

commented Apr 25, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Apr 25, 2019

@AndrewKushnir AndrewKushnir added cla: yes and removed cla: no labels Apr 25, 2019

@googlebot

This comment has been minimized.

Copy link

commented Apr 25, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot

This comment has been minimized.

Copy link

commented Apr 25, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Apr 25, 2019

@AndrewKushnir AndrewKushnir added cla: yes and removed cla: no labels Apr 25, 2019

@googlebot

This comment has been minimized.

Copy link

commented Apr 25, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@petersalomonsen petersalomonsen deleted the petersalomonsen:21191-serviceworker-bypass branch Apr 26, 2019

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

feat(service-worker): support bypassing SW with specific header/query…
… param (angular#30010)

Add support for bypassing the ServiceWorker for a request by using the
ngsw-bypass header or query parameter.

Fixes angular#21191

PR Close angular#30010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.