342 - (OC Client) Perform GET instead of POST for single component #347

i-b-dimitrov · 2017-01-11T16:24:36Z

Also:

Extended get-components-data error handling to anticipate a flat response
with a single error field when a major 4xx error is detected for the request.
In this case we extend the error message to provide as much data as possible.
Added more tests and fixed the existing ones.

i-b-dimitrov · 2017-01-11T16:45:37Z

Hey @matteofigus, I have really no idea why it is failing. I don't think my changes should affect the failing test in any way :-(

Also one of the tests is showing green but instead of green tick it has three green '��' in front of it?!
Any ideas are welcome...

matteofigus · 2017-01-11T18:33:33Z

Just hitting re-run worked ;)

i-b-dimitrov · 2017-01-12T09:28:08Z

Awesome, thanks :-)

i-b-dimitrov · 2017-02-06T10:45:16Z

Hi @matteofigus, can we merge this one as it becomes a kind of a blocker for us?
If there is something in particular that you don't like with this PR, just shout and we can refactor it :-)

matteofigus

Thanks for waiting and sorry for taking so long to review.

I added a couple of comments and requested some changes, but overall it seems pretty good.

matteofigus · 2017-02-06T23:30:22Z

test/acceptance/client.js

@@ -612,27 +654,15 @@ describe('The node.js OC client', function(){
        it('should contain the error details', function(){

          var expectedRequestWithExtraParams = {
-            url: 'http://localhost:1234',
-            method: 'post',
+            url: 'http://localhost:1234/hello-world/~1.0.0??hi=john',


Sorry, not sure I understand why there are 2 ?. What's wrong with 1?
Or, what about http://localhost:1234/hello-world/~1.0.0/?hi=john

That's because expectedResults are actually regular expressions and '?' is a special symbol so we need to escape it. If we have an expected result like 'some ? text' we need to turn it into 'some \\? text' before the final processing (single back slash doesn't work because '\?' equals '?'). However we also run the getRegExpFromJson(x) function on it, which employs JSON.stringify() that in turn will additionally escape every '\' with '\\' and our string becomes 'some \\\\? text' internally.
That's why I decided to escape it with double question mark '??'.
If you are asking if we can escape '?' with '/?' that's possible, too.
Any other ideas are welcome? :-)

Ok, it this is only for escaping that it is fine. Perhaps, just to be 100% sure about what happens, we may create a test for a component with parameters and test the response has the correct href with the structure component/version/?params - that could be done without regex and would be useful for tests readability I think?

Yes, it is only for escaping the symbol in the regex and only in the tests. Regarding the second part - I'm not quite sure I understand what you are asking for...
If it's to add an additional test about if we set the right URL/href address then the only way this can be done in the client.js acceptance tests is to test for an error condition. There we are getting the request URL in the error message (exactly what's happening in the tests). However if we want to intercept and check the request then we should create a unit test for get-component-data because renderComponent/s methods don't us the full response, just the data in it.

matteofigus · 2017-02-06T23:36:21Z

client/src/href-builder.js

-
-      return url.resolve(registrySegment, component.name) + versionSegment + qs;
+      
+      return url.resolve(registrySegment, component.name || '') + versionSegment + qs;


Can we actually validate component.name is not empty here and in case return a validation error? No point of making a req if component.name is missing.

matteofigus · 2017-02-06T23:40:21Z

client/src/get-components-data.js

+    var urlPath = component.name + (component.version ? '/' + component.version : '');
+    var queryString;
+
+    if (options.parameters) {


We have an href-builder for building the client-side failover urls. Can we move this there perhaps in a new "server" method?
https://github.com/opentable/oc/blob/master/client/src/href-builder.js#L9

I think you may be 1) able to reuse most of that stuff and perhaps refactor a bit as I think the only thing changing is the base url 2) keep the same convention which is version/?params instead of version??params

matteofigus · 2017-02-06T23:42:51Z

test/acceptance/client.js

@@ -38,7 +38,8 @@ describe('The node.js OC client', function(){
  var getRegExpFromJson = function(x){
    return JSON.stringify(x)
      .replace(/\+/g, '\\+')
-      .replace(/\[/g, '\\[');
+      .replace(/\[/g, '\\[')
+      .replace(/\?\?/g, '\\?'); //In our json regexp in order to preserve a single ? we are escaping it  with ??


Not a big fan I guess, see next comment where I ask for clarification

@matteofigus, I did some changes and introduced new tests to cover the expectations we have for get-component-data to prepare a GET request when just one component is passed and for href-builder to include the component parameters as URL query params for the GET request URL.
However I can't come up with an idea for getting rid of this '??' escaping. Perhaps I don't understand your question entirely :-(

matteofigus · 2017-02-10T23:10:32Z

client/src/href-builder.js

+      var queryString;
+
+      if (options.parameters) {
+        queryString = Object


I would prefer using querystring.stringify as in line 36: qs = !!component.parameters ? ('/?' + querystring.stringify(component.parameters)) : ''; if you don't mind?

matteofigus · 2017-02-12T02:04:43Z

test/acceptance/client.js

@@ -612,27 +654,15 @@ describe('The node.js OC client', function(){
        it('should contain the error details', function(){

          var expectedRequestWithExtraParams = {
-            url: 'http://localhost:1234',
-            method: 'post',
+            url: 'http://localhost:1234/hello-world/~1.0.0??hi=john',


Ok, it this is only for escaping that it is fine. Perhaps, just to be 100% sure about what happens, we may create a test for a component with parameters and test the response has the correct href with the structure component/version/?params - that could be done without regex and would be useful for tests readability I think?

…ust one component (opencomponents#342) Also: - Extended get-components-data error handling to anticipate a flat response with a single error field when a major 4xx error is detected for the request. In this case we extend the error message to provide as much data as possible. - Added more tests and fixed the existing ones.

- added a validation for null or empty component.name - added a validation for clientRenderingOptionsNotSet - the method is changed to not return null results - instead it will throw an exception

- Added a test to validate that getting the data for just one component will result in a href-builder.prepareGetRequest call and that the result of it will subsequently be used as a URL for the actual call. - Added a test to validate that getting more than one component (two components) will not result in calling href-builder.prepareGetRequest and will construct the resulting URL based only on the URL returned by href-builder.server method.

- prepareServerGet was changed to use querystring.stringify for addign the component parameters as URL query params. - change the returned URL to include a trailing '/' before the URL query params (and thus making it to follow the same pattern as in client method) - changed the client.js acceptance test according to the aforementioned changes - added client-href-builder.js unit test

matteofigus · 2017-02-17T11:16:27Z

Thanks @ivan-dimitrov-skyscanner this is a great work!

matteofigus added the node.js client label Jan 11, 2017

matteofigus mentioned this pull request Jan 12, 2017

Upgrade minimal-request #349

Merged

i-b-dimitrov force-pushed the 342-Get-Instead-Of-Post-For-Single-Component branch 2 times, most recently from 67bbebe to 831a8c2 Compare January 18, 2017 09:39

matteofigus mentioned this pull request Jan 24, 2017

Cleanup on headers handling #370

Merged

matteofigus requested changes Feb 6, 2017

View reviewed changes

i-b-dimitrov force-pushed the 342-Get-Instead-Of-Post-For-Single-Component branch from c43bc85 to bdfae81 Compare February 9, 2017 21:26

matteofigus requested changes Feb 12, 2017

View reviewed changes

i-b-dimitrov force-pushed the 342-Get-Instead-Of-Post-For-Single-Component branch from ddbdecf to 6e9d087 Compare February 16, 2017 16:21

i-b-dimitrov added 5 commits February 16, 2017 17:25

Added additional and separate parameter validation in href-builder.js

ad19525

- added a validation for null or empty component.name - added a validation for clientRenderingOptionsNotSet - the method is changed to not return null results - instead it will throw an exception

Separating url related logic into href-builder

205a169

i-b-dimitrov force-pushed the 342-Get-Instead-Of-Post-For-Single-Component branch from ada38c0 to d1e4949 Compare February 16, 2017 18:03

matteofigus approved these changes Feb 17, 2017

View reviewed changes

matteofigus merged commit 29e5bf9 into opencomponents:master Feb 17, 2017

i-b-dimitrov deleted the 342-Get-Instead-Of-Post-For-Single-Component branch February 20, 2017 11:49

matteofigus mentioned this pull request Apr 3, 2017

Component parameters are ignored when doing backend-rendering with oc-client nodejs library #431

Closed

dependencies bot mentioned this pull request Nov 9, 2019

Update chalk in package.json from ^2.1.0 to 3.0.0 #1128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

342 - (OC Client) Perform GET instead of POST for single component #347

342 - (OC Client) Perform GET instead of POST for single component #347

i-b-dimitrov commented Jan 11, 2017

i-b-dimitrov commented Jan 11, 2017

matteofigus commented Jan 11, 2017

i-b-dimitrov commented Jan 12, 2017

i-b-dimitrov commented Feb 6, 2017

matteofigus left a comment

matteofigus Feb 6, 2017

i-b-dimitrov Feb 10, 2017 •

edited

Loading

matteofigus Feb 12, 2017

i-b-dimitrov Feb 14, 2017

matteofigus Feb 6, 2017

i-b-dimitrov Feb 8, 2017

matteofigus Feb 6, 2017

matteofigus Feb 6, 2017

i-b-dimitrov Feb 16, 2017

matteofigus Feb 10, 2017

matteofigus Feb 12, 2017

matteofigus commented Feb 17, 2017


		return url.resolve(registrySegment, component.name) + versionSegment + qs;

		return url.resolve(registrySegment, component.name \|\| '') + versionSegment + qs;

342 - (OC Client) Perform GET instead of POST for single component #347

342 - (OC Client) Perform GET instead of POST for single component #347

Conversation

i-b-dimitrov commented Jan 11, 2017

i-b-dimitrov commented Jan 11, 2017

matteofigus commented Jan 11, 2017

i-b-dimitrov commented Jan 12, 2017

i-b-dimitrov commented Feb 6, 2017

matteofigus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i-b-dimitrov Feb 10, 2017 • edited Loading

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 commented Feb 17, 2017

i-b-dimitrov Feb 10, 2017 •

edited

Loading