Work around buggy encloding/xml ns attribute marshalling #218
Conversation
Go’s `encoding/xml` handling of namespaces is deeply broken. In particular, it can’t marshal XML using namespace prefixes, ala:
```xml
<document xmlns:ns="whatever">
<ns:node ns:type="foo">…</ns:node>
</document>
```
Only by setting the default namespace, ala:
```xml
<document xmlns="whatever">
<node type="foo">…</node>
</document>
```
This is broken. Per [the XML Specification](https://www.w3.org/TR/xml-names/#dt-defaultNS), default namespaces don’t affect attributes; they **MUST** have a prefix:
> The namespace name for an unprefixed attribute name always has no value.
This causes any attributes in WSDL requests to end up in the wrong namespace, which makes them fail validation & return a SOAP fault.
The only way to get attributes into the correct namespace is to include the namespace in the `xml:"…"` struct field, ala:
```go
Type string `xml:"http://namespace.uri/whatever type,attr,omitempty" json:"type,omitempty"`
```
This has the unfortunate side-effect of generating overly verbose output:
```xml
<document xmlns="whatever">
<node xmlns:ns="whatever" ns:type="foo">…</node>
</document>
```
However, it’s actually semantically correct, therefore an improvement over the current situation. A proper fix depends on `encoding/xml` being less hilariously wrong, which is out of scope here.
The implementation approach also leaves much to be desired. The crux of the issue is that attributes are generated in a sub-template which is called from multiple places (the schema, complex types, etc). The `XSDAttribute` containing the attr information used by that template doesn’t — and shouldn’t — include the namespace it’s scoped under. Ideally, I’d be able to thread the schema’s namespace down through the templates, but of course `text/template` doens’t support passing more than one argument to any template. Rather than create a bunch of types containing the namespace and whatever data that template needs (or one using namespace and `interface{}), and functions to allocate those to pass to the sub-templates, I opted to use mutable state. `GoWSDL` now has `currentNamespace` and get/set methods, which allow accessing the ns from the attribute template. This is not a very sound approach to the problem, in my opinion, but it seems like a smart trade to opt for concrete implementation concerns over abstract ideological ones.
|
Thanks for the well researched context @ieure and for the patch! I will try to get to it this week. 🙏 |
|
@c4milo Thanks for the reply. If there's any chance of getting this and #214 merged and a new release cut, that'd be terrific. Those two, and the fix from #208, are blockers for my usecase. For now, I've vendored 9e1cc9a with patches for the two PRs I opened, but I'd like very much to use a clean tagged release. Having this stuff is a priority for my employer, so if any changes on either PR are needed, I can turn those around quickly. |
|
OK, that’s good to know, I will review them tomorrow then.
On Mon, Sep 13, 2021 at 10:15 PM Ian Eure ***@***.***> wrote:
@c4milo <https://github.com/c4milo> Thanks for the reply. If there's any
chance of getting this and #214
<#214> merged and a new release
cut, that'd be terrific. Those two, and the fix from #208
<#208>, are blockers for my
usecase. For now, I've vendored 9e1cc9a
<9e1cc9a>
with patches for the two PRs I opened, but I'd like very much to use a
clean tagged release.
Having this stuff is a priority for my employer, so if any changes on
either PR are needed, I can turn those around quickly.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#218 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAKFUNK4VTQCPUZC3X6IBTUB2V43ANCNFSM5D6N4G3Q>
.
--
*Camilo Aguilar*
Software Engineer
http://github.com/c4milo
|
|
Have you had a chance to take a look at this? |
|
@ieure, I got caught up by end of quarter deadlines but once I'm out of the bushes I will look into this. Apologies! |
|
Not sure if it would really help. But this describes an approach to actually pass multiple arguments to a Go template using a dictionary helper: https://stackoverflow.com/questions/18276173/calling-a-template-with-several-pipeline-parameters. It is clearly a workaround to Go's template implementation. But given the circumstances, I think it is a pretty nice and good "solution". |
|
FYI, I have described another namespace related problem with naming collisions in issue #223 The problem had already been identified before (#184). I have attempted to give a more detailed and broader perspective on the problem. In particular I am hoping that by defining the "correct" approach for handling namespaces, we should be able to address as many problems and as much functionality as possible in one go. |
Thanks for the context. That looks like a different issue than what I'm discussing here. The root cause of the issue I laid out in this PR is that Go's For #223, the issue is 100% gowsdl. It should probably put each ns' schema into its own subpackage based on its ns name, so you have |
|
@ieure Thanks for your clear statement. Makes sense. |
Go’s
encoding/xmlhandling of namespaces is deeply broken. In particular, it can’t marshal XML using namespace prefixes, ala:Only by setting the default namespace, ala:
This is broken. Per the XML Specification, default namespaces don’t affect attributes; they MUST have a prefix:
This causes any attributes in WSDL requests to end up in the wrong namespace, which makes them fail validation & return a SOAP fault.
The only way to get attributes into the correct namespace is to include the namespace in the
xml:"…"struct field, ala:This has the unfortunate side-effect of generating overly verbose output:
However, it’s actually semantically correct, therefore an improvement over the current situation. A proper fix depends on
encoding/xmlbeing less hilariously wrong, which is out of scope here.The implementation approach also leaves much to be desired. The crux of the issue is that attributes are generated in a sub-template which is called from multiple places (the schema, complex types, etc). The
XSDAttributecontaining the attr information used by that template doesn’t — and shouldn’t — include the namespace it’s scoped under. Ideally, I’d be able to thread the schema’s namespace down through the templates, but of coursetext/templatedoens’t support passing more than one argument to any template. Rather than create a bunch of types containing the namespace and whatever data that template needs (or one using namespace andinterface{}), and functions to allocate those to pass to the sub-templates, I opted to use mutable state.GoWSDLnow hascurrentNamespaceand get/set methods, which allow accessing the ns from the attribute template. This is not a very sound approach to the problem, in my opinion, but it seems like a smart trade to opt for concrete implementation concerns over abstract ideological ones.