OxalisCommunity / oxalis

Oxalis - PEPPOL Access Point open source implementation - Core component
Other
129 stars 91 forks source link

SMP lookup #118

Closed monze closed 6 years ago

monze commented 10 years ago

We are using Oxalis version 3.0 (the trunk). we are able to both send and receive messag without problem. I have a comment on how the exception is handled in some classes. Don't know if it has been discussed before, and it actual is by design. And I'm a c# developer, so don't know to much about Java code styling.

Please look at the class eu.peppol.smp.SmpLookupManagerImpl in the method getEndpointType listed below.

private EndpointType getEndpointType(ParticipantId participant, PeppolDocumentTypeId documentTypeIdentifier) {
    try {
        SignedServiceMetadataType serviceMetadata = getServiceMetaData(participant, documentTypeIdentifier);
        return selectOptimalEndpoint(serviceMetadata);
    } catch (Exception e) {
        throw new RuntimeException("Problem with SMP lookup", e);
    }
}

Every time the SML/SMP lookups fails, a new RuntimeException with the message "Problem with SMP lookup" is return. Even in situation where a concrete SMP exception has been created, and returned. An example is this eu.peppol.smp.SmpSignedServiceMetaDataException, with the 'good' error message:

Unable to find information for participant: 9908:810017901, documentType: urn:oasis:names:specification:ubl:schema:xsd:Invoice-2::Invoice##urn:www.cenbii.eu:transaction:biicoretrdm010:ver1.0:#urn:www.peppol.eu:bis:peppol4a:ver1.0#urn:www.difi.no:ehf:faktura:ver1::2.0, at url: http://B-97ecb4d561d466396597c8f2566561f5.iso6523-actorid-upis.sml.peppolcentral.org/iso6523-actorid-upis%3A%3A9908%3A810017901/services/busdox-docid-qns%3A%3Aurn%3Aoasis%3Anames%3Aspecification%3Aubl%3Aschema%3Axsd%3AInvoice-2%3A%3AInvoice%23%23urn%3Awww.cenbii.eu%3Atransaction%3Abiicoretrdm010%3Aver1.0%3A%23urn%3Awww.peppol.eu%3Abis%3Apeppol4a%3Aver1.0%23urn%3Awww.difi.no%3Aehf%3Afaktura%3Aver1%3A%3A2.0 ; Unable to connect to http://B-97ecb4d561d466396597c8f2566561f5.iso6523-actorid-upis.sml.peppolcentral.org/iso6523-actorid-upis%3A%3A9908%3A810017901/services/busdox-docid-qns%3A%3Aurn%3Aoasis%3Anames%3Aspecification%3Aubl%3Aschema%3Axsd%3AInvoice-2%3A%3AInvoice%23%23urn%3Awww.cenbii.eu%3Atransaction%3Abiicoretrdm010%3Aver1.0%3A%23urn%3Awww.peppol.eu%3Abis%3Apeppol4a%3Aver1.0%23urn%3Awww.difi.no%3Aehf%3Afaktura%3Aver1%3A%3A2.0 ; B-97ecb4d561d466396597c8f2566561f5.iso6523-actorid-upis.sml.peppolcentral.org

If the SmpSignedServiceMetaDataException was allowed to pass though the try-catch in the getEndpointType method, I would be able to catch that exception my self. I know that there was a problem with SMP lookup, as it is a SMP exception, and I have the concrete error text.

Think the method should look more like this:

private EndpointType getEndpointType(ParticipantId participant, PeppolDocumentTypeId documentTypeIdentifier) throws SmpSignedServiceMetaDataException {
    try {
        SignedServiceMetadataType serviceMetadata = getServiceMetaData(participant, documentTypeIdentifier);
        return selectOptimalEndpoint(serviceMetadata);
    }
    catch (SmpSignedServiceMetaDataException e) {
        throw e;
    } catch (Exception e) {
        throw new RuntimeException("Problem with SMP lookup", e);
    }
}

If this is not changed, I have to catch all exception in one catch, and then look at the inner exception. I then have to switch over all the different exception type, that must be handle - that is exactly what a a try-catch perform for me. There must be a switch case of type SmpSignedServiceMetaDataException, before I know it is a SMP problem, but then I know what to do, and how to report back.

From my point of view, the current try-catch design is not good, and not the way to implement exception handler.

I can see that there are multiple concrete exceptions implemented in Oxalis (SmpLookupException, ParticipantNotRegisteredException, ...), but if they anyway are wrapped in a RuntimeException, what are they then used for?

Jacob Mogensen

teedjay commented 10 years ago

I agree with your point that hiding the underlying (and more specific) exception in this case seems illogical and not the best of designs. However there might be some subtle issues with un-checked vs checked exceptions at play, so I'll have to look into this in some more details before posting an eventual fix.

But we very much value your numerous comments and inputs lately, keep up the good work!

klakegg commented 6 years ago

Fixed in Oxalis 4.0 RC2.