Multiple fixes and rpc support draft #75

kernle32dll · 2017-12-23T15:55:31Z

So, after some coffee fueled coding days, I have finished all the necessary changes to make this project work for my use case. Which is RPC - as I found out rather late.

I tested this intensively with my use case, and everything seems fine. Compatibility with non-rpc is worked in, but I was unable to test this as I don't have any non-rpc server at hand to test with. I did verify however that requests and response structures are generated so specification (so they should work).

Besides rpc support this PR makes the necessary changes for functions with any number (zero and more than 2 in particular) of parameters. Functions which have more than one return value will probably not work correctly at the moment (this was broken before, but this PR makes most of the changes to get it working in the future)

So - this PR should ™ fix the following issues:
#65 "wsdl2go issues"
#74 "Soap funcs with != 1 parameter not working"
#45 "SOAP call returns empty string"
(not sure about that one, but the body looks like rpc - so it might work now)
#9 "Namespaced operations"
(I hacked support for tns - this is not finished, but should get most use-cases of the ground. Also, it can be overridden by the client just in case)

Before this gets merged, the tests need to be adjusted, and it would probably a good idea to write tests which test the generated xml for the requests. Also, this needs some intensive testing with non-rpc servers, to ensure that I did not break previously working behavior.

Fank · 2017-12-27T10:07:56Z

travis: cannot find package "github.com/iancoleman/strcase"

Fank · 2017-12-27T10:08:38Z

soap/client.go

@@ -44,6 +44,7 @@ type AuthHeader struct {
 type Client struct {
 	URL                    string              // URL of the server
 	Namespace              string              // SOAP Namespace
+	TNamespace             string              // SOAP Namespace


I think this should be renamed to ThisNamespace because the t stands for this

fiorix

I started reviewing diffs in the order you posted and then realized you already fixed some of the things I commented on, but not all.

It'd be great if you could squash your commits for a full review before this can be merged.

fiorix · 2017-12-27T14:27:53Z

wsdl/types.go

+	Operations  []*BindingOperation `xml:"operation"`
+}
+
+type SOAPBinding struct {


This is a such a small diff and yet it cannot figure out the difference between Binding and SOAPBinding by just looking at it. Also this will fail golint due to exported type with no documentation.

fiorix · 2017-12-27T14:28:48Z

soap/client.go

@@ -44,6 +44,7 @@ type AuthHeader struct {
 type Client struct {
 	URL                    string              // URL of the server
 	Namespace              string              // SOAP Namespace
+	TNamespace			   string              // SOAP Namespace


What's the difference between Namespace and TNamespace? Comments should clarify this.

fiorix · 2017-12-27T14:29:43Z

main.go

@@ -10,8 +10,8 @@ import (
 	"net/url"
 	"os"

-	"github.com/fiorix/wsdl2go/wsdl"
-	"github.com/fiorix/wsdl2go/wsdlgo"
+	"github.com/Kernle32DLL/wsdl2go/wsdl"


Bad imports will cause compilation errors.

When I fork go code I normally keep the original package path and just add origin to the local git config pointing to my fork. And then add upstream pointing to the fork's origin.

fiorix · 2017-12-27T14:32:40Z

wsdlgo/encoder.go

-	ge.needsExtPkg["github.com/fiorix/wsdl2go/soap"] = true
-	in[0] = renameParam(in[0], "α")
-	out[0] = renameParam(out[0], "β")
+	ge.needsExtPkg["github.com/Kernle32DLL/wsdl2go/soap"] = true


This will generate code that does not compile when people use the original repo.

fiorix · 2017-12-27T14:33:13Z

wsdlgo/encoder.go

+			Action              string
+			PortType            string
+			Name                string
+			OpName				string


Gotta run gofmt in all this code.

fiorix · 2017-12-27T14:36:54Z

wsdlgo/encoder.go

@@ -23,7 +23,8 @@ import (
 	"strings"
 	"text/template"

-	"github.com/fiorix/wsdl2go/wsdl"
+	"github.com/Kernle32DLL/wsdl2go/wsdl"
+	"github.com/iancoleman/strcase"


Can we avoid this new dependency? iirc we currently have zero external dependencies.

fiorix · 2017-12-27T14:43:25Z

CI is failing due to missing strcase package (https://travis-ci.org/fiorix/wsdl2go/jobs/320687939).

In one of the reviews I questioned whether we really need to import that package vs just copy that code. We currently have zero external deps.

It's such a trivial function anyway. I'd copy that code and add a comment pointing to the original repo giving credits to the author. Licenses are compatible.

kernle32dll · 2017-12-27T15:31:01Z

@fiorix @Fank Thanks for the review, I will work the changes in, and do a squash after that.

Also, the tests are probably failing - I will look into that.

kernle32dll · 2017-12-27T17:58:52Z

@fiorix @Fank I did implement the changes, and did a squash. Also, while updating the golden files I found and fixes a few errors I missed so far.

I have no idea how to rewrite client_test.go to make it work again tho. I presume this has to to with the fact that DoRoundTrip now does the body encapsulating itself.

As for the encoder tests: Renewing the *.golden files actually serves quite well to demonstrate the changes. Some notworthy things to comment on:

data.golden
Probably the most simple to overview. Note the changes to γ in GetData in particular. This change means that the response is now expected to be <parameters><return>...</return></parameters>,
instead of <getDataResp><return>...</return></getDataResp> - this aligns with data.wsdl line 64 (I have no idea why it was getDataResp previously)
memcache.golden
Funnily, this was a rpc wsdl. I don't know if this worked before, but interestingly the new encoder kinda reflect what was there before, with some strange twists.
The impact of rpc can be seen especially in the γ of e.g. GetMulti. While the xml key was right before (GetMultiResponse), the structure was wrong. We now expect <GetMultiResponse><Values><values>....</values><Values></GetMultiResponse> instead of <GetMultiResponse><Values></Values></GetMultiResponse>. I'm not sure if this is wrong, but the expected behavior aligns with the wsdl.
I presume the wsdl has ben hacked before (see first line of wsdl), so that the encoder built as it did (if so - one should probably change the wsdl back).
localimport.golden
I fear that the inline structs for GetLastTradePrice are wrong, as from the looks this will cause <Body><body>...</body></Body> marshalling (note the casing).
No idea whats going on here.... Thoughts?
soap12wcf.golden
Same as data.golden - also note that HelloRequest and HelloResponse types don't exist (and didn't before)
tpexample1.golden
SayHello was previously not implemented - it is now - yay!
w3example1.golden
Same as localimport
w3cexample1.golden w3cexample2.golden
No change - methods still not implemented.

Fank · 2017-12-27T18:09:12Z

Tests are failing

Fank · 2017-12-28T10:59:29Z

I'm currently try this PR with our wsdl files and i got some type issues:

File https://xbeservetest.bedirect.de/soap/wsdl/2

Generated wsdl.go:

// .. cut

// Operation wrapper for Search.
// OperationSearchOut was auto-generated from WSDL.
type OperationSearchOut struct {
	Return *BedirectServerSearchResult `xml:"return,omitempty" json:"return,omitempty" yaml:"return,omitempty"`
}

// Operation wrapper for Information.
// OperationInformationOut was auto-generated from WSDL.
type OperationInformationOut struct {
	Return *BedirectServerInformationResult `xml:"return,omitempty" json:"return,omitempty" yaml:"return,omitempty"`
}

// .. cut

// information
func (p *soapServicePort) Information(KID string, IDSTRING string, BIPID string, VERWENDUNGSZWECK1 string, VERWENDUNGSZWECK2 string) (*BedirectServerInformationResult, error) {
	α := struct {
		M OperationInformationIn `xml:"tns:information"`
	}{}
	α.M.KID = &KID
	α.M.IDSTRING = &IDSTRING
	α.M.BIPID = &BIPID
	α.M.VERWENDUNGSZWECK1 = &VERWENDUNGSZWECK1
	α.M.VERWENDUNGSZWECK2 = &VERWENDUNGSZWECK2

	γ := struct {
		M OperationInformationOut `xml:"informationResponse"`
	}{}
	if err := p.cli.RoundTripWithAction("https://asd.bedirect.de/soap/2#information", α, &γ); err != nil {
		return nil, err
	}
	return γ.M._return, nil
}


// search
func (p *soapServicePort) Search(KID string, IDSTRING string, FIRMENNAME_GESAMT string, ORT string, PLZ string, STRASSE_GESAMT string, TELEFON_VORWAHL string, TELEFON_RUFNUMMER string, EMAIL string, URL string, VERSION string, HR_NUMMER string, HR_ORT string, HR_TYP string, CRF string, ZCRF string, RECHTSFORM_BD_TEXT string, PRIMAERBRANCHE string, SEKUNDAERBRANCHE string, MARKENBRANCHE string, PERSON string, ID string, UMSATZSTEUER_ID string, STEUERNUMMER string, STEUERNUMMER_ELSTER string, HANDELSNAME string, Q string, VERWENDUNGSZWECK1 string, VERWENDUNGSZWECK2 string) (*BedirectServerSearchResult, error) {
	α := struct {
		M OperationSearchIn `xml:"tns:search"`
	}{}
	α.M.KID = &KID
	α.M.IDSTRING = &IDSTRING
	α.M.FIRMENNAME_GESAMT = &FIRMENNAME_GESAMT
	α.M.ORT = &ORT
	α.M.PLZ = &PLZ
	α.M.STRASSE_GESAMT = &STRASSE_GESAMT
	α.M.TELEFON_VORWAHL = &TELEFON_VORWAHL
	α.M.TELEFON_RUFNUMMER = &TELEFON_RUFNUMMER
	α.M.EMAIL = &EMAIL
	α.M.URL = &URL
	α.M.VERSION = &VERSION
	α.M.HR_NUMMER = &HR_NUMMER
	α.M.HR_ORT = &HR_ORT
	α.M.HR_TYP = &HR_TYP
	α.M.CRF = &CRF
	α.M.ZCRF = &ZCRF
	α.M.RECHTSFORM_BD_TEXT = &RECHTSFORM_BD_TEXT
	α.M.PRIMAERBRANCHE = &PRIMAERBRANCHE
	α.M.SEKUNDAERBRANCHE = &SEKUNDAERBRANCHE
	α.M.MARKENBRANCHE = &MARKENBRANCHE
	α.M.PERSON = &PERSON
	α.M.ID = &ID
	α.M.UMSATZSTEUER_ID = &UMSATZSTEUER_ID
	α.M.STEUERNUMMER = &STEUERNUMMER
	α.M.STEUERNUMMER_ELSTER = &STEUERNUMMER_ELSTER
	α.M.HANDELSNAME = &HANDELSNAME
	α.M.Q = &Q
	α.M.VERWENDUNGSZWECK1 = &VERWENDUNGSZWECK1
	α.M.VERWENDUNGSZWECK2 = &VERWENDUNGSZWECK2

	γ := struct {
		M OperationSearchOut `xml:"searchResponse"`
	}{}
	if err := p.cli.RoundTripWithAction("https://asd.bedirect.de/soap/2#search", α, &γ); err != nil {
		return nil, err
	}
	return γ.M._return, nil
}

Errors:

src/asd/wsdl.go:318:13: γ.M._return undefined (type OperationInformationOut has no field or method _return)
src/asd/wsdl.go:362:13: γ.M._return undefined (type OperationSearchOut has no field or method _return)

The file in #65 looks good.

fiorix · 2017-12-28T11:07:37Z

Thanks for trying this, @Fank. No idea where that _return is coming from. Ping @kernle32dll.

Fank · 2017-12-28T11:39:19Z

Did a full test, with file from #65 and everything is working as expected.

kernle32dll · 2017-12-28T14:04:18Z

@Fank Good catch - Fixed. This was a tough one - I added a wsdl testcase to cover this. The linked wsdl now also generates correctly (I presume).

For the curious:
Wsdl2go did not correctly handle parameters which start with a small letter (return vs e.g. Return), and are a keyword at the same time. While the field names for the struct will be title'd (so that the fields are exposed and can be used outside the package) before the keyword check is performed, for parameters this was not the case. The correct casing would only be set during the templating of the soap function. However, as return was recognized as a golang keyword just before the templating, it was replaced to _return, and could not be correct re-cased to Return to access the field name.

On the bright side, trough the changes I had to make I was able to support funcs with more than one output (we don't have a test case for that, though).

Fank · 2017-12-28T14:35:31Z

Some test cases are still failing, and validated linked wsdl above, it is now working as expected

This fixes some issues with wsdl2go. Namely this does the following: - Allows zero-parameter functions - Allows functions with more than one parameter to work - Allow correct sending of RPC style requests - Fixes issues with keywords and casing - Fixes truncate problem on binding regeneration

kernle32dll · 2017-12-28T15:54:03Z

@fiorix @Fank Now with green tests :)

fiorix · 2017-12-28T22:10:47Z

Hey, this is pretty good, thanks much. It'll definitely help a lot of other people to get their soap services off the ground quicker.

fiorix · 2017-12-28T22:11:59Z

@adriantam lmk if this works for you too, or if it causes any trouble :)

Along the way of fiorix#75 the correct default value for an interface{} type was lost. This causes default returns such as &interface{}{}

Along the way of #75 the correct default value for an interface{} type was lost. This causes default returns such as &interface{}{}

adriantam · 2018-01-02T16:47:19Z

wsdlgo/encoder.go

-	return ge.genParams(req, true), nil
+
+	// TODO: I had to disable this for my use case - do other use cases still work with false?
+	return ge.genParams(req, false), nil


Instead of always hard code this to false, can we make it configurable?

adriantam · 2018-01-02T22:32:28Z

Hmm..it looks like the generated golden file cannot be XML encoded properly.
See https://play.golang.org/p/1aVTcYXYb2j (which is derived from https://raw.githubusercontent.com/fiorix/wsdl2go/master/wsdlgo/testdata/data.golden)

I will create an issue request to note that

Fank suggested changes Dec 27, 2017

View reviewed changes

fiorix requested changes Dec 27, 2017

View reviewed changes

kernle32dll force-pushed the basic-rpc-support-rebase branch 3 times, most recently from ae735ea to 4a7bcbb Compare December 27, 2017 17:56

kernle32dll force-pushed the basic-rpc-support-rebase branch 2 times, most recently from 244fc94 to fab41d1 Compare December 28, 2017 14:02

kernle32dll force-pushed the basic-rpc-support-rebase branch from fab41d1 to 0d7f1d9 Compare December 28, 2017 15:38

Fank approved these changes Dec 28, 2017

View reviewed changes

kernle32dll force-pushed the basic-rpc-support-rebase branch from 0d7f1d9 to 29285d6 Compare December 28, 2017 15:48

fiorix approved these changes Dec 28, 2017

View reviewed changes

fiorix merged commit 0e4e8dc into fiorix:master Dec 28, 2017

fiorix mentioned this pull request Dec 28, 2017

Namespaced operations. #9

Closed

Fank mentioned this pull request Dec 29, 2017

generated bad code: 104:11: expected expression (and 1 more errors) #78

Closed

kernle32dll added a commit to kernle32dll/wsdl2go that referenced this pull request Dec 29, 2017

Fix issue fiorix#78

faea7d9

Along the way of fiorix#75 the correct default value for an interface{} type was lost. This causes default returns such as &interface{}{}

kernle32dll mentioned this pull request Dec 29, 2017

Fix issue #78 #80

Merged

fiorix pushed a commit that referenced this pull request Dec 29, 2017

Fix issue #78

e8c98b4

Along the way of #75 the correct default value for an interface{} type was lost. This causes default returns such as &interface{}{}

adriantam reviewed Jan 2, 2018

View reviewed changes

adriantam mentioned this pull request Jan 2, 2018

Allow configuration on whether XMLName is generated #84

Closed

kernle32dll deleted the basic-rpc-support-rebase branch January 2, 2018 17:33

adriantam mentioned this pull request Jan 2, 2018

Non RPC function not able to be xml encoded properly #86

Closed

kernle32dll mentioned this pull request Jan 3, 2018

Fix #86 by fixing non-rpc request and response marshalling regression #88

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple fixes and rpc support draft #75

Multiple fixes and rpc support draft #75

kernle32dll commented Dec 23, 2017

Fank commented Dec 27, 2017 •

edited

Loading

Fank Dec 27, 2017

fiorix left a comment

fiorix Dec 27, 2017

fiorix Dec 27, 2017

fiorix Dec 27, 2017

fiorix Dec 27, 2017

fiorix Dec 27, 2017

fiorix Dec 27, 2017

fiorix commented Dec 27, 2017

kernle32dll commented Dec 27, 2017 •

edited

Loading

kernle32dll commented Dec 27, 2017

Fank commented Dec 27, 2017 •

edited

Loading

Fank commented Dec 28, 2017

fiorix commented Dec 28, 2017

Fank commented Dec 28, 2017

kernle32dll commented Dec 28, 2017

Fank commented Dec 28, 2017 •

edited

Loading

kernle32dll commented Dec 28, 2017

fiorix commented Dec 28, 2017

fiorix commented Dec 28, 2017

adriantam Jan 2, 2018

adriantam commented Jan 2, 2018

Multiple fixes and rpc support draft #75

Multiple fixes and rpc support draft #75

Conversation

kernle32dll commented Dec 23, 2017

Fank commented Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

fiorix 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

fiorix commented Dec 27, 2017

kernle32dll commented Dec 27, 2017 • edited Loading

kernle32dll commented Dec 27, 2017

Fank commented Dec 27, 2017 • edited Loading

Fank commented Dec 28, 2017

fiorix commented Dec 28, 2017

Fank commented Dec 28, 2017

kernle32dll commented Dec 28, 2017

Fank commented Dec 28, 2017 • edited Loading

kernle32dll commented Dec 28, 2017

fiorix commented Dec 28, 2017

fiorix commented Dec 28, 2017

Choose a reason for hiding this comment

adriantam commented Jan 2, 2018

Fank commented Dec 27, 2017 •

edited

Loading

kernle32dll commented Dec 27, 2017 •

edited

Loading

Fank commented Dec 27, 2017 •

edited

Loading

Fank commented Dec 28, 2017 •

edited

Loading