SOAP integration: set root element namespace #41 #50

jumokeoladimeji · 2016-12-21T01:24:59Z

alexkwolfe · 2016-12-21T02:21:00Z

spec/soap-spec.coffee

+
+
+    it 'should require xmlnsAttribute value', ->
+      assert.equal soap.validate(url: 'http://foo', function: 'whatever', version:'1.1', namespace_prefix: 'tns', xmlnsAttribute_name: 'xmlns:tns'), 'XmlnsAttribute value is required'


We want this configuration to be optional.

alexkwolfe · 2016-12-21T02:22:06Z

src/soap.coffee

@@ -189,6 +204,9 @@ requestVariables = ->
    { name: 'url', description: 'WSDL URL', type: 'string', required: true }
    { name: 'function', description: 'Name of the SOAP function to call', type: 'string', required: true }
    { name: 'version', description: 'SOAP version to use: 1.1 or 1.2 (default: 1.1)', type: 'string', required: false }
+    { name: 'namespace_prefix', description: 'namespace prefix for the body element', type: 'string', required: true }
+    { name: 'xmlnsAttribute_name', description: 'xmlnsAttribute name for the body element', type: 'string', required: true }
+    { name: 'xmlnsAttribute_value', description: 'xmlnsAttribute value for the body element', type: 'string', required: true }


Let's make the variable name all lower case. Maybe root_xmlns_attribute_name and root_xmlns_attribute_value?

kellym · 2016-12-21T15:16:50Z

re: tests and validations, it seems as per https://www.w3.org/TR/REC-xml-names/ that xml namespaces are URIs. since this checks vars.url is a valid url, might make sense to do the same for the namespace. i seem to be reading conflicting things on if it's always a URI, but w3c seems to think it should be.

also, for testing the new attributes, you might want to test that the overrideRootElement changes leave the default when those vars are undefined and change it when they're passed in.

cgrayson · 2017-01-03T18:06:02Z

Kelly said:

since this checks vars.url is a valid url, might make sense to do the same for the namespace. i seem to be reading conflicting things on if it's always a URI, but w3c seems to think it should be.

I came across some of those conflicting things you mentioned on Wikipedia: "In general, however, users should assume that the namespace URI is simply a name, not the address of a document on the Web".

For this advanced setting, I think being more lax is better. If someone needs to set up this namespace business, they'll have to enter the value right one way or the other. I'm afraid that as soon as it's validated to be a URI, we'll have a customer that wants to use a non-URI value.

Any thoughts, @alexkwolfe ?

cgrayson

In addition to this feedback, there are outstanding questions to discuss about URI validation.

cgrayson · 2017-01-03T17:16:46Z

spec/soap-response-with-namespace.xml

+        <tns:AddLeadResponse xmlns:tns="http://donkey/ws.asmx/">
+            <AddLeadXMLResult>
+                &lt;Response&gt;
+                    &lt;Result&gt;true&lt;/Result&gt;


I don't see a reason for these tags to be in <s and >s, instead of <s and >s?

cgrayson · 2017-01-03T17:21:23Z

src/soap.coffee

 validateVersion = (vars) ->
  version = vars.version?.trim()
  return unless version?
  return 'Must be valid SOAP version: 1.1 or 1.2' unless version == '1.1' or version == '1.2'

+validateXmlnsAttributeValue = (vars) ->
+  if vars.root_xmlns_attribute_value?
+    console.log vars.root_xmlns_attribute_value,'validate url result', validate.isValidUrl(vars.root_xmlns_attribute_value)


Forgot to remove this debugging statement.

cgrayson · 2017-01-03T17:34:18Z

src/soap.coffee

@@ -189,6 +198,9 @@ requestVariables = ->
    { name: 'url', description: 'WSDL URL', type: 'string', required: true }
    { name: 'function', description: 'Name of the SOAP function to call', type: 'string', required: true }
    { name: 'version', description: 'SOAP version to use: 1.1 or 1.2 (default: 1.1)', type: 'string', required: false }
+    { name: 'root_namespace_prefix', description: 'namespace prefix for the body element', type: 'string', required: false }
+    { name: 'root_xmlns_attribute_name', description: 'xmlnsAttribute name for the body element', type: 'string', required: false }
+    { name: 'root_xmlns_attribute_value', description: 'xmlnsAttribute value for the body element', type: 'string', required: false }


These variables are presented to the user in the UI in the order they're listed here. I think these new settings won't be very commonly used, so they'd be better at the end of this array.

I also think we can give better information about these (admittedly advanced) settings than "xmlnsAttribute name for the body element".

cgrayson · 2017-01-03T18:13:54Z

spec/soap-spec.coffee

@@ -24,12 +25,14 @@ describe 'Outbound SOAP', ->
    vars =
      url: 'http://donkey/login/ws/ws.asmx?WSDL'
      function: 'someUnsupportedFunction'
+      root_xmlns_attribute_name: 'xmlns:tns'
+      root_xmlns_attribute_value: 'http://skylist.com/services/Soap'


I don't think these new lines are needed.

cgrayson · 2017-01-03T18:14:02Z

spec/soap-spec.coffee

@@ -41,6 +44,8 @@ describe 'Outbound SOAP', ->
    vars =
      url: 'http://donkey/login/ws/ws.asmx?WSDL'
      function: 'AddLead'
+      root_xmlns_attribute_name: 'xmlns:tns'
+      root_xmlns_attribute_value: 'http://donkey/login/ws/ws.asmx?WSDL'


Same here, I don't think these new lines are needed.

cgrayson · 2017-01-03T18:15:07Z

spec/soap-spec.coffee

+        done()
+
+
+


As Kelly mentioned previously, there should also be test coverage for when these new variables are not set, to ensure the requests are still constructed correctly.

kellym · 2017-01-03T18:52:29Z

@cgrayson I agree with being more lax on the namespace. My point of reference was https://www.w3.org/TR/REC-xml-names/#concepts -- with the definition being a URI reference. I did a bit more digging just now and found this unclear documentation as well -- not sure if it's referring to a valid URI format or actual endpoint: https://www.w3.org/TR/REC-xml-names/#ProcessorConformance.

So on that note, since namespace validation is pretty unclear, probably best to avoid it after all.

alexkwolfe · 2017-01-03T20:56:26Z

I vote for omitting URI validation for the namespaces.

jumokeoladimeji · 2017-01-04T15:20:47Z

@cgrayson I have made the changes.

cgrayson · 2017-01-04T22:36:16Z

src/validate.coffee

@@ -46,5 +46,3 @@ module.exports =
    # ensure that user-specified Content-Type is similar to what's allowed
    if headers['content-type']? and !headers['content-type']?.match(new RegExp(contentTypeBase, 'i'))
      return "Invalid Content-Type header value"
-
-


These whitespace changes are making this file show as changed when it hasn't, really.

cgrayson · 2017-01-04T22:39:08Z

spec/soap-response-with-namespace.xml

+          <Result>true</Result>
+          <Message>some message</Message>
+          <LeadId>12345</LeadId>
+          <Empty></Empty>


This would normally just be <Empty/>, as here.

cgrayson · 2017-01-04T23:27:06Z

spec/soap-spec.coffee

+        .reply 200, wsdl, 'Content-Type': 'text/xml'
+
+      @service = nock 'http://donkey'
+        .post '/login/ws/ws.asmx'


This test (and the following one) aren't quite sufficient. They're only verifying that a POST is made by the integration to that URL, but not what is posted. This test should verify that the correct SOAP header stuff is added, and the test below should verify that no SOAP header stuff is changed. I think using a regex, as described in the nock docs, would be the way to go.

cgrayson · 2017-01-04T23:30:44Z

src/soap.coffee

+      namespace: vars.root_namespace_prefix
+      xmlnsAttributes: [
+        { name: vars.root_xmlns_attribute_name, value: vars.root_xmlns_attribute_value }
+      ]


I think you'll find, once tests are more thorough, that these options need to be set only when the vars.root_… values are present. For example, I think this code will set an attribute like undefined="undefined".

Also, as mentioned in discussion on #41, there would ideally be support for multiple name/value pairs here.

cgrayson

Let me know if you have any questions, or when this is ready to review again

cgrayson · 2017-01-13T19:37:14Z

src/soap.coffee

+
+  # prevent root xlmns attribute key and value from being added to the root element when it is undefined
+  if !vars.root_xlmns_attributes
+    delete options.overrideRootElement


This logic would be a little clearer if the options values were only ever set if needed, rather than always set and then possibly deleted later. E.g., don't declare them at all at first, and then:

if _.isPlainObject(vars.root_xlmns_attributes) options.overrideRootElement = {} options.overrideRootElement.namespace = vars.root_namespace_prefix if vars.root_namespace_prefix for key, value of vars.root_xlmns_attributes options.overrideRootElement.xmlnsAttributes.push { name: key, value: value }

Though, unfortunately, the way you have this set up won't work. vars.root_xmlns_attributes here is an object, but data isn't addressable like that in LeadConduit. That is, users can't map an object like { 'xmlns:cal': 'http://donkey/ws.asmx/' } in the LC UI. To support multiples, I think there would need to be an array for root_xmlns_names, and another matching one for root_xmlns_values… (unless there's some way I'm not thinking of, @alexkwolfe?).

So, although I think ideally we would allow multiple, for now please step back to one pair, and we'll hope that's enough for the time being.

I have edited it. I agree that they should be arrays. The reason why I used wildcard as the type instead of an array is that array is not included as a type in the documentation.

cgrayson

Getting close, I just have that last concern.

cgrayson · 2017-01-16T15:59:35Z

src/soap.coffee

+  # prevent root xlmns attribute key and value from being added to the root element when it is undefined
+  if vars.root_xmlns_attribute_name and vars.root_xmlns_attribute_value
+    options.overrideRootElement =
+      namespace: vars.root_namespace_prefix


Hmm. I know this is set up as I suggested 😃 but I think there may be a problem here. If only root_namespace_prefix is set (and root_xmlns_attribute_name or …value isn't), then this namespace option will never be set. Maybe tests for each combination of values would make sure these cases are covered.

alexkwolfe · 2017-01-16T16:25:13Z

Jumoke, based on what you said in our integrations meeting today, it sounds like maybe you were thinking of implementing an array of namespaces. But it sounds like we're OK just skipping that for now. And the only remaining issue is the one that chris brought up here.

cgrayson · 2017-01-16T23:40:26Z

src/soap.coffee

+
+  # prevent root xlmns attribute key and value from being added to the root element when undefined
+  if vars.root_xmlns_attribute_name and vars.root_xmlns_attribute_value
+    options.overrideRootElement['xmlnsAttributes'] =


The problem here is that if vars.root_namespace_prefix isn't defined, then there is no options.overrideRootElement. If you add a test with root_xmlns_attribute_name and root_xmlns_attribute_value, but no vars.root_namespace_prefix, you'll see what I mean.

cgrayson

Otherwise LGTM

cgrayson · 2017-01-17T16:19:51Z

src/soap.coffee

  options =
    valueKey: '#value'
    forceSoap12Headers: vars.version?.trim() == '1.2'
    disableCache: true
+    overrideRootElement:
+      namespace: vars.root_namespace_prefix


Though it's evidently not currently causing problems, per your tests, when vars.root_namespace_prefix isn't set, this code is setting overrideRootElement.namespace to undefined, which makes me nervous.

Is this the last thing to address before we get this merged?

kellym · 2017-01-24T15:30:02Z

src/soap.coffee

+      ]
+
+  if !vars.root_namespace_prefix
+    delete options.overrideRootElement['namespace']


Instead of deleting this property, I think it makes more sense to not set it at all. I know you're running into an error when root_namespace_prefix isn't defined but the others are, so you could use _.set around L28 to reduce some of the logic to make sure options.overrideRootElement is defined:

_.set options, 'overrideRootElement.xmlnsAttributes', [ name: vars.root_xmlns_attribute_name value: vars.root_xmlns_attribute_value ]

cgrayson · 2017-01-26T19:03:24Z

Still LGTM

…ce or xmlns attribute name/value pair when specified

jumokeoladimeji self-assigned this Dec 21, 2016

jumokeoladimeji requested a review from cgrayson December 21, 2016 01:24

jumokeoladimeji changed the title ~~Fix/41~~ SOAP integration: set root element namespace #41 Dec 21, 2016

alexkwolfe requested changes Dec 21, 2016

View reviewed changes

cgrayson requested changes Jan 3, 2017

View reviewed changes

cgrayson requested changes Jan 4, 2017

View reviewed changes

cgrayson requested changes Jan 13, 2017

View reviewed changes

cgrayson requested changes Jan 16, 2017

View reviewed changes

cgrayson approved these changes Jan 17, 2017

View reviewed changes

kellym reviewed Jan 24, 2017

View reviewed changes

jumokeoladimeji force-pushed the fix/41 branch from cf07cdb to 026d1f5 Compare January 30, 2017 17:04

Add support for setting OverrideRootElement with root element namespa…

5c41ff2

…ce or xmlns attribute name/value pair when specified

jumokeoladimeji force-pushed the fix/41 branch from 026d1f5 to 5c41ff2 Compare January 30, 2017 17:36

jumokeoladimeji merged commit 776a724 into master Jan 30, 2017

kellym mentioned this pull request Feb 13, 2017

SOAP integration: set root element namespace #41

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOAP integration: set root element namespace #41 #50

SOAP integration: set root element namespace #41 #50

jumokeoladimeji commented Dec 21, 2016 •

edited

Loading

alexkwolfe Dec 21, 2016

alexkwolfe Dec 21, 2016

kellym commented Dec 21, 2016

cgrayson commented Jan 3, 2017

cgrayson left a comment

cgrayson Jan 3, 2017

cgrayson Jan 3, 2017

cgrayson Jan 3, 2017

cgrayson Jan 3, 2017

cgrayson Jan 3, 2017

cgrayson Jan 3, 2017

cgrayson Jan 3, 2017

kellym commented Jan 3, 2017

alexkwolfe commented Jan 3, 2017

jumokeoladimeji commented Jan 4, 2017

cgrayson Jan 4, 2017

cgrayson Jan 4, 2017

cgrayson Jan 4, 2017

cgrayson Jan 4, 2017

cgrayson Jan 4, 2017

cgrayson left a comment

cgrayson Jan 13, 2017

cgrayson Jan 13, 2017

jumokeoladimeji Jan 15, 2017 •

edited

Loading

cgrayson left a comment

cgrayson Jan 16, 2017

alexkwolfe commented Jan 16, 2017

cgrayson Jan 16, 2017

cgrayson left a comment

cgrayson Jan 17, 2017

alexkwolfe Jan 23, 2017

kellym Jan 24, 2017

cgrayson commented Jan 26, 2017



		it 'should require xmlnsAttribute value', ->
		assert.equal soap.validate(url: 'http://foo', function: 'whatever', version:'1.1', namespace_prefix: 'tns', xmlnsAttribute_name: 'xmlns:tns'), 'XmlnsAttribute value is required'

SOAP integration: set root element namespace #41 #50

SOAP integration: set root element namespace #41 #50

Conversation

jumokeoladimeji commented Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellym commented Dec 21, 2016

cgrayson commented Jan 3, 2017

cgrayson 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

kellym commented Jan 3, 2017

alexkwolfe commented Jan 3, 2017

jumokeoladimeji commented Jan 4, 2017

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

cgrayson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jumokeoladimeji Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

cgrayson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkwolfe commented Jan 16, 2017

Choose a reason for hiding this comment

cgrayson 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

cgrayson commented Jan 26, 2017

jumokeoladimeji commented Dec 21, 2016 •

edited

Loading

jumokeoladimeji Jan 15, 2017 •

edited

Loading