> 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.
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. - Himanshu ----------------------------------------------------------- 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 > >
