code review for {Group}{Plugin|Resource}ConfigurationUpdateCriteria

Joseph Marques jmarques at redhat.com
Mon Nov 15 18:43:11 UTC 2010


Mazz, as requested...
> commit cec59ed94fc58149083943bcafcc2e94abc71359
> Author: John Mazzitelli<mazz at redhat.com>
> Date:   Mon Nov 15 01:24:01 2010 -0500
>
> /modules/core/domain/src/main/java/org/rhq/core/domain/criteria/AbstractConfigurationUpdateCriteria.java
> ...
> +public abstract class AbstractConfigurationUpdateCriteria extends Criteria {
> +    private static final long serialVersionUID = 1L;
> +
> +    public static final String SORT_FIELD_CTIME = "ctime";
> +    public static final String SORT_FIELD_STATUS = "status";
> +
> +    private Integer filterId;
> +    private Long filterStartTime; // requires overrides
> +    private Long filterEndTime; // requires overrides

AbstractConfigurationUpdate has both createdTime and modifiedTime 
fields.  So I would either:

1) have filter{Created|Modified}{Start|End}Time filter pairs, or
2) keep the single filter{Start|End}Time pair, but change the generated 
query fragments to "createdTime > :startTime" and "modifiedTime < 
:endTime" respectively

> ...
> +    public AbstractConfigurationUpdateCriteria() {
> +
> +        filterOverrides.put("startTime", "ctime>= ?");
> +        filterOverrides.put("endTime", "ctime<= ?");
> +
> +        sortOverrides.put(SORT_FIELD_STATUS, "status");
> +    }

Note, "ctime" should have been "createdTime", otherwise those filters 
would have generated a query that didn't parse.

> modules/core/domain/src/main/java/org/rhq/core/domain/criteria/AbstractGroupConfigurationUpdateCriteria.java
> ...
> +public abstract class AbstractGroupConfigurationUpdateCriteria extends AbstractConfigurationUpdateCriteria {
> +    private static final long serialVersionUID = 1L;
> +
> +    private List<Integer>  filterResourceGroupIds; // requires override
> +
> +    public AbstractGroupConfigurationUpdateCriteria() {
> +        filterOverrides.put("resourceGroupIds", "group.id IN ( ? )");
> +    }
> +
> +    public void addFilterResourceGroupIds(List<Integer>  filterResourceGroupIds) {
> +        this.filterResourceGroupIds = filterResourceGroupIds;
> +    }
> +}

Please change the signature to take varargs and use 
CriteriaUtils.getListIgnoringNulls.  Here's the JDoc snippet for the 
utility method:

     /*
      * remove nulls from the passed items, and return the resultant 
list.  return null if no non-null items exist.
      * this is useful in criteria addFilterXXX methods which take 
varargs, where a single null argument should be
      * interpreted as a null collection (instead of one-item collection 
whose first value is null).
      */

How is this used in practice?  In an attempt to try and simplify the 
programming model for adding search/filtering to our views, the "S[] 
get{Array}Filter(DSRequest, String, Class<S>)" methods were added to 
RPCDataSource.  Here is a snippet of how simple it is to pull an array 
of types out of the request using these helper methods.

AlertDataSource {
     AlertCriteria getCriteria(DSRequest request) {
         ..
         criteria.addFilterPriorities(getArrayFilter(request, 
"severities", AlertPriority.class));
         ...
     }
}

By returning an array, it's possible to pass that result directly to the 
addFilterXXX varargs method, which CriteriaUtils then does further 
processing on to ensure that the private member variable removes 
elements that would generate invalid queries.

Also, varargs better supports the case where you only have one element 
to filter one.  From the interactive console / CLI and remote API 
standpoints, it's easier just to pass a single element directly, rather 
than having to wrap it in a collection.

> ...
> modules/core/domain/src/main/java/org/rhq/core/domain/criteria/PluginConfigurationUpdateCriteria.java
> ...
> +public class PluginConfigurationUpdateCriteria extends AbstractResourceConfigurationUpdateCriteria {
> ...
> +    private List<Integer>  filterResourceGroupIds; // requires overrides
> ...
> +    public PluginConfigurationUpdateCriteria() {
> ...
> +        filterOverrides.put("resourceGroupIds", "resource.id IN " //
> +            + "( SELECT res.id " //
> +            + "    FROM ResourceGroup rg " //
> +            + "    JOIN rg.explicitResources res " //
> +            + "   WHERE rg.id = ? )");
Since resourceGroupIds is a collection, you need to say "where rg.id IN 
( ? )".  In other words, it needs to be an in-clause that contains a 
sub-query with an in-clause.  An example of this style is the 
"explicitResourceIds" filter in ResourceGroupCriteria.
> ...
> +    public void addFilterResourceIds(Integer... filterResourceIds) {
> +        this.filterResourceIds = Arrays.asList(filterResourceIds);
> +    }
> +
> +    public void addFilterResourceGroupIds(Integer... filterResourceGroupIds) {
> +        this.filterResourceGroupIds = Arrays.asList(filterResourceGroupIds);
> +    }

See discussion about using CriteriaUtils above.

> modules/core/domain/src/main/java/org/rhq/core/domain/criteria/ResourceConfigurationUpdateCriteria.java

The following two issues were pre-existing:

The query fragment for "explicitResourceIds" in 
ResourceConfigurationUpdateCriteria has the same issue as 
"filterResourceGroupIds" PluginConfigurationUpdateCriteria above did.  
The sub-query needs to use "WHERE rg.id IN ( ? ) " as opposed to "WHERE 
rg.id = ? "

The varargs methods for ResourceConfigurationUpdateCriteria should 
leverage CriteriaUtils.


More information about the rhq-devel mailing list