Re: svn commit: r667888 - in /incubator/shindig/trunk/java: common/src/main/java/org/apache/shindig/common/xml/ common/src/test/java/org/apache/shindig/common/xml/ gadgets/src/main/java/org/apache/shindig/gadgets/spec/ gadgets/src/test/java/org/apach

[Kevin Brown <et...@google.com>]

Brian,

There are a bunch of issues with this patch (and the other one, but I'll get
to that separately). They're not consistent with the rest of the code (or
our style guide), and there are several subtle bugs here.

I can fix these if you'd like -- but they need to be addressed.

See inline comments.

On Sat, Jun 14, 2008 at 2:58 PM, <be...@apache.org> wrote:

> +  /**
> +   * Retrieves an attribute as a URI, and verifies that the URI is an http
> +   * or https URI.
> +   * @param node
> +   * @param attr
> +   * @param def
> +   * @return the parsed uri, or def if the attribute is not a valid http
> or
> +   * https URI.
> +   */
> +  public static URI getHttpUriAttribute(Node node, String attr, URI def) {
> +    URI uri = getUriAttribute(node, attr, def);
> +    if (uri == null) {
> +      return def;
> +    }
> +    if (!"http".equals(uri.getScheme()) &&
> !"https".equals(uri.getScheme())) {
> +      return def;
> +    }
> +    return uri;
> +  }


It seems to me that these methods will conflict with the goal of supporting
relative urls for all parts of the spec (
http://groups.google.com/group/opensocial-and-gadgets-spec/browse_thread/thread/50722f096c7a5746/9b2e40d61d837208?lnk=gst&q=base#9b2e40d61d837208
)

This is highly likely to be adopted into 0.9 (and 0.8 already requires it
for certain things, like message bundles). uri.getScheme() returns null for
relative paths.

+  /**
> +   * ModuleSpec/OAuthSpec


This comment is wrong -- the comments in the source files are supposed to be
the XPath queries to get to the element. There is no ModuleSpec/OAuthSpec
element.

  /**
> @@ -495,6 +509,7 @@
>     categories = prefs.getCategories();
>     features = prefs.getFeatures();
>     locales = prefs.getLocales();
> +    oauth = prefs.oauth;


The oauth service urls need to be substituted as well (as is required by
0.8).


> +/**
> + * Information about an OAuth service that a gadget wants to use.
> + *
> + * Instances are immutable.
> + */
> +public class OAuthService {
> +  private EndPoint requestUrl;
> +  private EndPoint accessUrl;
> +  private URI authorizationUrl;
> +  private String name;


These should be final, and initialized in the ctor to ensure that it's
immutable; otherwise the documentation is lying.

+  /**
> +   * Represents /OAuth/Service/Request elements.
> +   */
> +  public EndPoint getRequestUrl() {


These method names aren't consistent with other use of get*Url in the code;
getRequest or getRequestEndPoint would be more appropriate. An EndPoint is
not a url.

+  /**
> +   * Location for OAuth parameters in requests to an OAuth request token,
> +   * access token, or resource URL.  (Lowercase to match gadget spec
> schema)
> +   */
> +  public enum Location {
> +    header, url, body;


All enum values must be upper case (a custom parse(String) method can be
added for converting from strings, as is done with other enums). Otherwise
you may as well just use raw strings.

+  private static final String URL_ATTR = "url";
> +  private static final String PARAM_LOCATION_ATTR = "param_location";
> +  private static final String METHOD_ATTR = "method";


The constants should all be at the top of the class (following Sun's member
order as stated on our style guide page at
http://cwiki.apache.org/SHINDIGxSITE/java-style.html)

+  /**
> +   * Description of an OAuth request token or access token URL.
> +   */
> +  public static class EndPoint {
>

> +    public final URI url;
> +    public final Method method;
> +    public final Location location;


These should be private and exposed through getters, as is the case for all
the other spec elements. Consistency is important.


> +  /**
> +   * Constructor for testing only.
> +   */
> +  OAuthService() {
> +  }


This method is unnecessary, and it actually makes the tests bad by making
them fragile (if I change implementation details, tests break), and by
making them less thorough (tests against the output of the proper
constructors never test the error cases, only the "valid" input cases).

Take a look at the tests for all of the other spec elements. These tests
must verify that the entire element is parsed correctly and converted to the
proper internal data structures, and that when there's something wrong with
them, proper errors are returned. How they're parsed is an irrelevant
implementation detail that will change over time.

+  URI parseAuthorizationUrl(Element child) throws SpecParserException {
> +    URI url = XmlUtil.getHttpUriAttribute(child, URL_ATTR);
> +    if (url == null) {
> +      throw new SpecParserException("OAuth/Service/Authorization @url is
> not valid: " +
> +          child.getAttribute(URL_ATTR));
> +    }
> +    return url;
> +  }
> +
> +
> +  EndPoint parseEndPoint(String where, Element child) throws
> SpecParserException {


"where" isn't used anywhere in this method. Why is it being passed?


> +    Location location = Location.header;
> +    String locationString = child.getAttribute(PARAM_LOCATION_ATTR);
> +    if (!"".equals(locationString)) {
> +      location = Location.valueOf(locationString);
> +    }


This belongs in a parse method on Location.

+    Method method = Method.POST;
> +    String methodString = child.getAttribute(METHOD_ATTR);
> +    if (!"".equals(methodString)) {
> +      method = Method.valueOf(methodString);
> +    }


Same here.


> +  public Map<String, OAuthService> getServices() {
> +    return Collections.unmodifiableMap(serviceMap);


This should be done once in the ctor -- it's relatively cheap to do it here,
but it also doesn't guarantee immutability, as the class comment claims.

+  @Test
> +  public void testParseAuthorizeUrl_nourl() throws Exception {
> +    String xml = "<Authorization/>";
> +    try {
> +      service.parseAuthorizationUrl(XmlUtil.parse(xml));
> +      fail("Should have rejected malformed Authorization element");
> +    } catch (SpecParserException e) {
> +      assertEquals("OAuth/Service/Authorization @url is not valid: ",
> e.getMessage());


This is a very fragile test. If I change the string (0 impact on library
users or end users), the test breaks. Even if I modify SpecParserException
to annotate the message with some extra data, it breaks. Are you trying to
test that your code does what it's supposed to do, or that the runtime is
doing what you told it to do? All that matters here is that an exception is
thrown if there is no url on the authorization element; the specific message
attached to that exception is unimportant (and if it was important, there
should be a constant for it).

+  @Test
> +  public void testOAuthSpec_noservice() throws Exception {


noservice is two words and needs to be capitalized as such;  the same goes
for other similar test names; naming needs to be consistent.