add fields sequence if it's required (need help) #914

vfrbgt · 2017-01-16T11:15:36Z

I want sort args on call soap method if sequence required(check parent element sequence). It's my implement for this functional. But I have problem 3 test not passed. Please help me fix it. I really need this functionality in your library. Thank you.

coveralls · 2017-01-16T11:19:51Z

Coverage decreased (-0.2%) to 92.996% when pulling 847ec16 on vfrbgt:master into 54d20e8 on vpulim:master.

jsdevel · 2017-01-16T18:24:41Z

please add a test

coveralls · 2017-01-17T01:00:26Z

Coverage decreased (-0.6%) to 92.611% when pulling f382547 on vfrbgt:master into 54d20e8 on vpulim:master.

herom · 2017-01-17T07:55:23Z

test/client-test.js

+      //   });
+      // });
+
+      it('check sort args on sequence required method', function(done) {


how does this test guarantee the functionality of sorting the args in the correct "sequence" order?

herom · 2017-01-17T07:56:02Z

lib/client.js

+      if(argsScheme) {
+        args = this._setSequenceArgs(argsScheme, args);
+      }
+      console.log(args);


please remove calls to console alltogether - use debug instead

herom · 2017-01-17T07:57:01Z

lib/client.js

+Client.prototype._isSequenceRequired = function(method) {
+  try {
+    var tns = this.wsdl.definitions.xmlns.tns;
+    var childrenName = this.wsdl.definitions.schemas[tns].complexTypes.pullFileParams.children[0].name;


are these pullFileParams specific to your project or are they defined in the specification?

herom · 2017-01-17T07:58:43Z

lib/client.js

+    if(typeof argsScheme[partIndex] !== 'object') {
+      result[partIndex] = args[partIndex];
+    }
+    else {


Please, pull else { to the same line as the closing curly-brace of the if {}

-->

if { //code } else { // code }

herom · 2017-01-17T08:01:04Z

lib/client.js

+
+Client.prototype._getArgsScheme = function(methodName) {
+  try {
+    return this.wsdl.definitions.messages[methodName].parts;


Please consider that we're using lodash within our project which does provide a get() method which can achieve the same thing you're doing here, without using a costly try/catch

herom · 2017-01-17T08:02:17Z

lib/client.js

@@ -202,6 +242,17 @@ Client.prototype._invoke = function(method, args, location, callback, options, e
    },
    xmlnsSoap = "xmlns:" + envelopeKey + "=\"http://schemas.xmlsoap.org/soap/envelope/\"";

+  if(name === 'pullFile') {


again, is pullFile a specified element of the WSDL specification or "just" related to your project?

vfrbgt · 2017-01-18T01:53:16Z

sorry after use test xml files from this projects i detected errors in my implementstion, in this time i'm try fix it. I'm write here when i finish fix this functional. Thanks for review.

vfrbgt · 2017-01-18T04:26:08Z

Can you help me understand, how i can get info about methods and his arguments. I tried get it from client.wsdl.definitions but unable understand which property i must use for this, some properties duplicate data about method arguments (for example client.wsdl.definitions.schemas[tns].complexTypes[methodName+'Params'].children[0].children and this.wsdl.definitions.bindings.RpcExample.methods.pullFile.input.parts.params.children[0].children) How i can get this data correctly.

vfrbgt · 2017-01-18T06:40:07Z

I want understand why for https://swsim.stamps.com/swsim/swsimv50.asmx?wsdl and test file rpcexample.wsdl for exmaple client.describe().SwsimV50.SwsimV50Soap.CreateIndicium.input and client.describe().RpcExample.RpcExample.pullFile.input.params have different structure.

coveralls · 2017-01-20T11:07:54Z

Coverage decreased (-0.3%) to 92.85% when pulling 4cecdec on vfrbgt:master into 54d20e8 on vpulim:master.

coveralls · 2017-01-20T11:07:54Z

Coverage decreased (-0.3%) to 92.85% when pulling 4cecdec on vfrbgt:master into 54d20e8 on vpulim:master.

coveralls · 2017-01-20T11:07:54Z

Coverage decreased (-0.3%) to 92.85% when pulling 4cecdec on vfrbgt:master into 54d20e8 on vpulim:master.

coveralls · 2017-01-20T11:10:10Z

Coverage decreased (-0.3%) to 92.85% when pulling f94684c on vfrbgt:master into 54d20e8 on vpulim:master.

vfrbgt · 2017-01-20T16:31:44Z

need code review

jsdevel · 2017-01-20T20:46:31Z

Looks good to me!

coveralls · 2017-01-24T23:52:20Z

Coverage decreased (-0.1%) to 93.021% when pulling 73dcc1d on vfrbgt:master into 54d20e8 on vpulim:master.

coveralls · 2017-01-25T17:53:33Z

Coverage decreased (-0.1%) to 93.038% when pulling 923cfb6 on vfrbgt:master into 54d20e8 on vpulim:master.

jsdevel · 2017-01-25T20:14:57Z

lib/client.js

@@ -182,6 +182,54 @@ Client.prototype._defineMethod = function(method, location) {
  };
 };

+Client.prototype._isSequenceRequired = function(methodName) {
+  var tns = this.wsdl.definitions.$targetNamespace;
+  var methodRequestName = _.result(this.wsdl.definitions, 'messages.'+methodName+'.$name');


can you add spaces between operators? e.g. 'messages.' + methodName + '...'

It would be nice if we had this failing the build as part of linting too (for another PR if needed).

jsdevel · 2017-01-25T20:16:02Z

lib/client.js

+    return false;
+  }
+  var complexTypeName = _.result(this.wsdl.definitions, 'messages.'+methodRequestName+'.element.$name');
+  var modeOfComplexType = _.result(this.wsdl.definitions, 'schemas[\''+tns+'\'].elements.'+complexTypeName+'.children[0].children[0].name');


can you break this up so the line is shorter? same comment with spaces around operators

jsdevel · 2017-01-25T20:16:23Z

lib/client.js

+    }
+  }
+
+


at most there should be one empty line.

coveralls · 2017-01-25T21:19:32Z

Coverage decreased (-0.1%) to 93.038% when pulling b59b1e7 on vfrbgt:master into 54d20e8 on vpulim:master.

aleung · 2017-02-24T04:07:43Z

@jsdevel test/sequence-test.js has lint error when I execute npm run test. Why your CI passed?

> jshint index.js lib test

test/sequence-test.js: line 53, col 5, Expected '}' to have an indentation at 7 instead at 5.
test/sequence-test.js: line 55, col 1, Expected '}' to have an indentation at 3 instead at 1.

2 errors

pihvi · 2017-05-24T14:19:08Z

lib/client.js

@@ -202,6 +258,13 @@ Client.prototype._invoke = function(method, args, location, callback, options, e
    },
    xmlnsSoap = "xmlns:" + envelopeKey + "=\"http://schemas.xmlsoap.org/soap/envelope/\"";

+  if(this._isSequenceRequired(name)) {


I had to revert to v.0.18.0 as this breaks node-soap. When a client is formed from a WSDL and it has an array inside nested object in input like this (the way client.describe() tells):

input: { ResourceId: "s:string", PeriodList: { PeriodType[]: { PeriodId: "s:int", Status: "s:string", DateFrom: "s:dateTime", DateTo: "s:dateTime"...

It will ignore given array in input PeriodType: [...], but will accept "PeriodType[]": {} and it will generate malformed XML by generating <PeriodType[]></PeriodType[]> element.

v.0.18.0 Works as expected by accepting PeriodType: [...], also works when this if is disabled if(false && this._isSequenceRequired(name)) {

pihvi

This breaks node-soap: https://github.com/vpulim/node-soap/pull/914/files#r118265159

herom · 2017-05-27T19:24:35Z

@pihvi would it be possible for you to add a failing test for this? I think we should follow the path of fixing a bug by first creating (and merging!) a failing test for it. What do you think @jsdevel ?

I also got this vpulim#914 (comment) when running npm test and build failed. Now it passes.

pihvi · 2017-05-28T17:50:05Z

I tried running test locally, but build failed for jshint. Did PR #931 to fix that first.

I also got this vpulim#914 (comment) when running npm test and build failed. Now it passes.

I also got this #914 (comment) when running npm test and build failed. Now it passes.

v. 0.19.0 broke giving array as a parameter. It would now accept the "array" with invalid syntax like this: {input: {PeriodList: {"PeriodType[]": {PeriodId: '1'}}}} i.e. it wants the "[]" as the parameter name and it does not accept an array, but an object. The invalid syntax then prints invalid XML like this: "<PeriodList><PeriodType[]><PeriodId>1</PeriodId></PeriodType[]></PeriodList>" This test shows old and correct syntax. This test passes if you revert commit 5dbcb1d or disable if statement mentioned in vpulim#914 (review)

pihvi · 2017-05-29T12:31:20Z

@herom added a failing test for this in PR #933

v. 0.19.0 broke giving array as a parameter. It would now accept the "array" with invalid syntax like this: {input: {PeriodList: {"PeriodType[]": {PeriodId: '1'}}}} i.e. it wants the "[]" as the parameter name and it does not accept an array, but an object. The invalid syntax then prints invalid XML like this: "<PeriodList><PeriodType[]><PeriodId>1</PeriodId></PeriodType[]></PeriodList>" This test shows old and correct syntax. This test passes if you revert commit 5dbcb1d or disable if statement mentioned in #914 (review)

…new behaviour in vpulim#914)

I also got this vpulim#914 (comment) when running npm test and build failed. Now it passes.

v. 0.19.0 broke giving array as a parameter. It would now accept the "array" with invalid syntax like this: {input: {PeriodList: {"PeriodType[]": {PeriodId: '1'}}}} i.e. it wants the "[]" as the parameter name and it does not accept an array, but an object. The invalid syntax then prints invalid XML like this: "<PeriodList><PeriodType[]><PeriodId>1</PeriodId></PeriodType[]></PeriodList>" This test shows old and correct syntax. This test passes if you revert commit 5dbcb1d or disable if statement mentioned in vpulim#914 (review)

I also got this vpulim/node-soap#914 (comment) when running npm test and build failed. Now it passes.

v. 0.19.0 broke giving array as a parameter. It would now accept the "array" with invalid syntax like this: {input: {PeriodList: {"PeriodType[]": {PeriodId: '1'}}}} i.e. it wants the "[]" as the parameter name and it does not accept an array, but an object. The invalid syntax then prints invalid XML like this: "<PeriodList><PeriodType[]><PeriodId>1</PeriodId></PeriodType[]></PeriodList>" This test shows old and correct syntax. This test passes if you revert commit 5dbcb1d715843310c799419f260e0962722cd2fb or disable if statement mentioned in vpulim/node-soap#914 (review)

add fields sequence if it's required

847ec16

add test and fix get sequence element method (not ended)

f382547

herom reviewed Jan 17, 2017

View reviewed changes

vfrbgt added 2 commits January 20, 2017 18:05

sequence set for wsdl files where params right parsed in wsdl.js

4cecdec

fix style

f94684c

sequence args if sequence args is known

73dcc1d

vfrbgt added 2 commits January 26, 2017 00:45

fix sequence reuire check

7b5feec

fix test wrong parsed params for sequnce

923cfb6

jsdevel requested changes Jan 25, 2017

View reviewed changes

code style fixes

b59b1e7

jsdevel approved these changes Jan 25, 2017

View reviewed changes

jsdevel merged commit 5dbcb1d into vpulim:master Jan 25, 2017

pihvi reviewed May 24, 2017

View reviewed changes

pihvi added a commit to pihvi/node-soap that referenced this pull request May 28, 2017

Fix build by satisfying jshint indentation

aba7700

I also got this vpulim#914 (comment) when running npm test and build failed. Now it passes.

pihvi mentioned this pull request May 28, 2017

Fix build by satisfying jshint indentation #931

Merged

pihvi added a commit to pihvi/node-soap that referenced this pull request May 29, 2017

Fix build by satisfying jshint indentation

d0aafcb

I also got this vpulim#914 (comment) when running npm test and build failed. Now it passes.

herom pushed a commit that referenced this pull request May 29, 2017

Fix build by satisfying jshint indentation (#931)

18c2790

I also got this #914 (comment) when running npm test and build failed. Now it passes.

pihvi mentioned this pull request May 29, 2017

Add test for accepting array as parameter based on wsdl #933

Merged

herom added a commit to herom/node-soap that referenced this pull request May 29, 2017

[FIX] Possible fix vpulim#933 (regression because of introduction of …

588b5b1

…new behaviour in vpulim#914)

herom added a commit to herom/node-soap that referenced this pull request May 29, 2017

[FIX] Possible fix vpulim#933 (regression because of introduction of …

dab5c87

…new behaviour in vpulim#914)

herom mentioned this pull request May 29, 2017

[FIX] [DO NOT MERGE] Possible fix #933 (regression because of introduction of new be… #935

Closed

herom added a commit to herom/node-soap that referenced this pull request May 29, 2017

[FIX] Possible fix vpulim#933 (regression because of introduction of …

b18e382

…new behaviour in vpulim#914)

jsdevel added a commit that referenced this pull request May 30, 2017

Reverting #914

2f8c045

jsdevel mentioned this pull request May 30, 2017

Remove brackets from argsScheme string if they exist #929

Closed

jsdevel added a commit that referenced this pull request May 30, 2017

Reverting #914

f2f05fc

jsdevel mentioned this pull request May 30, 2017

Reverting #914 #937

Merged

jsdevel added a commit that referenced this pull request May 30, 2017

Reverting #914 (#937)

97e0452

jcald1 pushed a commit to jcald1/node-soap that referenced this pull request Jun 8, 2017

Fix build by satisfying jshint indentation (vpulim#931)

39cefa3

I also got this vpulim#914 (comment) when running npm test and build failed. Now it passes.

jcald1 pushed a commit to jcald1/node-soap that referenced this pull request Jun 8, 2017

Reverting vpulim#914 (vpulim#937)

f5ebbf5

johnolson2219 pushed a commit to johnolson2219/soap-nodeJS that referenced this pull request Aug 26, 2023

Fix build by satisfying jshint indentation (#931)

5a2757d

I also got this vpulim/node-soap#914 (comment) when running npm test and build failed. Now it passes.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add fields sequence if it's required (need help) #914

add fields sequence if it's required (need help) #914

vfrbgt commented Jan 16, 2017

coveralls commented Jan 16, 2017 •

edited

Loading

jsdevel commented Jan 16, 2017

coveralls commented Jan 17, 2017 •

edited

Loading

herom Jan 17, 2017

herom Jan 17, 2017

herom Jan 17, 2017

herom Jan 17, 2017

herom Jan 17, 2017

herom Jan 17, 2017

vfrbgt commented Jan 18, 2017

vfrbgt commented Jan 18, 2017

vfrbgt commented Jan 18, 2017

coveralls commented Jan 20, 2017 •

edited

Loading

coveralls commented Jan 20, 2017

coveralls commented Jan 20, 2017

coveralls commented Jan 20, 2017 •

edited

Loading

vfrbgt commented Jan 20, 2017

jsdevel commented Jan 20, 2017

coveralls commented Jan 24, 2017 •

edited

Loading

coveralls commented Jan 25, 2017 •

edited

Loading

jsdevel Jan 25, 2017

jsdevel Jan 25, 2017

jsdevel Jan 25, 2017

jsdevel Jan 25, 2017

coveralls commented Jan 25, 2017 •

edited

Loading

aleung commented Feb 24, 2017

pihvi May 24, 2017 •

edited

Loading

pihvi left a comment

herom commented May 27, 2017

pihvi commented May 28, 2017

pihvi commented May 29, 2017

add fields sequence if it's required (need help) #914

add fields sequence if it's required (need help) #914

Conversation

vfrbgt commented Jan 16, 2017

coveralls commented Jan 16, 2017 • edited Loading

jsdevel commented Jan 16, 2017

coveralls commented Jan 17, 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

vfrbgt commented Jan 18, 2017

vfrbgt commented Jan 18, 2017

vfrbgt commented Jan 18, 2017

coveralls commented Jan 20, 2017 • edited Loading

coveralls commented Jan 20, 2017

coveralls commented Jan 20, 2017

coveralls commented Jan 20, 2017 • edited Loading

vfrbgt commented Jan 20, 2017

jsdevel commented Jan 20, 2017

coveralls commented Jan 24, 2017 • edited Loading

coveralls commented Jan 25, 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

coveralls commented Jan 25, 2017 • edited Loading

aleung commented Feb 24, 2017

pihvi May 24, 2017 • edited Loading

Choose a reason for hiding this comment

pihvi left a comment

Choose a reason for hiding this comment

herom commented May 27, 2017

pihvi commented May 28, 2017

pihvi commented May 29, 2017

coveralls commented Jan 16, 2017 •

edited

Loading

coveralls commented Jan 17, 2017 •

edited

Loading

coveralls commented Jan 20, 2017 •

edited

Loading

coveralls commented Jan 20, 2017 •

edited

Loading

coveralls commented Jan 24, 2017 •

edited

Loading

coveralls commented Jan 25, 2017 •

edited

Loading

coveralls commented Jan 25, 2017 •

edited

Loading

pihvi May 24, 2017 •

edited

Loading