deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Lutterkort <lut...@redhat.com>
Subject Re: [PATCH core 1/4] CIMI: Initial addition of attribute validation for CIMI models
Date Thu, 07 Feb 2013 19:18:37 GMT
On Thu, 2013-02-07 at 13:52 +0100, mfojtik@redhat.com wrote:
> From: Michal Fojtik <mfojtik@redhat.com>
> 
> This addition will make possible to specify the 'required => true'
> option for all attributes in CIMI models, like:
> 
> class Machine < Base
>   text :name, :required => true
> end
> 
> To run a check if the instance of Machine is valid or not you can do
> following:
> 
> Machine.from_xml(xml_body).validate!
> 
> This method will raise the CIMI::Model::ValidationError exception if
> attribute marked as 'required' have no value set.

Nice; yes, we'll need that and more validation (e.g., that things need
to be valid URL's, numbers, or from a fixed set of values) 

We should mimic AR's validation stuff as much as possible. In that vein,
we shouldn't expose a validate! method, just valid? is enough. In fact,
wouldn't it be enough to run validation (1) at the end of parsing (2)
just before serializing internally generated objects ?

Also, I don't think that throwing errors is the right thing to do here -
we just want to store human-readable error messages in the model
objects. If the error is from deserializing a request, we want to give
the frontend a chance to produce a 400 Bad Request that lists all
errors. If the error happens when serializing an object we generated
internally we need to produce a 500 ISE with some indication of what
went wrong (an exception for that is perfectly fine)

> diff --git a/server/lib/cimi/models/base.rb b/server/lib/cimi/models/base.rb
> index ac838a4..998c36d 100644
> --- a/server/lib/cimi/models/base.rb
> +++ b/server/lib/cimi/models/base.rb
> @@ -73,6 +73,24 @@ require_relative '../helpers/database_helper'
>  
>  module CIMI::Model
>  
> +  class ValidationError < StandardError
> +
> +    def initialize(attribute)
> +      @attr_name  = attribute
> +      super("Required attribute '#{@attr_name}' must be set.")

The error message should at least include either the id of the offending
object or, if there is no id, its class name.
 
> @@ -150,6 +160,10 @@ class CIMI::Model::Schema
>        json
>      end
>  
> +    def valid?(value)
> +      @schema.required_attributes.any? { |a| a.valid?(value.send(a.name) )}

You mean all?, right ?

David



Mime
View raw message