TRWTF is Amazon


  • :belt_onion:

    So I'm working on a service to integrate to Amazon's MWS system... using Java war stuff.

    Today I went spelunking in the MWS code to find what kind of exception got thrown by a call... and I found this.

    /******************************************************************************* 
     * Copyright 2009-2012 Amazon Services. All Rights Reserved.
     * Licensed under the Apache License, Version 2.0 (the "License"); 
     *
     * You may not use this file except in compliance with the License. 
     * You may obtain a copy of the License at: http://aws.amazon.com/apache2.0
     * This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR 
     * CONDITIONS OF ANY KIND, either express or implied. See the License for the 
     * specific language governing permissions and limitations under the License.
     *******************************************************************************
     * Marketplace Web Service Runtime Client Library
     */
    package com.amazonservices.mws.client;
    
    /**
     * Exception thrown by MWS API requests.
     * 
     * @author mayerj
     */
    public class MwsException extends RuntimeException {
    
        /** Default SVUID. */
        private static final long serialVersionUID = 1L;
    
        /** The HTTP status code. */
        private int statusCode;
    
        /** The error message. */
        private String message;
    
        /** The response xml from server. */
        private String xml;
    
        /** The error code from server. */
        private String errorCode;
    
        /** The error type from server. */
        private String errorType;
    
        /** The error detail. */
        private String detail;
    
        /** The response header metadata. */
        private MwsResponseHeaderMetadata rhmd;
    
    /* More code snipped */
    
    /******************************************************************************* 
     *  Copyright 2009 Amazon Services.
     *  Licensed under the Apache License, Version 2.0 (the "License"); 
     *  
     *  You may not use this file except in compliance with the License. 
     *  You may obtain a copy of the License at: http://aws.amazon.com/apache2.0
     *  This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR 
     *  CONDITIONS OF ANY KIND, either express or implied. See the License for the 
     *  specific language governing permissions and limitations under the License.
     * ***************************************************************************** 
     *
     *  Marketplace Web Service Java Library
     *  API Version: 2009-01-01
     *  Generated: Wed Feb 18 13:28:48 PST 2009 
     * 
     */
    
    
    
    package com.amazonaws.mws;
    
    import com.amazonaws.mws.model.ResponseHeaderMetadata;
    
    /**
     * Marketplace Web Service  Exception provides details of errors 
     * returned by Marketplace Web Service  service
     *
     */
    @SuppressWarnings("serial")
    public class MarketplaceWebServiceException extends Exception {
        
        private String message = null;
        private int statusCode = -1;
        private String errorCode = null;
        private String errorType = null;
        private String requestId = null;
        private String xml = null;
        private ResponseHeaderMetadata responseHeaderMetadata = null;
    /* More code snipped */
    

    So...

    MarketplaceWebServiceException is thrown by some calls. It extends Exception so it's a checked exception. Which... is probably a good thing...

    MWSException is thrown by other calls. It extends RuntimeException and so it's not checked...

    None of these calls throw any kind of IOException or anything to deal with web errors. They just wrap them in the Amazon proprietary exceptions and... well... eh... Maybe we can figure out what happened? Maybe?

    TL;DR Amazon has 2 Exception classes with duplicate names, one of which is checked and the other isn't. For no good reason. At all.


  • I survived the hour long Uno hand

    @sloosecannon said:

    For no good reason. At all.

    Duh, it's so they can specify which exceptions are actually important and which ones can be safely ignored ;)



  • They aren't duplicate names, one's spelled-out the other is an acronym.



  • @blakeyrat said:

    They aren't duplicate names, one's spelled-out the other is an acronym.

    He didn't say they were, Mr. Obvious.

    He just said there are two exceptions, one checked, one not, and all other errors are buried inside these two exceptions. Not the worst architecture decision ever, but ranking right up there.


  • :belt_onion:

    @CoyneTheDup said:

    @blakeyrat said:
    They aren't duplicate names, one's spelled-out the other is an acronym.

    He didn't say they were, Mr. Obvious.

    He just said there are two exceptions, one checked, one not, and all other errors are buried inside these two exceptions. Not the worst architecture decision ever, but ranking right up there.

    Eh, technically I did 😛

    @sloosecannon said:

    TL;DR Amazon has 2 Exception classes with duplicate names

    But I buttumed people would understand I meant duplicate name... conventions? I don't even know what the term for that is...



  • @CoyneTheDup said:

    He didn't say they were, Mr. Obvious.

    @sloosecannon said:

    TL;DR Amazon has 2 Exception classes with duplicate names,

    QED



  • @sloosecannon said:

    But I buttumed people would understand I meant duplicate name... conventions?

    But now you know at least which bad exception is checked and which isn't. Good now?


  • :belt_onion:

    Honestly, knowing Amazon, I'm surprised they weren't actually duplicate. Because I could definitely see them doing that.



  • @sloosecannon said:

    It extends Exception so it's a checked exception. Which... is probably a good thing...

    Java's implementation of checked exceptions is pretty terrible. However, having two exceptions for the same thing is worse. I bet they are in the process of transitioning to the runtime exceptions.


  • :belt_onion:

    It looks like they are, since the RuntimeException class is the newer one. Honestly, I'd prefer the checked exception because then I'd actually know what the inner calls threw rather than having to go spelunking to figure it out myself...



  • @sloosecannon said:

    They just wrap them in the Amazon proprietary exceptions and... well... eh... Maybe we can figure out what happened? Maybe?

    There's a term for this: exception translation.

    Joshua Bloch talks about it in Effective Java, Second Edition, Item 61: Throw exceptions appropriate to the abstraction.

    Edit: I should probably explain this further. In your application, you don't really care why a call to AMS failed, only that it failed. If you really wanted to know why it failed, you could check the exception's inner exception and find out.


  • :belt_onion:

    Well I know it's a pattern, the issue is that it's being used to swallow network exceptions with an exception used to indicate a bad response...
    It doesn't appear to actually save the cause exception either, so I'm left to guess why the hell my call failed and figure out whether it was a network issue or if we got rejected.



  • @sloosecannon said:

    It doesn't appear to actually save the cause exception either, so I'm left to guess why the hell my call failed and figure out whether it was a network issue or if we got rejected.

    This, on the other hand, is an issue. The original exception should have been saved.


  • :belt_onion:

    Yup, that's my primary issue with it. I can understand cutting down the exceptions thrown, but I'd like to know what caused the issue in the first place :)



  • You'd almost think Exceptions should have a constructor that allows you to chain exceptions... oh wait, they do!

    Also, why does MwsException have a String message field? Did they miss that Exception already has this (and a setterconstructor for it)?



  • i believe i heard of this article here


  • Discourse touched me in a no-no place

    @sloosecannon said:

    It doesn't appear to actually save the cause exception either

    So there's a :wtf: after all. Just not in any of the bits you quoted to us. Jolly good show!


  • Winner of the 2016 Presidential Election

    @sloosecannon said:

    Java war

    Eh? How would stuff from an early 19th century war in Indonesia be useful for promoting copacetic racial relations in serving a silk marketplace in South America?



  • The MWS C# library redefines Message in its Exception base class too. Better yet, it's often null, which breaks the contract of .NET's own Exception class. Resharper/etc. get confused by this and shout at you if you null check it.


  • Discourse touched me in a no-no place

    :rimshot:



  • This post is deleted!

  • :belt_onion:

    You don't think having two exceptions with the same purpose is a WTF?

    @Dreikin said:

    Eh? How would stuff from an early 19th century war in Indonesia be useful for promoting copacetic racial relations in serving a silk marketplace in South America?

    + :rimshot:

    @nexekho said:

    The MWS C# library redefines Message in its Exception base class too. Better yet, it's often null, which breaks the contract of .NET's own Exception class. Resharper/etc. get confused by this and shout at you if you null check it.

    Oh good, so it's not just a Java thing. Amazon is stupid no matter the language...


  • Discourse touched me in a no-no place

    @sloosecannon said:

    You don't think having two exceptions with the same purpose is a WTF?

    Having two classes with the same purpose (in all meaningful ways) is pretty common when dealing with large strongly-typed systems. Why the duplication? Because they're on different sides of some sort of critical partitioning of the overall system (e.g., they're in independent libraries) and the people involved won't work together to remove the duplication because they've no real incentive to do so. Given that this happens with other simple classes, that it happens with exceptions as well is not very surprising.

    It's possible to avoid such things, but it's really much harder than it appears to be.


  • ♿ (Parody)

    @dkf said:

    Having two classes with the same purpose (in all meaningful ways) is pretty common when dealing with large strongly-typed systems.

    Sure, but you could do something like change the names beyond abbreviating (or changing the case or any other :wtf: way of beating a parser) the name so they're effectively the same name and you just have to remember the arbitrary difference.


  • Discourse touched me in a no-no place

    @boomzilla said:

    Sure, but you could do something like change the names beyond abbreviating (or changing the case or any other :wtf: way of beating a parser) the name so they're effectively the same name and you just have to remember the arbitrary difference.

    They'll be in a different namespace/package. Big whoop.

    It's not exactly what I was talking about though. It's relatively common to end up with parallel hierarchies though, where each is about the same as the other, but not quite. The semantics are a bit different (some difference in methods implemented) and they may be in different libraries because the two sides don't communicate, or at least not meaningfully. (Of course they agree to standardise: all you've got to do is just get the other side to change their code because they sure aren't interested in changing anything they do. This is the programming equivalent of megaphone diplomacy…)

    The bigger the project, the more you end up writing stupid glue to connect the bits together. All the pain of strong typing, but with almost none of the benefits.


  • BINNED

    @sloosecannon said:

    Amazon is stupid no matter the language...

    +1

    Still remember trying to log into a new EC2 instance. Ok, instructions... download this, no this, no... OK, this one, click... no download link, no repo, no nothing. Oh, look a link... that leads back to where I came from... Ok, ok, let's read carefully... retrieve some key thing, then use this client, but no, you should use this client which requires Java...

    Or, oh, look, go to dashboard and copy the link you can just feed to plain old ssh. Lovely. WHY DIDN'T YOU LIST THAT AS AN OPTION THEN?


Log in to reply