> On Nov. 12, 2014, 3:17 p.m., Jonathan Hurley wrote: > >
Thanks for reviewing! Update patch coming soon. > On Nov. 12, 2014, 3:17 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewInstanceResourceProvider.java, > > line 268 > > <https://reviews.apache.org/r/27913/diff/1/?file=759580#file759580line268> > > > > Should this be a separate method called isValidationRequired() or > > something similar? It looks like this is getting the validator and then > > determining based on whether the validator is null whether or not > > validation is required. > > > > It's a nit, but if the validator isn't needed at this point, then a > > boolean should be enough. Yes, good catch. I was initially using the validator here but refactored a bunch of common code. A boolean is good enough. > On Nov. 12, 2014, 3:17 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/view/ViewRegistry.java, > > line 1056 > > <https://reviews.apache.org/r/27913/diff/1/?file=759583#file759583line1056> > > > > Does this need to be static? It doesn't appear to be accessed > > statically. Doesn't need to be static but nothing in the method is tied to an instance. It's really just a private helper method. > On Nov. 12, 2014, 3:17 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/view/ViewRegistry.java, > > line 1333 > > <https://reviews.apache.org/r/27913/diff/1/?file=759583#file759583line1333> > > > > Potential NPE here? Or is viewDefinition being null at this point not > > really possible? Should be okay. The viewDefinition is new'd just before calling this. It's private so I don't have to worry about unintended usages. > On Nov. 12, 2014, 3:17 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/view/validation/InstanceValidationResultImpl.java, > > line 69 > > <https://reviews.apache.org/r/27913/diff/1/?file=759585#file759585line69> > > > > Gson is thread-safe to reuse, but costly to instantiate. I would say > > make this static and just reuse the Gson instance that's built once. Good point. Thanks! - Tom ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27913/#review60989 ----------------------------------------------------------- On Nov. 12, 2014, 2:11 p.m., Tom Beerbower wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27913/ > ----------------------------------------------------------- > > (Updated Nov. 12, 2014, 2:11 p.m.) > > > Review request for Ambari, Alejandro Fernandez and Jonathan Hurley. > > > Bugs: AMBARI-8295 > https://issues.apache.org/jira/browse/AMBARI-8295 > > > Repository: ambari > > > Description > ------- > > Proposal: > The view framework SPI exposes a new Validator interface that could > optionally be implemented by the view developer. The Validator interface > allows the view developer to define the validation logic that gets applied > when a new view instance is created or updated. The interface specifies a > svalidate method that is passed a ViewInstanceDefinition. The given view > instance is the instance that is being created or updated. The validator has > full access to the instance definition as well as the owning view definition > so it can check things like parameters and properties. If the validator > successfully validates the given instance it simply returns. If the > validation fails then the validator should throw an ValidationException with > a message that describes the failure. > > {code} > public interface Validator { > /** > * Validate the given view instance definition. Return {@code null} to > indicate that > * no validation was performed. > * > * @param definition the view instance definition > * @param mode the validation mode > * > * @return the instance validation result; may be {@code null} > */ > public ValidationResult validateInstance(ViewInstanceDefinition definition, > ValidationContext mode); > > /** > * Validate a property of the given view instance definition. Return > {@code null} to indicate that > * no validation was performed. > * > * @param property the property name > * @param definition the view instance definition > * @param mode the validation mode > * > * @return the instance validation result; may be {@code null} > */ > public ValidationResult validateProperty(String property, > ViewInstanceDefinition definition, ValidationContext mode); > > /** > * The context in which a view instance validation check is performed. > */ > public enum ValidationContext { > PRE_CREATE, // validation prior to creation > PRE_UPDATE, // validation prior to update > EXISTING // validation of an existing view instance > } > } > {code} > > The context allows the framework a way to let the validator know at what > point in the view instance lifecycle the validation is being performed. If a > validation fails during instance create (PRE_CREATE), the view framework will > rollback the instance creation and will fail with an appropriate response > (see below). The same is true for instance update (PRE_UPDATE). The > validator (if specified) will also be called during a GET of view instance > resources (EXISTING) and be reported back as properties of the resource in > the normal response. > > So, for example, a view instance may have a 'server' property. During > instance creation (PRE_CREATE), the validator for the view might check the > 'server' property for invalid characters and fail the create if found. After > an instance is created and a request to get the instance resource is made > (EXISTING), the validator might check to see if the server specified by the > 'server' property is actually reachable and fail the verify if not. In this > case the view instance resource is still reachable but the fact that the > validation failed will show as a property in the instance resource (see > below). > > The result of the validation for the view instance and each of its properties > are returned from the validator to the view framework in the validation > result classes ... > {code} > public interface ValidationResult { > > /** > * Determine whether or not the result is valid. > * > * @return true if the result is valid > */ > public boolean isValid(); > > /** > * Get the detail of the validation result. > * > * @return the validation result detail > */ > public String getDetail(); > > /** > * Successful validation result. > */ > public static final ValidationResult SUCCESS = new ValidationResult() { > @Override > public boolean isValid() { > return true; > } > > @Override > public String getDetail() { > return "OK"; > } > }; > } > {code} > > > If a POST or PUT for a view instance fails because of a validation check the > results will look something like this (note in the example that the > validation for one property passed while the other failed) ... > > {code} > { > "status": 400, > "message": "{"propertyResults": { > "cities": { > "valid": true, > "detail": "OK" > }, > "units": { > "valid": false, > "detail": "Units must be either metric or imperial." > } > }, "valid": false, "detail": "The instance has invalid properties."}" > } > {code} > > > When a view specifies a validator, the instance resource will include the > properties 'validation_result' and 'property_validation_results' which will > be reevaluated each time the resource is requested... > > {code} > { > "href" : > "http://c6401.ambari.apache.org:8080/api/v1/views/WEATHER/versions/1.0.0/instances/EUROPE";, > "ViewInstanceInfo" : { > "instance_name" : "EUROPE", > ... > "validation_result" : { > "valid" : true, > "detail" : "OK" > }, > "version" : "1.0.0", > "view_name" : "WEATHER", > ... > "properties" : { > "cities" : "London, UK;Paris;Munich", > "units" : "imperial" > }, > "property_validation_results" : { > "cities" : { > "valid" : true, > "detail" : "OK" > }, > "units" : { > "valid" : true, > "detail" : "OK" > } > } > }, > ... > {code} > > Note that in the examples above, the detail messages are provided by the view > Validator implementation and are specific to the view (it can be any text > that makes sense for that view). > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseProvider.java > b4d582a > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewInstanceResourceProvider.java > 3f62cc3 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewEntity.java > d42e1a0 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewInstanceEntity.java > efa2818 > ambari-server/src/main/java/org/apache/ambari/server/view/ViewRegistry.java > a5d11bd > > ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java > 3a23ea7 > > ambari-server/src/main/java/org/apache/ambari/server/view/validation/InstanceValidationResultImpl.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/view/validation/ValidationResultImpl.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseProviderTest.java > fc8acd0 > > ambari-server/src/test/java/org/apache/ambari/server/orm/entities/ViewEntityTest.java > 965cebb > > ambari-server/src/test/java/org/apache/ambari/server/orm/entities/ViewInstanceEntityTest.java > 08aea6e > > ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java > f6ec403 > > ambari-server/src/test/java/org/apache/ambari/server/view/validation/InstanceValidationResultImplTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/view/validation/ValidationResultImplTest.java > PRE-CREATION > > ambari-views/src/main/java/org/apache/ambari/view/validation/ValidationResult.java > PRE-CREATION > ambari-views/src/main/java/org/apache/ambari/view/validation/Validator.java > PRE-CREATION > ambari-views/src/main/resources/view.xsd b95e4ec > > Diff: https://reviews.apache.org/r/27913/diff/ > > > Testing > ------- > > Manual tests. > > New unit tests added. All existing tests pass ... > > Results : > > Tests run: 2244, Failures: 0, Errors: 0, Skipped: 14 > > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 24:20 min > [INFO] Finished at: 2014-11-12T07:51:20-05:00 > [INFO] Final Memory: 41M/351M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Tom Beerbower > >
