Re: svn commit: r678591 - in /incubator/shindig/trunk/java/social-api: ./ src/test/java/org/apache/shindig/social/opensocial/util/ src/test/resources/

[Ian Boston]

Cassie,

Complexity
---------------
BeanJsonLibConverter is really just a configuration of the underlying  
SF json-lib, so most of the complexity surrounds.

1. Getting the Guice injector to create the classes,  
(setNewBeanInstanceStrategy)
2. Ensuring that the various arrays are created directly with no post  
processing (eg person.addresses) the classMap
3. Ensuring that there are morphers in place for the Enum  
conversions. the morpherRegistry.

I think much of this complexity is driven by the way in which the SF  
json-lib needs to be configured.

Actually, those parts don't bloat the code, the part that bloats the  
code more is the special handling of poorly formed json (eg "" ->  
"{}") and the ability to handle both [] and {} as the roots.  We  
could remove some of this code, but that would make the conversion  
less robust.

On the positive side this puts the introspection inside the json-lib.

Improvement ?
--------------------

I think there are 2 things here.

the ORG json-lib appears to be poorly supported compared to the SF  
version that it became, however that better support has made it a  
more complex (probably -1) and arguably capable library (+1).

In the performance tests I did, there was a small improvement on the  
number of objects going through the GC (10 - 20 %) , but a large  
(ish) improvement on speed, 3x faster, on output.

I do have some concerns about using a library with dependencies,  
rather than owning all the code, however the support looks good, and  
the decencies are minimal

[DEBUG]     net.sf.json-lib:json-lib:jar:jdk15:2.2:compile (selected  
for compile)
[DEBUG]       commons-beanutils:commons-beanutils:jar:1.7.0:compile  
(selected for compile)
[DEBUG]         commons-logging:commons-logging:jar:1.0.3:compile  
(removed - nearer found: 1.1.1)
[DEBUG]       commons-collections:commons-collections:jar:3.2:compile  
(applying version: 3.2.1)
[DEBUG]       commons-logging:commons-logging:jar:1.1.1:compile  
(removed - nearer found: 1.1)
[DEBUG]       commons-logging:commons-logging:jar:1.1:compile  
(selected for compile)
[DEBUG]       net.sf.ezmorph:ezmorph:jar:1.0.4:compile (selected for  
compile)


So, possible actions:

Pull the JsonConfig setup out into a separate bean which would make  
the converter look simpler.
Try and simplify the edge case handling.



Ian


On 22 Jul 2008, at 01:30, Cassie wrote:

Thanks for fixing this Ian!
One overall comment though.. the BeanJsonLibConverter looks a lot more
complicated than the BeanJsonConverter... is it really an improvement?
Can we pare down the code at all?

- Cassie

On Mon, Jul 21, 2008 at 3:48 PM,  <[EMAIL PROTECTED]> wrote:
Author: ieb
Date: Mon Jul 21 15:48:25 2008
New Revision: 678591

URL: http://svn.apache.org/viewvc?rev=678591&view=rev
Log:
SHINDIG-451

Removed the dependency on shindig-eatures from the APIiValidator.
added in log4j.properties to the test scope to configure logging  
correctly

Added:
   incubator/shindig/trunk/java/social-api/src/test/resources/
   incubator/shindig/trunk/java/social-api/src/test/resources/ 
log4j.properties
   incubator/shindig/trunk/java/social-api/src/test/resources/ 
simplelog.properties
Modified:
   incubator/shindig/trunk/java/social-api/pom.xml
   incubator/shindig/trunk/java/social-api/src/test/java/org/ 
apache/shindig/social/opensocial/util/ApiValidator.java
   incubator/shindig/trunk/java/social-api/src/test/java/org/ 
apache/shindig/social/opensocial/util/ApiValidatorTest.java
   incubator/shindig/trunk/java/social-api/src/test/java/org/ 
apache/shindig/social/opensocial/util/BeanJsonLibConverterTest.java

Modified: incubator/shindig/trunk/java/social-api/pom.xml
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/ 
social-api/pom.xml?rev=678591&r1=678590&r2=678591&view=diff
===================================================================== 
=========
--- incubator/shindig/trunk/java/social-api/pom.xml (original)
+++ incubator/shindig/trunk/java/social-api/pom.xml Mon Jul 21  
15:48:25 2008
@@ -60,13 +60,6 @@
      <groupId>org.apache.shindig</groupId>
      <artifactId>shindig-common</artifactId>
    </dependency>
-    <!-- is this Ok, only used in test to load the features -->
-    <dependency>
-      <groupId>org.apache.shindig</groupId>
-      <artifactId>shindig-features</artifactId>
-      <scope>test</scope>
-    </dependency>
-
    <!-- external depenencies -->
    <dependency>
      <groupId>org.json</groupId>

Modified: incubator/shindig/trunk/java/social-api/src/test/java/ 
org/apache/shindig/social/opensocial/util/ApiValidator.java
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/ 
social-api/src/test/java/org/apache/shindig/social/opensocial/util/ 
ApiValidator.java?rev=678591&r1=678590&r2=678591&view=diff
===================================================================== 
=========
--- incubator/shindig/trunk/java/social-api/src/test/java/org/ 
apache/shindig/social/opensocial/util/ApiValidator.java (original)
+++ incubator/shindig/trunk/java/social-api/src/test/java/org/ 
apache/shindig/social/opensocial/util/ApiValidator.java Mon Jul 21  
15:48:25 2008
@@ -65,7 +65,7 @@
   *                 feature is missing
   *
   */
-  public ApiValidator(String feature) throws IOException,
+  private ApiValidator(String feature) throws IOException,
      ParserConfigurationException, SAXException {
    ctx = Context.enter();
    scope = ctx.initStandardObjects();
@@ -73,6 +73,15 @@
  }

  /**
+   * Load the ApiValidator with no features, this avoids having  
features in the classpath
+   * @throws IOException
+   */
+  public ApiValidator() throws IOException {
+    ctx = Context.enter();
+    scope = ctx.initStandardObjects();
+  }
+
+  /**
   * @param json
   *                The json to validate expected in a form { xyz:  
yyy } form

All these java docs are in slightly the wrong format. The explanation
of the param should come one space after the param (as opposed to on
the next line)

Also, I saw some if ( x ) lines which should be if (x) (no spaces
between parens)

   * @param object
@@ -120,7 +129,61 @@
    }
    log.debug("Loaded " + so);

-    return validateOject(so, object, optionalFields, nullfields);
+    ScriptableObject specification = getScriptableObject(object);
+    log.debug("Looking for  " + object + " found " + specification);
+    listScriptable(object, specification);
+    Object[] fields = specification.getIds();
+    String[] fieldNames = new String[fields.length];
+    for (int i = 0; i < fields.length; i++) {
+      Object fieldName = specification.get(String.valueOf(fields 
[i]), specification);
+      fieldNames[i] = String.valueOf(fieldName);
+    }
+
+    return validateObject(so, fieldNames, optionalFields,  
nullfields);
+
+  }
+
+  /**
+   * @param json
+   *                The json to validate expected in a form  
{ xyz: yyy } form
+   * @param fieldNames
+   *                An Array of field names that the oject should  
be tested against
+   * @param optionalFields
+   *                If any of the fields that appear in the json  
structure are
+   *                optional, then they should be defined in this  
parameter.
+   * @param nullfields
+   * @throws ApiValidatorExpcetion
+   *                 if there is a problem validating the json
+   * @return a map so string object pairs containing the fields  
at the top level
+   *         of the json tree. Where these are native java  
objects, they will
+   *         appear as native object. Complex json objects will  
appear as Rhino
+   *         specific objects
+   */
+  public Map<String, Object> validate(String json, String[]  
fieldNames,
+      String[] optionalFields, String[] nullfields)
+      throws ApiValidatorExpcetion {
+
+
+    log.debug("Loading " + json);
+    json = json.trim();
+    if (!json.endsWith("}")) {
+      json = json + "}";
+    }
+    if (!json.startsWith("{")) {
+      json = "{" + json;
+    }
+    json = "( testingObject = " + json + " )";
+
+    Object so = null;
+    try {
+      so = ctx.evaluateString(scope, json, "test json", 0, null);
+    } catch (EvaluatorException ex) {
+      log.error("Non parseable JSON " + json);
+    }
+    log.debug("Loaded " + so);
+
+
+    return validateObject(so, fieldNames, optionalFields,  
nullfields);

  }

@@ -133,7 +196,7 @@
   * @return
   * @throws ApiValidatorExpcetion
   */
-  public Map<String, Object> validateOject(Object jsonObject,  
String object,
+  public Map<String, Object> validateObject(Object jsonObject,  
String[] fieldNames,
      String[] optionalFields, String[] nullFields)
      throws ApiValidatorExpcetion {
    Map<String, String> optional = new HashMap<String, String>();
@@ -145,19 +208,14 @@
      nullf.put(nf, nf);
    }

-    ScriptableObject specification = getScriptableObject(object);
-    log.debug("Looking for  " + object + " found " + specification);
-    listScriptable(object, specification);

    Map<String, Object> resultFields = new HashMap<String, Object>();

    if (jsonObject instanceof ScriptableObject) {
      ScriptableObject parsedJSONObject = (ScriptableObject)  
jsonObject;
      listScriptable("testingObject", parsedJSONObject);
-      Object[] fields = specification.getIds();
-      for (Object f : fields) {
-        Object fieldName = specification.get(String.valueOf(f),  
specification);
-        Object o = parsedJSONObject.get(String.valueOf(fieldName),
+      for (String fieldName : fieldNames) {
+        Object o = parsedJSONObject.get(fieldName,
            parsedJSONObject);
        if (o == ScriptableObject.NOT_FOUND) {
          if (optional.containsKey(fieldName)) {
@@ -174,12 +232,12 @@
            if (nullf.containsKey(fieldName)) {
              log.error("Null Fields has been serialized " +  
fieldName);
            }
-            log.debug("Got a Null object for Field " + f + ":" +  
fieldName
+            log.debug("Got a Null object for Field " + fieldName
                + " on json [[" + jsonObject + "]]");

          } else {

-            log.debug("Got JSON Field  Field," + f + ":" +  
fieldName + " as "
+            log.debug("Got JSON Field  Field,"  + fieldName + " as "
                + o + " " + o.getClass());
          }
          resultFields.put(String.valueOf(fieldName), o);
@@ -266,7 +324,8 @@
        in = this.getClass().getClassLoader().getResourceAsStream(
            "features/" + scriptPath);
        if (in == null) {
-          throw new IOException("Cant load spec " + spec + " or  
features/"+spec+" from classpath");
+          throw new IOException("Cant load spec " + spec + " or  
features/"
+              + spec + " from classpath");
        }
      }
      InputStreamReader reader = new InputStreamReader(in);
@@ -315,9 +374,10 @@
        features);
    if (in == null) {
      in = this.getClass().getClassLoader().getResourceAsStream(
-          "features/"+features);
+          "features/" + features);
      if (in == null) {
-        throw new IOException("Cant find " + features + " or  
features/" + features+ " in classpath ");
+        throw new IOException("Cant find " + features + " or  
features/"
+            + features + " in classpath ");
      }
    }
    DocumentBuilderFactory builderFactory = DocumentBuilderFactory

Modified: incubator/shindig/trunk/java/social-api/src/test/java/ 
org/apache/shindig/social/opensocial/util/ApiValidatorTest.java
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/ 
social-api/src/test/java/org/apache/shindig/social/opensocial/util/ 
ApiValidatorTest.java?rev=678591&r1=678590&r2=678591&view=diff
===================================================================== 
=========
--- incubator/shindig/trunk/java/social-api/src/test/java/org/ 
apache/shindig/social/opensocial/util/ApiValidatorTest.java  
(original)
+++ incubator/shindig/trunk/java/social-api/src/test/java/org/ 
apache/shindig/social/opensocial/util/ApiValidatorTest.java Mon  
Jul 21 15:48:25 2008
@@ -37,6 +37,11 @@
   */
  private static final String TEST_DEFINITION =
    "var TestDef = {}; TestDef.Field = { FIELD1 : \"json\",  
FIELD2 : \"xyz\", FIELD3 : \"shouldBeMissing\" };";
+  private static final String[] TEST_DEFINITION_FIELDS = {
+    "json",
+    "xyz",
+    "shouldBeMissing"
+  };

  /**
   * test the validator for successful validation
@@ -47,7 +52,7 @@
   */
  @Test
  public void testValidator() throws ApiValidatorExpcetion,  
IOException, ParserConfigurationException, SAXException {
-    ApiValidator apiVal = new ApiValidator("opensocial-reference");
+    ApiValidator apiVal = new ApiValidator();
    apiVal.addScript(TEST_DEFINITION);
    String[] optional = {"shouldBeMissing"};
    String[] nullfields = {};
@@ -72,7 +77,7 @@
   */
  @Test
  public void testValidatorFail() throws ApiValidatorExpcetion,  
IOException, ParserConfigurationException, SAXException {
-    ApiValidator apiVal = new ApiValidator("opensocial-reference");
+    ApiValidator apiVal = new ApiValidator();
    apiVal.addScript(TEST_DEFINITION);
    String[] optional = {};
    String[] nullfields = {};
@@ -82,8 +87,49 @@
    } catch ( ApiValidatorExpcetion ex ) {

    }
+  }
+  /**
+   * test the validator for successful validation
+   * @throws ApiValidatorExpcetion
+   * @throws IOException
+   * @throws ParserConfigurationException
+   * @throws SAXException
+   */
+  @Test
+  public void testFieldsValidator() throws ApiValidatorExpcetion,  
IOException, ParserConfigurationException, SAXException {
+    ApiValidator apiVal = new ApiValidator();
+    String[] optional = {"shouldBeMissing"};
+    String[] nullfields = {};
+     Map<String, Object> result = apiVal.validate("{ json: \"A  
Test JSON\", xyz : 123 }", TEST_DEFINITION_FIELDS, optional,  
nullfields );
+     Assert.assertNotNull(result);
+     Assert.assertNotNull(result.get("json"));
+     Assert.assertNotNull(result.get("xyz"));
+     Assert.assertEquals(String.class,result.get("json").getClass 
());
+     Assert.assertEquals(Integer.class,result.get("xyz").getClass 
());
+     Assert.assertEquals("A Test JSON",result.get("json"));
+     Assert.assertEquals(123,((Integer)result.get("xyz")).intValue 
());


  }
+
+  /**
+   * Test for a failing validation
+   * @throws ApiValidatorExpcetion
+   * @throws IOException
+   * @throws ParserConfigurationException
+   * @throws SAXException
+   */
+  @Test
+  public void testFieldsValidatorFail() throws  
ApiValidatorExpcetion, IOException, ParserConfigurationException,  
SAXException {
+    ApiValidator apiVal = new ApiValidator();
+    String[] optional = {};
+    String[] nullfields = {};
+    try {
+      apiVal.validate("{ jsonIsMissing: \"A Test JSON\", xyz :  
123 }", TEST_DEFINITION_FIELDS, optional, nullfields );
+      Assert.fail("Should have Generated an APIValidatorException  
");
+    } catch ( ApiValidatorExpcetion ex ) {
+
+    }
+  }

 }

Modified: incubator/shindig/trunk/java/social-api/src/test/java/ 
org/apache/shindig/social/opensocial/util/ 
BeanJsonLibConverterTest.java
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/ 
social-api/src/test/java/org/apache/shindig/social/opensocial/util/ 
BeanJsonLibConverterTest.java? 
rev=678591&r1=678590&r2=678591&view=diff
===================================================================== 
=========
--- incubator/shindig/trunk/java/social-api/src/test/java/org/ 
apache/shindig/social/opensocial/util/ 
BeanJsonLibConverterTest.java (original)
+++ incubator/shindig/trunk/java/social-api/src/test/java/org/ 
apache/shindig/social/opensocial/util/ 
BeanJsonLibConverterTest.java Mon Jul 21 15:48:25 2008
@@ -49,6 +49,23 @@

  private static final Log log = LogFactory
      .getLog(BeanJsonLibConverterTest.class);
+  // taken from opensocial-reference/person.js
+  private static final String[] PERSON_FIELDS = { "id", "name",  
"nickname",
+      "thumbnailUrl", "profileUrl", "currentLocation",  
"addresses", "emails",
+      "phoneNumbers", "aboutMe", "status", "profileSong",  
"profileVideo",
+      "gender", "sexualOrientation", "relationshipStatus", "age",
+      "dateOfBirth", "bodyType", "ethnicity", "smoker",  
"drinker", "children",
+      "pets", "livingArrangement", "timeZone", "languagesSpoken",  
"jobs",
+      "jobInterests", "schools", "interests", "urls", "music",  
"movies",
+      "tvShows", "books", "activities", "sports", "heroes",  
"quotes", "cars",
+      "food", "turnOns", "turnOffs", "tags", "romance", "scaredOf",
+      "happiestWhen", "fashion", "humor", "lookingFor", "religion",
+      "politicalViews", "hasApp", "networkPresence" };
+
+  // taken from opensocial-reference/name.js
+  private static final String[] NAME_FIELDS = { "familyName",  
"givenName",
+      "additionalName", "honorificPrefix", "honorificSuffix",  
"unstructured"};
+
  private Person johnDoe;
  private Activity activity;

@@ -80,19 +97,25 @@
    beanJsonConverter = new BeanJsonLibConverter(Guice
        .createInjector(new JsonLibTestsGuiceModule()));

-    apiValidator = new ApiValidator("opensocial-reference");
+    apiValidator = new ApiValidator();

  }

  public static class SpecialPerson extends PersonImpl {
    public static final String[] OPTIONALFIELDS = {};
-    public static final String[] NULLFIELDS  =  
{"jobInterests","nickname","romance","religion","timeZone",
-     
"relationshipStatus","tags","networkPresence","books","quotes","phone 
Numbers","languagesSpoken",
-     
"activities","jobs","dateOfBirth","profileVideo","bodyType","urls","s 
chools","music","addresses",
-     
"livingArrangement","thumbnailUrl","humor","sports","scaredOf","movie 
s","age","pets","hasApp","turnOffs",
-     
"gender","fashion","drinker","aboutMe","children","sexualOrientation" 
,"heroes","profileSong","lookingFor",
-     
"cars","turnOns","tvShows","profileUrl","status","currentLocation","s 
moker","happiestWhen","ethnicity",
-     
"food","emails","politicalViews","interests","familyName","honorificS 
uffix","additionalName","honorificPrefix","givenName"};
+    public static final String[] NULLFIELDS = { "jobInterests",  
"nickname",
+        "romance", "religion", "timeZone", "relationshipStatus",  
"tags",
+        "networkPresence", "books", "quotes", "phoneNumbers",
+        "languagesSpoken", "activities", "jobs", "dateOfBirth",  
"profileVideo",
+        "bodyType", "urls", "schools", "music", "addresses",
+        "livingArrangement", "thumbnailUrl", "humor", "sports",  
"scaredOf",
+        "movies", "age", "pets", "hasApp", "turnOffs", "gender",  
"fashion",
+        "drinker", "aboutMe", "children", "sexualOrientation",  
"heroes",
+        "profileSong", "lookingFor", "cars", "turnOns", "tvShows",
+        "profileUrl", "status", "currentLocation", "smoker",  
"happiestWhen",
+        "ethnicity", "food", "emails", "politicalViews",  
"interests",
+        "familyName", "honorificSuffix", "additionalName",  
"honorificPrefix",
+        "givenName" };

    private String newfield;

@@ -120,13 +143,13 @@

    String result = beanJsonConverter.convertToString(cassie);

-    validatePerson(result, "5", "robot",  
SpecialPerson.OPTIONALFIELDS, SpecialPerson.NULLFIELDS);
+    validatePerson(result, "5", "robot",  
SpecialPerson.OPTIONALFIELDS,
+        SpecialPerson.NULLFIELDS);

-    apiValidator.addScript(" specialPerson = { SPECIAL :  
\"newfield\" }; ");
    String[] optional = {};
    String[] nullfields = {};
    Map<String, Object> special = apiValidator.validate(result,
-        "specialPerson", optional,nullfields);
+        new String[] { "newfield" }, optional, nullfields);
    assertNotNull(special.get("newfield"));
    assertEquals(String.class, special.get("newfield").getClass());
    assertEquals("nonsense", special.get("newfield"));
@@ -156,18 +179,18 @@
   * @param result
   * @throws ApiValidatorExpcetion
   */
-  private void validatePerson(String result, String id, String  
name, String[] optional, String[] nullfields)
-      throws ApiValidatorExpcetion {
+  private void validatePerson(String result, String id, String name,
+      String[] optional, String[] nullfields) throws  
ApiValidatorExpcetion {

-    Map<String, Object> standard = apiValidator.validate(result,
-        "opensocial.Person.Field", optional,nullfields);
+    Map<String, Object> standard = apiValidator.validate(result,  
PERSON_FIELDS,
+        optional, nullfields);
    assertNotNull(standard.get("id"));
    assertEquals(String.class, standard.get("id").getClass());
    assertEquals(id, standard.get("id"));

    assertNotNull(standard.get("name"));
-    Map<String, Object> nameJSON = apiValidator.validateOject 
(standard
-        .get("name"), "opensocial.Name.Field", optional,nullfields);
+    Map<String, Object> nameJSON = apiValidator.validateObject 
(standard
+        .get("name"), NAME_FIELDS, optional, nullfields);
    ApiValidator.dump(nameJSON);

    assertNotNull(nameJSON.get("unstructured"));
@@ -269,16 +292,15 @@
    // map should contain, so we have to tell it
    beanJsonConverter.addMapping("item1", Map.class);
    beanJsonConverter.addMapping("item2", Map.class);
-    Map<?, ?> parsedMap = beanJsonConverter
-        .convertToObject(result, Map.class);
+    Map<?, ?> parsedMap = beanJsonConverter.convertToObject 
(result, Map.class);

    if (outputInfo) {
      log.info("Dumping Map (" + parsedMap + ")");
    }
    ApiValidator.dump(parsedMap);

-    assertEquals("1", ((Map<?,?>)parsedMap.get("item1")).get 
("value"));
-    assertEquals("2", ((Map<?,?>)parsedMap.get("item2")).get 
("value"));
+    assertEquals("1", ((Map<?, ?>) parsedMap.get("item1")).get 
("value"));
+    assertEquals("2", ((Map<?, ?>) parsedMap.get("item2")).get 
("value"));
  }

  public void testListsToJson() throws Exception {
@@ -299,8 +321,8 @@
    if (outputInfo) {
      log.info("JSON (" + result + ")");
    }
-    Map<?, ?>[] parsedList = beanJsonConverter.convertToObject(
-        result, Map[].class);
+    Map<?, ?>[] parsedList = beanJsonConverter.convertToObject 
(result,
+        Map[].class);

    assertEquals("1", parsedList[0].get("value"));
    assertEquals("2", parsedList[1].get("value"));

Added: incubator/shindig/trunk/java/social-api/src/test/resources/ 
log4j.properties
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/ 
social-api/src/test/resources/log4j.properties?rev=678591&view=auto
===================================================================== 
=========
--- incubator/shindig/trunk/java/social-api/src/test/resources/ 
log4j.properties (added)
+++ incubator/shindig/trunk/java/social-api/src/test/resources/ 
log4j.properties Mon Jul 21 15:48:25 2008
@@ -0,0 +1,8 @@
+log4j.rootCategory=info
+log4j.rootLogger=info, stdout
+
+
+log4j.appender.stdout=org.apache.log4j.ConsoleAppender
+log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
+log4j.appender.stdout.layout.ConversionPattern= %p %m  [%d] (%F:% 
L) %n
+

Added: incubator/shindig/trunk/java/social-api/src/test/resources/ 
simplelog.properties
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/ 
social-api/src/test/resources/simplelog.properties? 
rev=678591&view=auto
===================================================================== 
=========
--- incubator/shindig/trunk/java/social-api/src/test/resources/ 
simplelog.properties (added)
+++ incubator/shindig/trunk/java/social-api/src/test/resources/ 
simplelog.properties Mon Jul 21 15:48:25 2008
@@ -0,0 +1,8 @@
+log4j.rootCategory=info
+log4j.rootLogger=info, stdout
+
+
+log4j.appender.stdout=org.apache.log4j.ConsoleAppender
+log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
+log4j.appender.stdout.layout.ConversionPattern= %p %m  [%d] (%F:% 
L) %n
+