Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestions for more idiomatic code #106

Closed
anjmao opened this issue Oct 5, 2018 · 4 comments · Fixed by #107
Closed

Suggestions for more idiomatic code #106

anjmao opened this issue Oct 5, 2018 · 4 comments · Fixed by #107

Comments

@anjmao
Copy link
Collaborator

anjmao commented Oct 5, 2018

Hi,

  1. Current api could be improved a bit. Example from current generated soap client.
func NewSOAPClient(url string, insecureSkipVerify bool, auth *BasicAuth) *SOAPClient {
	tlsCfg := &tls.Config{
		InsecureSkipVerify: insecureSkipVerify,
	}
	return NewSOAPClientWithTLSConfig(url, tlsCfg, auth)
}

func NewSOAPClientWithTLSConfig(url string, tlsCfg *tls.Config, auth *BasicAuth) *SOAPClient {
	return &SOAPClient{
		url:    url,
		tlsCfg: tlsCfg,
		auth:   auth,
	}
}

This is not very friendly api to use. Instead it's a common practice in go to use ...option. These 2 functions which are both not flexible can be refactored to

func NewSOAPClient(opt ...SOAPOption) *SOAPClient {
	opts := defaultSOAPOptions
	for _, o := range opt {
		o(&opts)
	}
        return &SOAPClient{
		url:    opts.url,
		tlsCfg: opts.tlsCfg,
		auth:   opts.auth,
	}
}

Usage

client := NewSOAPClient(
       gowsdl.URL('myservice.asmx'), 
       gowsdl.TLSCfg(....),
       gowsdl.Auth(...),
       gowsdl.SothingElse(...)
)

It will also allow to stay backwards compatible add be able to add new config options in the feature.

  1. It's not possible to set global request headers, for example there is hardcoded req.Header.Set("User-Agent", "gowsdl/0.1"), but service which I want to call checks if user-agent is name-x otherwise to throws unauthorised fault error.
    This can be implemented simply by having SOAPOption RequestHeaders.

  2. There are log.Println statments inside code https://github.com/hooklift/gowsdl/blob/master/soap_tmpl.go#L268. This is bad when using docker and kubernetes. For example in my services all stdout logs a automatically pushed elastic search(kibana). In general print statements should be used only on some tracing mode enabled.

@c4milo
Copy link
Member

c4milo commented Oct 5, 2018

I welcome all the above improvements @anjmao.

@anjmao
Copy link
Collaborator Author

anjmao commented Oct 5, 2018

Here is some work I already did in my branch https://github.com/trackforce/gowsdl/tree/refactor-soap-client.

Idea is to pass soap client to each generated service so there is no complex call logic inside generated files.
I will probably finish on Monday.

@c4milo
Copy link
Member

c4milo commented Oct 5, 2018

@anjmao I like those changes. We should probably initialize a go module for this library and tag it before introducing breaking changes.

@anjmao
Copy link
Collaborator Author

anjmao commented Oct 8, 2018

@c4milo I Agree for modules. I'm currently testing library with my changes by calling some of our production soap services, but I'm getting XML marshal/unmarshal errors. I added comment in my PR.

anjmao added a commit that referenced this issue Oct 9, 2018
* Generate public service interfaces
* Move soap client to separate package
* Add example folder

closes #106
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants