Method to lookup persistent entities by an example object

Question

i'm currently writing a generic data access object for the persistent entities I have to handle in my JavaEE application. What this codeReview is about is a findByExample method that is used to find database entries on the basis of a given example object. This may already be implemented for openJPA but I'm bound to openJPA snapshot 1.2.x (old) and I couldn't find it anywhere in the manual. please tell me about the pitfalls, improvements and bad practices you see in my code. I'm willing to learn.

Here we go

Using it as following

MyEntity me = new MyEntity();
me.setName("john")
me.setAge(18);

entityAccess.findByExample(me);

will create the following JPQL query using only the parameters that were set

"SELECT x FROM MyEntity x WHERE x.name = :name AND x.age = :age"

Then it will set the parameters and exequte the query.

This is how I implemented it.

The two public methods (BasicEntity is a mappedSuperClass)

public <T extends BasicEntity> List<T> findAllByExample(T entity) {
return getQueryFromExample(entity).getResultList();
}

public <T extends BasicEntity> T findByExample(T entity) throws NoResultException, NonUniqueResultException {
return (T) getQueryFromExample(entity).getSingleResult();
}

The method thats creates the Query and sets the parameters. (em is the jpa entityManager)

private <T extends BasicEntity> Query getQueryFromExample(T entity) {

HashMap<String, Object> fieldNameValuePairs = getFieldNameValuePairs(entity.getClass(), entity);

/* build query string */
StringBuilder queryString = new StringBuilder();
queryString.append("SELECT x FROM ");
queryString.append(entity.getClass().getSimpleName());
queryString.append(" x WHERE ");
for (Iterator<String> iterator = fieldNameValuePairs.keySet().iterator(); iterator.hasNext();) {
    String fieldName = iterator.next();
    queryString.append("x." + fieldName + " = :" + fieldName);
    if (iterator.hasNext()) {
    queryString.append(" AND ");
    }
}


/* create query and set parameters */
Query q = em.createQuery(queryString.toString());
for (Entry<String, Object> entry : fieldNameValuePairs.entrySet()) {
    String fieldName = entry.getKey();
    Object fieldValue = entry.getValue();
    q.setParameter(fieldName, fieldValue);
}

return q;
}

The following method puts relevant and non null fields into a HashMap. It walks up the type hierarchy with a recursion. Object.getClass().getSuperClass returns null.

private HashMap<String, Object> getFieldNameValuePairs(Class<?> type, Object instance) {

if (type != null) {
    HashMap<String, Object> fieldNameValuePairs = new HashMap<String, Object>();

    for (Field field : type.getDeclaredFields()) {

    /* we only care for persitent fields */
    if (isPersistentField(field)) {
        field.setAccessible(true);
        String fieldName = field.getName();
        Object fieldValue = null;
        try {
        fieldValue = field.get(instance);
        } catch (Exception e) {
        throw new RuntimeException(
            "Error reflecting fields while building queryString for findByExample. Cannot read "
                + instance.getClass().getSimpleName() + "." + fieldName, e);
        }

        /* ignore unset fields */
        if (!(fieldValue == null || fieldValue.equals(""))) {
        fieldNameValuePairs.put(fieldName, fieldValue);
        System.out.println(fieldName + " = " + fieldValue.toString());
        }
    }

    }

    /* recursive step up the type hierarchy */
    fieldNameValuePairs.putAll(getFieldNameValuePairs(type.getSuperclass(), instance));
    return fieldNameValuePairs;
} else {
    return new HashMap<String, Object>();
}
}

the following section of code determines whether a field is relevant (since i don't want for example serialVersionUID in my JPQL query) I chose to use the fields annotations to decide, because I thought this makes the persisted fields unique.

private static final List<Class<? extends Annotation>> persistenceAnnotations = new ArrayList<Class<? extends Annotation>>();
static {
persistenceAnnotations.add(Id.class);
persistenceAnnotations.add(Column.class);
persistenceAnnotations.add(JoinColumn.class);
persistenceAnnotations.add(OneToOne.class);
persistenceAnnotations.add(ManyToOne.class);
persistenceAnnotations.add(OneToMany.class);
persistenceAnnotations.add(ManyToMany.class);
persistenceAnnotations.add(JoinColumns.class);
persistenceAnnotations.add(Basic.class);
persistenceAnnotations.add(Lob.class);
}

private boolean isPersistentField(Field field) {
for (Annotation annotation : field.getAnnotations()) {
    if (persistenceAnnotations.contains(annotation.annotationType())) {
    return true;
    }
}
return false;
}

Aug29 - REFACTORED VERSION

private <T extends BasicEntity> Query getQueryFromExample(T entity) {
    HashMap<String, Object> fieldNameValuePairs = getFieldNameValuePairs(entity.getClass(), entity);

    /* build query string */
    String queryString = String.format("SELECT x FROM %s WHERE %s", entity.getClass().getSimpleName(),
            join(asJpqlConditions(fieldNameValuePairs.keySet(), "x"), " AND "));

    /* create query and set parameters */
    Query q = em.createQuery(queryString.toString());
    for (Entry<String, Object> entry : fieldNameValuePairs.entrySet()) {
        q.setParameter(entry.getKey(), entry.getValue());
    }

    return q;
}

private List<String> asJpqlConditions(Collection<String> fieldNames, String identifier) {
    List<String> conditions = new ArrayList<String>();
    for (String fieldName : fieldNames) {
        conditions.add(String.format("%1$.%2$s = :%2$s", identifier, fieldName));
    }
    return conditions;
}

private String join(List<String> parts, String glue) {
    StringBuilder sb = new StringBuilder();
    boolean first = true;
    for (String part : parts) {
        if (!first) {
            sb.append(glue);
        }
        sb.append(part);
        first = false;
    }
    return sb.toString();
}

private HashMap<String, Object> getFieldNameValuePairs(Class<?> type, Object instance) {

    /* return empty HashMap in last recursion step (type is null) */
    if (type == null) {
        return new HashMap<String, Object>();
    }

    /* recursive call to fill the HashMap with the fields from supertypes */
    HashMap<String, Object> fieldNameValuePairs = getFieldNameValuePairs(type.getSuperclass(), instance);

    for (Field field : type.getDeclaredFields()) {

        /* we only care for persitent fields */
        if (!isPersistentField(field)) {
            continue;
        }

        boolean wasAccessible = field.isAccessible();
        field.setAccessible(true);
        String fieldName = field.getName();
        Object fieldValue = null;
        try {
            fieldValue = field.get(instance);
        } catch (Exception e) {
            throw new RuntimeException("Error reflecting fields while building queryString for findByExample. Cannot read "
                    + instance.getClass().getSimpleName() + "." + fieldName, e);
        }
        field.setAccessible(wasAccessible);

        /* ignore unset fields */
        if (!(fieldValue == null || fieldValue.equals(""))) {
            fieldNameValuePairs.put(fieldName, fieldValue);
        }
    }

    return fieldNameValuePairs;
}

private boolean isPersistentField(Field field) {
    for (Annotation annotation : field.getAnnotations()) {
        if (annotation.annotationType().equals(PersistentField.class)) { //added custom marker annotation
            return true;
        }
    }
    return false;
}

Answer 1

I found the code an interesting read and quite a pleasant one at that. Nothing really critical jumped out thought there are some precondition checking, logging, style and other violations in respect to what I might do.

1) I like to abstract trivial things away. For example, when you create the query string I would try to emphasize the structure of the query string instead of the noise caused by it's building. This might be done by creating method(s) that create the where condition of the query:

import static java.lang.String.format; 

... 

String queryString = format("SELECT x FROM %s WHERE %s",
                             entity.getClass().getSimpleName(),
                             join(asConditions(fieldNameValuePairs.keySet()), " AND "));

... 

private Collection<String> asConditions(Collection<String> fieldNames) {
   List<String> conditions = new ArrayList<String>();
   for(String fieldName : fieldNames) {
       conditions.add(format("x.%1$s = :%1$s", fieldName)); // 1$ reuses the 1st argument
   }
   return conditions;
}

private String join(Collection<String> parts, String glue) {
   StringBuilder sb = new StringBuilder();
   boolean first = true;
   for(String part : parts) {
      if(!first) {
         sb.append(glue);
      }
      sb.append(part);
      first = false;
   }
   return sb.toString();
}

2) I find it that the assigments are not needed here as they create new lines of codes without really increasing the readability of the whole.

for (Entry<String, Object> entry : fieldNameValuePairs.entrySet()) {
   String fieldName = entry.getKey();
   Object fieldValue = entry.getValue();
   q.setParameter(fieldName, fieldValue);
}

=>

for (Entry<String, Object> entry : fieldNameValuePairs.entrySet()) {
   q.setParameter(entry.getKey(), entry.getValue());
}

3) In the second method, you have created a long if-block in cas type is null. I would rather have failed early so the preconditions of the method is shown in the beginning. Also I feel that if the precondition is not met then some logging is needed or better yet an exception would be thrown because I would think that the missing type is a programmer error of some sorts and should not be let slip by.

If the null check is because of the recursion, you might want to create a comment as the recursion step is a bit far removed from the branching statement. Better yet, put the recursion in one method and the logic for the recursion step in another.

4) After noticing that the second method is private I noticed that there is no precondition checking in the first method. If the preconditions would be checked there, you can be a bit more relaxed with the checking in private methods although private methods may end up being reused by another developer at some point.

5) Again when branching on isPersistentField() you create a large block instead of exiting quickly. If you rather use this style then I would suggest you put the content of the block in a separate method. This way you can emphasize the logic of selection and keep methods nice a small.

Edit: Current impl:

for (Field field : type.getDeclaredFields()) {
  if (isPersistentField(field)) {
    field.setAccessible(true);
    String fieldName = field.getName();
    Object fieldValue = null;
    try {
       fieldValue = field.get(instance);
    } catch (Exception e) {
      throw new RuntimeException("foo", e);
    }

    if (!(fieldValue == null || fieldValue.equals(""))) {
      fieldNameValuePairs.put(fieldName, fieldValue);
    }
  }
}

Alternative 1:

for (Field field : type.getDeclaredFields()) {
  if (!isPersistentField(field)) {
    continue;
  }
  field.setAccessible(true);
  String fieldName = field.getName();
  Object fieldValue = null;
  try {
    fieldValue = field.get(instance);
  } catch (Exception e) {
    throw new RuntimeException("foo", e);
  }

  if (!(fieldValue == null || fieldValue.equals(""))) {
    fieldNameValuePairs.put(fieldName, fieldValue);
  }
}

Alternative 2:

for (Field field : type.getDeclaredFields()) {
  if (isPersistentField(field)) {
     addNameValuePair(field, instance, fieldNameValuePairs);
  }
}

...

private void addNameValuePair(Field field, Object instance, Map<String, Object> fieldNameValuePairs) {
  Object fieldValue = valueOf(field, instance);
  if (!(fieldValue == null || fieldValue.equals(""))) {
    fieldNameValuePairs.put(field.getName(), fieldValue);
  }
}

private Object valueOf(Field field, Object instance) {
  boolean wasAccessible = field.isAccessible();
  try {
    field.setAccessible(true);
    return field.get(instance);
  } catch (Exception e) {
    throw new RuntimeException("foo", e);
  } finally { 
    field.setAccessible(wasAccessible);
  } 
}

6) Set up a logger instead of System.out -> That way you can keep the debug level logs for later use if they needed.

7) JPA does not require that each field is annotated. This might cause some trouble on the long run. You could check how OpenJPA deals with this. They have their own FieldMetaData implementation that has a method setManagement that is used to signal if the field is persistent, transactional or not managed. This value is later used in ClassMetaData to return only those field that are managed. I actually got bored before finding the right place where setManagement is called but that it shouldn't be too hard to google.

8) I assume you have made an intentional decision not to think about the ability to use annotated getters instead of fields.

Disclaimer None of the code is tested