DescriptionElement.getTypesElement() should not be a factory method

• Key: WODEN-141
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Dan Harvey
• Reporter: John Kaputin
• Component/s: Parser
• Fix Version/s: M8

Description

The behaviour of the DescriptionElement.getTypesElement() method is to return the types element if it exists or if not, create it and return it. This means that the model can never reflect an infoset that does not have a <wsdl:types> element because this getter method will always create one even if there isn't one in the WSDL.

Instead, this method should return null if there is no types element and a new factory method is required - createTypesElement(), which will create a TypesElement, set the DescriptionElement as its parent and return a reference to it. This is similar behaviour to the addXXXXElement methods that create sets of child elements, except that it acts on a single types element only (hence the method name 'create' rather than 'add').

Comments

[Dan Harvey]

As only one TypesElement can exist in a DescriptionElement in the WSDL 2.0 Spec the createTypesElement() factory method has to decide what to do if a TypesElement already exists in the DescriptionElement.

I have agreed with John that we will just replace the existing TypesElement if createTypesElement() is called and have put this behaviour in the JavaDoc comment for the createTypesElement() method interface.

[Jeremy Hughes]

Some time ago, we removed the 'createXXX' in favour of 'addXXX'. We resolved then that createXXX shouldn't have side-effects - so a createXXX method would create an XXX and nothing else. Then we would have needed addXXX methods to add the XXX onto the DescriptionElement. This seemed a bit cumbersome so we resolved to have addXXX create *and* add XXX which followed a pattern in a JSR (sorry forget the number/ technology ... I can dig it up if you like).

Since there can only be one types element we decided it wasn't the right thing to have an addTypesElement method. I can see the problem this JIRA is trying to solve - how to determine whether there's a types element and if there is one how to remove it. Clearly in checking whether there's one there we don't want to create one if we didn't want one. So IMO #1 getTypesElement should return null if one doesn't exist. This kinda mirrors the other getXXX methods (except those return empty array).

We're going to need removeXXX methods - there's a comment in DescriptionElement:

    /*
     * Element accessor and modifier methods
     *
     * TODO removeXXX(obj), getXXX(key) methods
     *
     */

so we're going to have a removeTypesElement() (note: doesn't need param as there is only 0 or 1 of them). So to have createTypesElement/removeTypesElement when everything else is addXXX/removeXXX doesn't feel so neat. So #2 I'd prefer createTypesElement() to be called addTypesElement() which would (#3) throw an exception if one is already there.

So
#1 getTypesElement() returns null if there is none
#2 addTypesElement() instead of createTypesElement()
#3 addTypesElement() throws WSDLException if there is one already

What do you all think?

[Dan Harvey]

#1 Yes.

#2 If you've agreed createXXX should not have side-effects then addXXX should be used and more consistent with the rest of the class.

#3 In keeping with the word add and the way the rest of the class uses it we should never overwrite or remove old TypesElements, so that would be a good way to do it.

If no one disagrees I'll create the changes in the way Jeremy described above.

[Dan Harvey]

For reference the convention of add/removeXXX was agreed in WODEN-40 .

[Dan Harvey]

The only thing I'm not sure about with this is what to do when we already have a TypesElement. Do we throw an exception or return null? As this is not done anywhere else there is not really anything else to be consistent with.

Returning null would be very uninformative as there could be various reasons that has happened.

Throwing an exception would make most sense as at runtime the developers can react to the error on their half for not checking that a TypesElement doesn't already exists, using getTypesElement.

So for now I will throw a WSDLException with a message describing the problem.

[John Kaputin]

Dan has contributed the fix, so assigning the JIRA to him.

[John Kaputin]

Slight changes and additions to Dan's patches.

[John Kaputin]

Minor change to Dan's patch.

[John Kaputin]

I have made the following minor changes/additions to Dan's patches:

BaseWSDLReader.java.2.patch:
There may only be 1 wsdl:types element in a wsdl:description, so in parseTypes() we can just add a (new) TypesElement and if one already exists (i.e we have already parsed a wsdl:types) then a WSDLException is thrown.
replaced;
        TypesElement types = desc.getTypesElement();
        if (types == null) {
            types = desc.addTypesElement();
        }
with;
        TypesElement types = desc.addTypesElement();


DescriptionImpl.java.2.patch:
In the addTypesElement() method replaced hardcoded string msg with new formatted msg WSDL523 and added WSDL523 to Messages.properties

DescriptionTest.java.patche:
In the setUp() method changed fDescriptionElement.getTypesElement() to addTypesElement()

[John Kaputin]

r563158

[John Kaputin]

I think there is still a problem how Woden handles the wsdl:types element, but it's tied to a problem with the WSDL 2.0 spec so I will raise a bugzilla against the spec and hopefully the new W3C Web Services core working group being formed around September 2007 will address it.

The problem with Woden is that it throws a 'duplicate types element' WSDLException if DescriptionElement.addTypesElement is called more than once. This means that parsing will stop on this error. The general behaviour of Woden is to parse the entire WSDL document, even if it has errors, report any schema validation errors or, if no schema errors exist, then to validate ALL wsdl assertions and report any assertion errors. That is, woden doesn't stop parsing when it hits a WSDL error. However, it will now stop parsing if a wsdl:description element contains more than one wsdl:types element.

The problem with the spec is that the WSDL 2.0 schema permits multiple wsdl:types element, so we can't detect this error during schema validation. And although the spec states that the wsdl:description may have only ONE wsdl:types element, it does not capture an assertion for this, so at the moment we can't pick up this error via assertion validation.

I think the solution might be to change the WSDL 2.0 schema to restrict wsdl:description to a single wsdl:types element. I'll report this via W3C bugzilla for now. This JIRA can remain 'resolved'. I will open a new Woden JIRA to keep track of the bugzilla and any follow-up woden action required.