333 - Implement customHeadersToSkipOnWeakVersion functionality #343

i-b-dimitrov · 2017-01-06T21:52:54Z

Introduced a new registry option - customHeadersToSkipOnWeakVersion, which expects
to be an array of strings denoting the headers to be omitted from
the response in case a weak version is requested (i.e 1.x.x). The rational
behind this is the need to control cache related headers - in case 1.x.x
version is requested we would not like cache headers to be returned because
otherwise the response will be cached. Next time when someone asks for 1.x.x
it will get the cached response even though a new version may be available.
by default customHeadersToSkipOnWeakVersion is set to be empty.
added unit and integration tests.

matteofigus

Thanks for your PR. It already looks excellent. I think it may benefit from a couple of improvements. What do you think?

matteofigus · 2017-01-10T12:06:51Z

src/registry/routes/component.js

+      res.set(componentResponse.headers);
+    } else {
+      //weak version request, therefore skip the blacklisted headers if any
+      if (_.isEmpty(res.conf.customHeadersToSkipOnWeakVersion || [])) {


Isn't || [] redundant as you're already doing that in the options' sanitiser?

matteofigus · 2017-01-10T12:12:03Z

src/registry/routes/component.js

+      if (_.isEmpty(res.conf.customHeadersToSkipOnWeakVersion || [])) {
+          res.set(componentResponse.headers);
+      } else {
+        var headersToSkip = new Set(res.conf.customHeadersToSkipOnWeakVersion);


Wouldn't be easier to use underscore's omit here?

var headersToSet = _.omit(componentResponse.headers.map(function(el){ return el.toLowerCase(); }), res.conf.customHeadersToSkipOnWeakVersion); if(!_.isEmpty(headersToSet)) res.set(headersToSet);

Fixed! Thanks for the idea ;-)

matteofigus · 2017-01-10T12:16:15Z

test/acceptance/registry.js

+      requestData.headers = options.headers || {};
+      requestData.method = 'get';
+
+      var req = require('http').request(requestData).on('response', function(response) {


I guess you're doing all of this because minimal-request doesn't allow you to access response headers. This is not great, I can totally extend minimal-request to do that and then this test should look a bit more neat. Still, not a blocker for merging this, definitely a quick improvement for a later PR. Sorry for that 👍

Completely agree with you. Will modify/fix this once we have the change in minimal-request.

matteofigus · 2017-01-10T12:21:35Z

src/registry/routes/component.js

+        var headersToSkip = new Set(res.conf.customHeadersToSkipOnWeakVersion);
+
+        _.forEach(Object.keys(componentResponse.headers), function(header) {
+          if (!headersToSkip.has(header.toLowerCase())) {


I was thinking that if we change setHeader to sanitise the headers by "lowercasing" it then we may not need to "lowercase" it here?

i-b-dimitrov · 2017-01-13T11:00:15Z

@matteofigus, all the requested changes are in place now. This one also includes upgrading of minimal-request to version 2.2.0 :-)

matteofigus

Just a couple more very minimal changes! Getting there!

Thanks 👍

matteofigus · 2017-01-13T12:07:09Z

test/unit/registry-routes-component.js

@@ -465,6 +465,53 @@ describe('registry : routes : component', function(){
    });
  });

+  describe('when getting a component with server.js that sets custom headers with empty customHeadersToSkipOnWeakVersion', function() {


"not empty"?

Should be fixed

matteofigus · 2017-01-13T12:10:07Z

test/unit/registry-routes-component.js

+    it('should not set the HTTP response test-headers', function() {
+      expect(response.headers).to.not.be.null;      
+      expect(response.headers['test-header']).to.equal('test-value');
+      expect(headers).to.be.undefined;      


I think here I got a bit confused by the response.headers and headers. Do we need here to test response.headers if what we care about is just expect(headers).to.be.undefined to prove that should not set the HTTP response test-headers?

Also, I think same would apply for line 456

Yea, I see your point.
The idea of skipping some predefined headers from the response is to cope with the normal HTTP proxies that stay on the request-response route. Because of the GET request if they are going to do something with the response (like caching) they will do it based on the request string where we don't have a strong version. That's why we validate we don't set the HTTP response headers.
On the other hand in the response body we do have both the requested version and the actually returned version. That's why I'm exposing the headers there as well, so the response is something like:

{ "type": "oc-component-local", "version": "1.0.1", "requestVersion": "1.0.1", "name": "hello-world", "renderMode": "rendered", "href": "http://localhost:3030/hello-world/1.0.1", "headers": { "test-header": "test-value" }, "html": "...." }

This allows us to have those custom headers in POST responses as well :-)

But more and more I'm thinking over this and more I agree with your note here...
Regarding the consistency perhaps it's better to strip them from everywhere.
If you agree I can issue one more commit with this in place?

I think I can't see any value for having headers into the GET /component response body, as we should use the headers for that. So, for simplicity, I agree about stripping the headers from the headers 🤣 in case this setting is on, and I vote for just removing the headers from the response in any case.

In case of the POST batch, I think it is a bit different. Using the response body for the headers is the simplest way to include the headers in each component. The way I would tackle it there, is to have a headers key in the response that behaves exactly as the headers in the GET. Still, I think we can do the POST in a separate PR if you agree.

I updated the change to include this -- I also put the logic for POST requests together with some tests

matteofigus · 2017-01-13T12:13:46Z

npm-shrinkwrap.json

-      "from": "minimal-request@2.1.1",
-      "resolved": "https://registry.npmjs.org/minimal-request/-/minimal-request-2.1.1.tgz"
+      "from": "minimal-request@2.2.0",
+      "resolved": "https://registry.npmjs.org/minimal-request/-/minimal-request-2.2.0.tgz"


I actually have done that on master already with #349 so I guess you just need to rebase from master here?

Oh I actually was able to rebase here. Hope you don't mind 👍

- Introduced new registry option - customHeadersToSkipOnWekVersion which expects to be an array of strings denoting the headers that have to be omitted from the response in case a weak version is requested (i.e 1.x.x). The rational behind this is the need to control cache related headers - in case 1.x.x version is requested we would not like cache headers to be returned because otherwise the response will be cached. Next time when someone asks for 1.x.x it will get the cached response even though a new version may be available. - added unit and integration tests

- also context.setHeader function was extended a bit and now includes a sanity check for the arguments

matteofigus

Only one thing not very clear to me?

matteofigus · 2017-01-16T18:47:51Z

test/acceptance/registry.js

+        it('should skip Cache-Control header', function() {
+          expect(result[0].response.version).to.equal('1.0.0');
+          expect(result[0].response.name).to.equal('hello-world-custom-headers');
+          expect(result[0].response.headers).to.be.deep.equal({'cache-control': 'public max-age=3600', 'test-header': 'Test-Value'});


Shouldn't this skip 'Cache-Control' header and be eql to {'test-header': 'Test-Value'}?

You are right. It's more on that though. Actually I didn't realise the POST requests go to components.js instead of component.js. Now this is fixed. Also one of the big trouble with the tests for POST requests was that they are following after those for GET and they are working with an already set customHeadersToSkipOnWeakVersion. So I did a slight refactoring of the before/afters there as well.

Apologies for this mess :-)
And can I ask for one more review, please?

No problem, looking now 👍

…and GET requests - for GET it will put the custom headers in the HTTP response headers - for POST it will put the custom headers in the response body - the setCustomHeaders was moved out of component.js to get-component.js under the name filterCustomHeaders. It's because in case of POST request the code path is different and goes to component(s).js instead of component.js. Now we do the filtering in one and the same place. - fixed a bug where we used to not take into account the custom headers in case of a unrendered component. For this reason the actual setting of response headers was moved from line 205 up to line 175. - fixed the registry acceptance tests. In their former variant when the GET tests passes they were used to leave the customHeadersToSkipOnWeakVersion settings so the POST tests were starting directly with this setting on. That's why new before/after sections were defined to reinitialize the registry before the tests and then recover it after them.

matteofigus · 2017-01-17T19:00:19Z

Amazing work @ivan-dimitrov-skyscanner - I published and updated the docs here: https://github.com/opentable/oc/wiki/Registry#registry-configuration - what do you think about the description?

i-b-dimitrov · 2017-01-18T09:21:12Z

Awesome :-)
Thank you very much @matteofigus.
I like the description in the docs. Perhaps I have to put some example to document setHeader(...) method in the context (probably I'll do it in the next days).

matteofigus · 2017-01-18T10:23:41Z

Thanks to you! Feel free to edit the wiki if you want, you should be enabled to do it ;)

matteofigus requested changes Jan 10, 2017

View reviewed changes

matteofigus added the Registry's API label Jan 11, 2017

i-b-dimitrov force-pushed the 333-customHeadersToSkipOnWeakVersion branch from 84b3223 to ec01742 Compare January 12, 2017 16:09

matteofigus mentioned this pull request Jan 12, 2017

Upgrade minimal-request #349

Merged

i-b-dimitrov force-pushed the 333-customHeadersToSkipOnWeakVersion branch from ec01742 to 526fe04 Compare January 13, 2017 10:55

matteofigus requested changes Jan 13, 2017

View reviewed changes

i-b-dimitrov added 2 commits January 16, 2017 11:11

333 - Fixed some problems raised in the code review

8c4d71f

- also context.setHeader function was extended a bit and now includes a sanity check for the arguments

i-b-dimitrov force-pushed the 333-customHeadersToSkipOnWeakVersion branch from 05ec822 to 6658726 Compare January 16, 2017 11:19

matteofigus requested changes Jan 16, 2017

View reviewed changes

i-b-dimitrov force-pushed the 333-customHeadersToSkipOnWeakVersion branch from 6658726 to b2f17e4 Compare January 17, 2017 16:29

matteofigus approved these changes Jan 17, 2017

View reviewed changes

matteofigus merged commit 7d46fb9 into opencomponents:master Jan 17, 2017

matteofigus mentioned this pull request Jan 17, 2017

When asking for a component with an unspecified version (i.e. 1.x.x) the response should not contain custom headers #333

Closed

i-b-dimitrov deleted the 333-customHeadersToSkipOnWeakVersion branch January 18, 2017 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

333 - Implement customHeadersToSkipOnWeakVersion functionality #343

333 - Implement customHeadersToSkipOnWeakVersion functionality #343

i-b-dimitrov commented Jan 6, 2017 •

edited

Loading

matteofigus left a comment

matteofigus Jan 10, 2017

i-b-dimitrov Jan 12, 2017

matteofigus Jan 10, 2017

i-b-dimitrov Jan 12, 2017

matteofigus Jan 10, 2017

i-b-dimitrov Jan 12, 2017

matteofigus Jan 10, 2017

i-b-dimitrov Jan 12, 2017

i-b-dimitrov commented Jan 13, 2017

matteofigus left a comment

matteofigus Jan 13, 2017

i-b-dimitrov Jan 13, 2017

matteofigus Jan 13, 2017

i-b-dimitrov Jan 13, 2017

i-b-dimitrov Jan 13, 2017

matteofigus Jan 13, 2017

i-b-dimitrov Jan 13, 2017

matteofigus Jan 13, 2017

matteofigus Jan 13, 2017

matteofigus left a comment

matteofigus Jan 16, 2017

i-b-dimitrov Jan 17, 2017

matteofigus Jan 17, 2017

matteofigus commented Jan 17, 2017

i-b-dimitrov commented Jan 18, 2017 •

edited

Loading

matteofigus commented Jan 18, 2017

333 - Implement customHeadersToSkipOnWeakVersion functionality #343

333 - Implement customHeadersToSkipOnWeakVersion functionality #343

Conversation

i-b-dimitrov commented Jan 6, 2017 • edited Loading

matteofigus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i-b-dimitrov commented Jan 13, 2017

matteofigus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matteofigus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matteofigus commented Jan 17, 2017

i-b-dimitrov commented Jan 18, 2017 • edited Loading

matteofigus commented Jan 18, 2017

i-b-dimitrov commented Jan 6, 2017 •

edited

Loading

i-b-dimitrov commented Jan 18, 2017 •

edited

Loading