search

DivideBV / Postnl

Library to connect to PostNL's SOAP service called CIF
GNU General Public License v2.0
31 stars 40 forks source link

PostNL switched to one exception data object. #62

Closed kim-dongit closed 5 years ago

kim-dongit commented 5 years ago

I noticed this change on PostNL's new platform. I have asked them if they have a list of all changes compared to the old platform, to make our lives easier. I have not discovered any documentation on this myself, however a support employee has admitted that there are some "accepted differences", i.e. accepted by PostNL.

Er zijn inderdaad een aantal "geaccepteerde" verschillen tussen de nieuwe en de oude API. Deze zijn geaccepteerd omdat ze geen impact hebben op klanten en zaken duidelijker maken (waar er eerst drie foutcodes kwamen, krijg je er nu bijv. maar één).

ameenross commented 5 years ago

Arg, this is exactly what I meant by not wanting to take action before they provide proper documentation.

ameenross commented 5 years ago

For which service is this exactly? I've just received errors from the Confirming webservice, and they're still an array.

kim-dongit commented 5 years ago

Thanks for the feedback. Changed the code to accept both an array of objects and a single object. (It simply puts the single object in an array, as it should for ComplexTypes\ArrayOfExceptionData.)

At least following caused a single object as exception data when an incorrect API key was provided:

I am not even sure if each service is consistent by itself. It might be that if the response contains 1 error it's a single object, whereas if there are multiple, an array is returned. I will call them today as I have not heard of them on the point of the list of changes. (Tried inpecting the wdsls and found no specification of CifExceptionFault.)

Problem is that PostNL will discontinue the old platform as per 1 November, so I would like to move forward. Hope that they will support us.

ameenross commented 5 years ago

Well, I'm not going to rush things because PostNL doesn't get their act together. So far the new environment has caused more problems than I anticipated for something as self-contained as an auth method change.

ameenross commented 5 years ago

This is the bit about CifException from the confirming WSDL:

<xs:complexType name="CifException">
  <xs:sequence>
    <xs:element minOccurs="0" name="Errors" nillable="true" type="tns:ArrayOfExceptionData"/>
  </xs:sequence>
</xs:complexType>

<xs:element name="CifException" nillable="true" type="tns:CifException"/>

<xs:complexType name="ArrayOfExceptionData">
  <xs:sequence>
    <xs:element minOccurs="0" maxOccurs="unbounded" name="ExceptionData" nillable="true" type="tns:ExceptionData"/>
  </xs:sequence>
</xs:complexType>

<xs:element name="ArrayOfExceptionData" nillable="true" type="tns:ArrayOfExceptionData"/>

<xs:complexType name="ExceptionData">
  <xs:sequence>
    <xs:element minOccurs="0" name="Description" nillable="true" type="xs:string"/>
    <xs:element minOccurs="0" name="ErrorMsg" nillable="true" type="xs:string"/>
    <xs:element minOccurs="0" name="ErrorNumber" nillable="true" type="xs:string"/>
  </xs:sequence>
</xs:complexType>

<xs:element name="ExceptionData" nillable="true" type="tns:ExceptionData"/>

And this is from the Barcode WSDL:

<xs:complexType name="CifException">
  <xs:sequence>
    <xs:element minOccurs="0" name="Errors" nillable="true" type="tns:ArrayOfExceptionData"/>
  </xs:sequence>
</xs:complexType>

<xs:element name="CifException" nillable="true" type="tns:CifException"/>

<xs:complexType name="ArrayOfExceptionData">
  <xs:sequence>
    <xs:element minOccurs="0" maxOccurs="unbounded" name="ExceptionData" nillable="true" type="tns:ExceptionData"/>
  </xs:sequence>
</xs:complexType>

<xs:element name="ArrayOfExceptionData" nillable="true" type="tns:ArrayOfExceptionData"/>

<xs:complexType name="ExceptionData">
  <xs:sequence>
    <xs:element minOccurs="0" name="Description" nillable="true" type="xs:string"/>
    <xs:element minOccurs="0" name="ErrorMsg" nillable="true" type="xs:string"/>
    <xs:element minOccurs="0" name="ErrorNumber" nillable="true" type="xs:string"/>
  </xs:sequence>
</xs:complexType>

<xs:element name="ExceptionData" nillable="true" type="tns:ExceptionData"/>

They are the same. However, looking at the one from the Timeframe Client, which according to you should be different:

<xs:complexType name="CifException">
  <xs:sequence>
    <xs:element minOccurs="0" name="Errors" nillable="true" type="tns:ArrayOfExceptionData"/>
  </xs:sequence>
</xs:complexType>

<xs:element name="CifException" nillable="true" type="tns:CifException"/>

<xs:complexType name="ArrayOfExceptionData">
  <xs:sequence>
    <xs:element minOccurs="0" maxOccurs="unbounded" name="ExceptionData" nillable="true" type="tns:ExceptionData"/>
  </xs:sequence>
</xs:complexType>

<xs:element name="ArrayOfExceptionData" nillable="true" type="tns:ArrayOfExceptionData"/>

<xs:complexType name="ExceptionData">
  <xs:sequence>
    <xs:element minOccurs="0" name="Description" nillable="true" type="xs:string"/>
    <xs:element minOccurs="0" name="ErrorMsg" nillable="true" type="xs:string"/>
    <xs:element minOccurs="0" name="ErrorNumber" nillable="true" type="xs:string"/>
  </xs:sequence>
</xs:complexType>

<xs:element name="ExceptionData" nillable="true" type="tns:ExceptionData"/>

It also looks the same... So are they violating their own WSDLs?

kim-dongit commented 5 years ago

You are right on all occasions (the simple change which is not, the violation of their own specifications, and not hurrying). I found another wsdl violation.

In https://api.postnl.nl/shipment/v2_1/locations/soap.wsdl:

<xs:element minOccurs="0" name="OpeningTime" nillable="true" type="xs:string">

While a request with null for OpeningTime (lib's default):

<?xml version="1.0" encoding="UTF-8"?>
<SOAP-ENV:Envelope xmlns:SOAP-ENV="http://schemas.xmlsoap.org/soap/envelope/" xmlns:ns1="http://postnl.nl/cif/domain/LocationWebService/" xmlns:ns2="http://schemas.microsoft.com/2003/10/Serialization/Arrays" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:ns3="http://postnl.nl/cif/services/LocationWebService/">
    <SOAP-ENV:Body>
        <ns3:GetNearestLocations>
            <ns1:Countrycode>NL</ns1:Countrycode>
            <ns1:Location>
                <ns1:AllowSundaySorting>true</ns1:AllowSundaySorting>
                <ns1:DeliveryDate>03-10-2018</ns1:DeliveryDate>
                <ns1:DeliveryOptions>
                    <ns2:string>PG</ns2:string>
                </ns1:DeliveryOptions>
                <ns1:OpeningTime xsi:nil="true"/>
                <ns1:Options>
                    <ns2:string>Daytime</ns2:string>
                </ns1:Options>
                <ns1:City xsi:nil="true"/>
                <ns1:Coordinates xsi:nil="true"/>
                <ns1:HouseNr xsi:nil="true"/>
                <ns1:HouseNrExt xsi:nil="true"/>
                <ns1:Postalcode>2316XC</ns1:Postalcode>
                <ns1:Street xsi:nil="true"/>
            </ns1:Location>
            <ns1:Message>
                <ns1:MessageID>1</ns1:MessageID>
                <ns1:MessageTimeStamp>02-10-2018 16:20:23</ns1:MessageTimeStamp>
            </ns1:Message>
        </ns3:GetNearestLocations>
    </SOAP-ENV:Body>
</SOAP-ENV:Envelope>

Triggers an error stating that OpeningTime should not be null:

<?xml version="1.0" encoding="UTF-8"?>
<s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/">
<s:Body>
    <s:Fault>
        <faultcode>s:CIF Framework Message Interceptor</faultcode>
        <faultstring xml:lang="en-US">Check CIFException in the detail section</faultstring>
        <detail>
            <CifException xmlns="http://postnl.nl/cif/services/common/" xmlns:i="http://www.w3.org/2001/XMLSchema-instance">
                <Errors>
                    <ExceptionData>
                        <Description i:nil="true"/>
                        <ErrorMsg>Parameter 'Location.OpeningTime' may not be null.</ErrorMsg>
                        <ErrorNumber>7</ErrorNumber>
                    </ExceptionData>
                </Errors>
            </CifException>
        </detail>
    </s:Fault>
</s:Body>
</s:Envelope>

On the other hand another inconsistency in the old platform was fixed. I had to mend old code from my colleague. He did a PostNl::getLocation()::getGetLocationsResult()::getResponseLocation() which should have returned an array (ResponseLocation[]), however he was able to call a ->getAddress() on it. The result on the old platform must have been a ResponseLocation instead of a ResponseLocation[], which is also assumed in the lib.

I have decided on doing workarounds for the code I need, unless PostNL shows up with quick and adequate responses.

ameenross commented 5 years ago

however he was able to call a ->getAddress()

That is probably because of the way PHP SoapClient handles single element arrays. This has been changed in 2.x with #32

Can you post raw request and response of the TimeframeClient with the CifException?

kim-dongit commented 5 years ago

That is probably because of the way PHP SoapClient handles single element arrays. This has been changed in 2.x with #32

Thanks for pointing that out. I have not taken changes in the library into account. Seems like a sure candidate for documenting in a migration path section/document, .

kim-dongit commented 5 years ago

Can you post raw request and response of the TimeframeClient with the CifException?

Request:

<?xml version="1.0" encoding="UTF-8"?>
<SOAP-ENV:Envelope xmlns:SOAP-ENV="http://schemas.xmlsoap.org/soap/envelope/" xmlns:ns1="http://postnl.nl/cif/domain/TimeframeWebService/" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:ns2="http://schemas.microsoft.com/2003/10/Serialization/Arrays" xmlns:ns3="http://postnl.nl/cif/services/TimeframeWebService/">
  <SOAP-ENV:Body>
    <ns3:GetTimeframes>
      <ns1:Message>
        <ns1:MessageID>1</ns1:MessageID>
        <ns1:MessageTimeStamp>03-10-2018 10:35:31</ns1:MessageTimeStamp>
      </ns1:Message>
      <ns1:Timeframe>
        <ns1:City xsi:nil="true"/>
        <ns1:CountryCode>NL</ns1:CountryCode>
        <ns1:EndDate>11-10-2018</ns1:EndDate>
        <ns1:HouseNr>103</ns1:HouseNr>
        <ns1:HouseNrExt xsi:nil="true"/>
        <ns1:Options>
          <ns2:string>Daytime</ns2:string>
          <ns2:string>Evening</ns2:string>
        </ns1:Options>
        <ns1:PostalCode>2316XC</ns1:PostalCode>
        <ns1:StartDate>04-10-2018</ns1:StartDate>
        <ns1:Street xsi:nil="true"/>
        <ns1:SundaySorting>true</ns1:SundaySorting>
      </ns1:Timeframe>
    </ns3:GetTimeframes>
  </SOAP-ENV:Body>
</SOAP-ENV:Envelope>

Response, changed formatting (whitespace) for readability:

<?xml version="1.0"?>
<s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/">
    <s:Body>
        <s:Fault>
            <faultcode>s:CIF Framework Message Interceptor</faultcode>
            <faultstring xml:lang="en-US">Check CIFException in the detail section</faultstring>
            <detail>
                <ns0:CifException xmlns:ns0="http://postnl.nl/cif/services/common/"
                                  xmlns:i="http://www.w3.org/2001/XMLSchema-instance">
                    <ns1:Errors xmlns:ns1="http://postnl.nl/cif/services/common/">
                        <ns2:ExceptionData xmlns:ns2="http://postnl.nl/cif/services/common/">
                            <ns3:Description xmlns:ns3="http://postnl.nl/cif/services/common/" i:nil="true"/>
                            <ns4:ErrorMsg xmlns:ns4="http://postnl.nl/cif/services/common/">The apikey is not authorized
                                for this api
                            </ns4:ErrorMsg>
                            <ns5:ErrorNumber xmlns:ns5="http://postnl.nl/cif/services/common/">2</ns5:ErrorNumber>
                        </ns2:ExceptionData>
                    </ns1:Errors>
                </ns0:CifException>
            </detail>
        </s:Fault>
    </s:Body>
</s:Envelope>
ameenross commented 5 years ago

Seems like a sure candidate for documenting in a migration path section/document

Yes, that's the only change besides auth though. It's something that had been pending for a while, but merging the PR would have broken backward compatibility. So the auth change (which also breaks BC) was an opportunity to merge that.

ameenross commented 5 years ago

Response, changed formatting (whitespace) for readability:

Looks like they stick to their structure, they might just have merged the content of multiple errors. That's not a big deal for the library though, only potentially for your own business logic.

I guess that makes this PR invalid.

kim-dongit commented 5 years ago

Looks like they stick to their structure, they might just have merged the content of multiple errors. That's not a big deal for the library though, only potentially for your own business logic.

I guess that makes this PR invalid.

Maybe you are right about PostNL complying with their wsdl in this case. however, I do no think the PR is invalid. The library should handle the response properly, but currently does not, so it is a big deal. I found that the old code already contained the last change I proposed, however, it was removed in Use SOAP_SINGLE_ELEMENT_ARRAYS. I guess the change in src/Postnl.php should be reverted, to be able to handle live data.

ameenross commented 5 years ago

That code was necessary because PHP's Soap stack would return Object instead of [Object], when there's only 1 error. With SOAP_SINGLE_ELEMENT_ARRAYS that should no longer be necessary. Although you're right, I'm getting an error.

I'll try to find out what I'm missing

kim-dongit commented 5 years ago

I think I have found the answer. To know whether something should be parsed as an array, the SoapClient should have an XSD, however PostNL does not return it in a schemaLocation attribute. I was already wondering how you found the XSDs in https://github.com/DivideBV/Postnl/pull/62#issuecomment-426313200 (could not find them in PostNL's documentation either).

kim-dongit commented 5 years ago

I think Use SOAP_SINGLE_ELEMENT_ARRAYS should be reverted as a consequence.

ameenross commented 5 years ago

To know whether something should be parsed as an array, the SoapClient should have an XSD, however PostNL does not return it in a schemaLocation attribute.

The XSDs are referred to in the WSDL, however, the WSDL points to the old domain, so it could well be that it's currently broken. What's your idea: https://api.postnl.nl/shipment/v1_1/barcode/soap.wsdl

If that's the problem, this is a won't fix. Then PostNL has to fix their WSDLs, which are truly messy even if they weren't broken....

kim-dongit commented 5 years ago

Could you elaborate on how the XSDs are referred to in the WDSL? I could not find them. I also did not find the old domain (test)service.postnl.com in the WDSL.

ameenross commented 5 years ago

This is the URL of the barcode service (new env): https://api.postnl.nl/shipment/v1_1/barcode/soap.wsdl

Funny, it contains another URL now, I'm pretty sure it contained this earlier today (unless I'm losing it):

<wsdl:import namespace="https://www.postnl.nl/cif/services/BarcodeWebService/" location="BarcodeWebService_1.wsdl"/>

Turns out the URL http://postnl.nl/cif/services/BarcodeWebService/BarcodeWebService_1.wsdl redirects to https://www.postnl.nl/cif/services/BarcodeWebService/BarcodeWebService_1.wsdl which is a 404.

Now the entire content is:

<wsdl:definitions name="BarcodeWebService" targetNamespace="http://tempuri.org/">
  <wsdl:import namespace="http://postnl.nl/cif/services/BarcodeWebService/" location="BarcodeWebService_1.wsdl"/>
  <wsdl:types/>
  <wsdl:binding name="BasicHttpBinding_IBarcodeWebService" type="i0:IBarcodeWebService">
    <soap:binding transport="http://schemas.xmlsoap.org/soap/http"/>
    <wsdl:operation name="GenerateBarcode">
      <soap:operation soapAction="http://postnl.nl/cif/services/BarcodeWebService/IBarcodeWebService/GenerateBarcode" style="document"/>
      <wsdl:input name="GenerateBarcodeRequestContract">
        <soap:body use="literal"/>
      </wsdl:input>
      <wsdl:output name="GenerateBarcodeResponseContract">
        <soap:body use="literal"/>
      </wsdl:output>
      <wsdl:fault name="CifExceptionFault">
        <soap:fault name="CifExceptionFault" use="literal"/>
      </wsdl:fault>
    </wsdl:operation>
  </wsdl:binding>
  <wsdl:service name="BarcodeWebService">
    <wsdl:port name="BasicHttpBinding_IBarcodeWebService" binding="tns:BasicHttpBinding_IBarcodeWebService">
      <soap:address location="https://api.postnl.nl/shipment/v1_1/barcode/"/>
    </wsdl:port>
  </wsdl:service>
</wsdl:definitions>

There's still http://tempuri.org/ in there though. For shame.

Also, here's the WSDL for the sandbox env of the labelling service: https://api-sandbox.postnl.nl/shipment/v2_0/label/soap.wsdl

<wsdl:definitions name="LabellingWebService" targetNamespace="http://tempuri.org/">
  <wsdl:import namespace="http://postnl.nl/cif/services/LabellingWebService/" location="LabellingWebService_1.wsdl"/>
  <wsdl:types/>
  <wsdl:binding name="BasicHttpBinding_ILabellingWebService" type="i0:ILabellingWebService">
    <soap:binding transport="http://schemas.xmlsoap.org/soap/http"/>
    <wsdl:operation name="GenerateLabel">
      <soap:operation soapAction="http://postnl.nl/cif/services/LabellingWebService/ILabellingWebService/GenerateLabel" style="document"/>
      <wsdl:input name="GenerateLabelRequestContract">
        <soap:body use="literal"/>
      </wsdl:input>
      <wsdl:output name="GenerateLabelResponseContract">
        <soap:body use="literal"/>
      </wsdl:output>
      <wsdl:fault name="CifExceptionFault">
        <soap:fault name="CifExceptionFault" use="literal"/>
      </wsdl:fault>
    </wsdl:operation>
    <wsdl:operation name="GenerateLabelWithoutConfirm">
      <soap:operation soapAction="http://postnl.nl/cif/services/LabellingWebService/ILabellingWebService/GenerateLabelWithoutConfirm" style="document"/>
      <wsdl:input name="GenerateLabelRequestContract">
        <soap:body use="literal"/>
      </wsdl:input>
      <wsdl:output name="GenerateLabelResponseContract">
        <soap:body use="literal"/>
      </wsdl:output>
      <wsdl:fault name="CifExceptionFault">
        <soap:fault name="CifExceptionFault" use="literal"/>
      </wsdl:fault>
    </wsdl:operation>
  </wsdl:binding>
  <wsdl:service name="LabellingWebService">
    <wsdl:port name="BasicHttpBinding_ILabellingWebService" binding="tns:BasicHttpBinding_ILabellingWebService">
      <soap:address location="https://api-sandbox.postnl.nl/shipment/v2_0/label/"/>
    </wsdl:port>
  </wsdl:service>
</wsdl:definitions>

Is contains a link to http://postnl.nl/cif/services/LabellingWebService/LabellingWebService_1.wsdl, which is a 404.

kim-dongit commented 5 years ago

If that's the problem, this is a won't fix. Then PostNL has to fix their WSDLs, which are truly messy even if they weren't broken....

Depends on whether XSDs were available on the old platform, I guess. It did not work there either, I just tested. (I used the old wsdl in the new code, however also with SOAP_SINGLE_ELEMENT_ARRAYS, the single ExceptionData was not parsed as a single element array.)

I think the change itself makes a lot of sense, however only if the XSDs are made available to the SoapClient. That is not the case. PostNL might be persuaded to make it work. However, I guess they will only do that in an update as it did not work previously. So if we do not fix, stable release will have to wait for that and we will have to implement all other changes of the update. I suppose that complicates matters, while reverting to the previous code seems pretty simple.

ameenross commented 5 years ago

32 will not be reverted. This is because more than just the SoapFaults benefit from it. The fact that the WSDL is broken can only be an argument to put a workaround in place for this specific error. And that doesn't require reverting #32

kim-dongit commented 5 years ago

You are entirely right. I noticed it too, but had to run. (Re-)added the simple fix. Left all the commits with reverts, but all can be squashed resulting into the last.

ameenross commented 5 years ago

After a night's rest having a fresh look at it. In fact I wrote that try/catch converting the stdClasses to class instances because SoapClient does not seem to do it - for whatever reason. SoapClient does know about the complextypes, because responses are converted to class instances perfectly fine. It's only the SoapFaults that aren't.

Whether that is because of how SoapClient works internally or because the WSDL is wrong is something I have no interest in trying to find out now. I already wasted too much time on this.

I did find out a bit more about how XSD importing works (I didn't exactly memorize the XSD spec - frankly I think XML is overly complex, especially for what it's mostly used for in practice).

kim-dongit commented 5 years ago

Thank you for your time. Same for me: too much time and agree with overcomplication of SOAP in particular.