[jira] [Updated] (SQOOP-2006) bad smell: NPE check in SQOOP-1995 is not obvious

["Qian Xu (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/SQOOP-2006?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Qian Xu updated SQOOP-2006:
---------------------------
    Description: 
SQOOP-1995 fixed a NPE, which is adding a schema NPE check in 6 get/set FooData methods. [~vybs] explained that the IDF class can be instantiated by reflection, which will call the default no arg constructor, so that the property schema will not be assigned expectedly. 

IMO, although the patch can guard invalid access to a null schema, but as the getter and setter methods have nothing to do with schema and there is no comment left, it will be hard to understand why we have such code after couple of months. 

It seems a IDF class should have a not null schema the whole life time. I've some options:
1. We check null schema where a NPE happens
2. Disallow reflection of IDF class or search all code occurrences to make sure schema will be set properly (I think we already have test cases to guard use cases)
3. Use some design by contract framework (https://github.com/nhatminhle/cofoja) to define invariant, e.g. {{@Invariant(schema != null)}}. 




  was:
SQOOP-1995 fixed a NPE, which is adding a schema NPE check in 6 get/set FooData methods. [~vybs] explained that the IDF class can be instantiated by reflection, which will be constructed without setting a proper schema. The code can guard invalid access to null schema. As the getter and setter methods do not have anything to do with schema, and there is not a comment to explain that, it will be hard to track the reason of having such code after couple of month. 

IMO, it seems a not null schema should be an invariant condition for a IDF class. Either we check null schema where a NPE happens or use some design by contract framework to define contracts. 

I'd suggest move the schema validation to where a NPE happens instead of adding it to all getter and setters.




> bad smell: NPE check in SQOOP-1995 is not obvious
> -------------------------------------------------
>
>                 Key: SQOOP-2006
>                 URL: https://issues.apache.org/jira/browse/SQOOP-2006
>             Project: Sqoop
>          Issue Type: Improvement
>    Affects Versions: 1.99.5
>            Reporter: Qian Xu
>            Assignee: Qian Xu
>            Priority: Minor
>             Fix For: 1.99.5
>
>
> SQOOP-1995 fixed a NPE, which is adding a schema NPE check in 6 get/set FooData methods. [~vybs] explained that the IDF class can be instantiated by reflection, which will call the default no arg constructor, so that the property schema will not be assigned expectedly. 
> IMO, although the patch can guard invalid access to a null schema, but as the getter and setter methods have nothing to do with schema and there is no comment left, it will be hard to understand why we have such code after couple of months. 
> It seems a IDF class should have a not null schema the whole life time. I've some options:
> 1. We check null schema where a NPE happens
> 2. Disallow reflection of IDF class or search all code occurrences to make sure schema will be set properly (I think we already have test cases to guard use cases)
> 3. Use some design by contract framework (https://github.com/nhatminhle/cofoja) to define invariant, e.g. {{@Invariant(schema != null)}}. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)