> On May 7, 2015, 4:57 a.m., Amareshwari Sriramadasu wrote: > > lens-cli/src/test/java/org/apache/lens/cli/TestLensFactCommandsWithMissingWeight.java, > > line 110 > > <https://reviews.apache.org/r/33838/diff/2/?file=951313#file951313line110> > > > > Didnt this through an exception because of missing weight? > > Yash Sharma wrote: > No. > Infact the generated class for XFactTable used weight as primitive and > hence it gets the default of 0d. > > ```java > @XmlAttribute(name = "weight", required = true) > protected double weight; > ``` > > Amareshwari Sriramadasu wrote: > Then, I see making it "required" actually didnt solve the purpose. > Because missing value actually made it zero. > Inthat case I would say we might go with adding default as zero in xsd to > be clear, and allow only positive values for weight. Himanshu, thoughts? > > Himanshu Gahlaut wrote: > From perspective of domain objects, putting a checkArgument(weight > 0) > in AbstractCubeTable constructor shall help in making sure that domain object > CubeFactTable is constructed in valid state. > > For XSD and XFactTable, we can try using xs:restriction in XSD. Something > as below: > <xs:attribute name="weight"> > <xs:simpleType> > <xs:restriction base="xs:double"> > <xs:minExclusive value="0"></xs:minExclusive> > </xs:restriction> > </xs:simpleType> > </xs:attribute> > > and then using <xs:attribute ref="weight" use="required"/> in > XDimensionTablea nd XFactTable defintion in XSD. > > > For test case, one integration test case which makes sure that FactTable > creation gracefully fails when a fact is created with missing weight and one > unit test case which ensures that CubeFactTable instance creation throws > IllegalArgumentException when it is created with weight <= 0 shall give us > sufficient coverage. > > Yash Sharma wrote: > @Himanshu: I am still able to pass my test scenario successfully with the > xs:restriction implemented and missing weight attribute. The XFactTable > weight still gets initialized with a default value of zero. > Adding updated patch for review. > > Himanshu Gahlaut wrote: > In existing code > [here](https://github.com/apache/incubator-lens/blob/master/lens-client/src/main/java/org/apache/lens/client/LensMetadataClient.java#L71) > , there is no schema set for JAXB_UNMARSHALLER instance. This would mean > that no validation would happen when fact_without_weight.xml is unmarshalled > into XFactTable. A separate jira ticket can be parked to first figure out > whether existing code is doing XSD validation before unmarshalling or not. If > it is not doing it, then it has be enabled first. [setSchema in UnMarshaller > interface](http://docs.oracle.com/javaee/7/api/javax/xml/bind/Unmarshaller.html#setSchema(javax.xml.validation.Schema)) > is used for setting schema for unmarshalling validation. > > If this is getting enabled for the first time, it might not be straight > forward to make it work. A separate Jira ticket might help in that case. > > Yash Sharma wrote: > I missed this comment. I will resume working on this today. Thanks
setSchema() is not being used anywhere in code currently so we do not have any restrictions imposed now. Also if its implemented now - how would we like to handle the validation error. Should we raise an exception directly in setEventHandler? - Yash ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33838/#review82792 ----------------------------------------------------------- On May 7, 2015, 4:42 p.m., Yash Sharma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33838/ > ----------------------------------------------------------- > > (Updated May 7, 2015, 4:42 p.m.) > > > Review request for lens. > > > Repository: lens > > > Description > ------- > > NPE when weight is not set in XFactTable object while creating fact table > > > Diffs > ----- > > lens-api/src/main/resources/cube-0.1.xsd 10b1dfa > > lens-cli/src/test/java/org/apache/lens/cli/TestLensFactCommandsWithMissingWeight.java > PRE-CREATION > lens-cli/src/test/resources/fact_without_weight.xml PRE-CREATION > > Diff: https://reviews.apache.org/r/33838/diff/ > > > Testing > ------- > > Yes. > TestCase: > mvn test -Dtest=TestLensFactCommandsWithMissingWeight#testFactCommands > > > Thanks, > > Yash Sharma > >
