another null check patch

[senordhuff]

Hi,

I know this is a recurring theme. But, I had to patch a method to add a null check. We do a lot of dynamic field generation. And, for some reason, we were hitting a bug where a dynamically added datasource field is causing dataSource.getFields() to return an array with a null entry. So, this null check is needed in DataSource.getFieldNames. I've tested this and it solves our problems with no apparent issues. Would you consider adding this check to the code so we don't have to maintain this patch permanently?

	if (window.isc &&  isc.version.startsWith("v9.0")){
		
	    isc.DataSource.getPrototype().addProperties({
	    	getFieldNames:function (_1){
	    		if(isc.$cv)arguments.$cw=this;
	    		if(!_1)return isc.getKeys(this.getFields());
	    		var _2=this.getFields(),_3=[],_4=0;
	    		for(var _5 in _2){
	    			//9/17/13..added null check since a null field is getting included
	    			if(_2[_5]!=null && !_2[_5].hidden)_3[_4++]=_5
	    		}
	    		return _3;
	    	}

	    })
	    
	}

[Isomorphic]

It seems quite likely that you have an application level problem - we would not expect you to be able to get into a situation where getFieldNames would fail in this way unless you have something invalid in your specified fields array.
We've added the null check anyway (it seems safe), but we can't guarantee that there isn't something deeper going wrong here without seeing a reproducible test case demonstrating the problem.

Regards
Isomorphic Software

[senordhuff]

Thank you. Either there is some problem with our logic that is temporary and self corrects. Or, it is not self correcting and we have problems that are not being discovered during our testing. Or, it is also possible that we are doing things that are more complex than you see with typical usage from other clients. We seem to have a recurring history of us finding small little corner case issues like this that your other users aren't finding. So, I'd like to think that our in-depth usage of your product is helping you bullet-proof in ways that others are not. I know that it is frustrating that we keep coming with small patches and no test case because of the level of effort that would be involved with producing a test case. But, they are usually just very small null checks like this that do not dramatically impact your code. So, thank you for your understanding and willingness to add them so we don't have to maintain a huge patch library indefinitely.

[Isomorphic]

I think we understand each other :)

As I think we've established with other postings, we can add this kind of defensive coding check to some degree for you guys as supported customers with a well established app on a tight deadline - (provided the changes are safe and low impact, of course).
We'd prefer to see test cases if possible and may require them for us to roll in more in-depth changes - but we understand they're not always something you can readily produce and are being as flexible as possible.

This "disclaimer" is really there for a couple of reasons, and we'll continue to post similar warnings if additional cases like this arise going forward:

1) It's a warning to you guys that you really may have app level issues that could cause other problems. We simply can't know without seeing the underlying cause, so we'd rather warn you that while this may resolve an immediate problem you may run into something else, and we'd always recommend you perform an investigation to figure out how you're getting into this state which we haven't seen before. This investigation may yield it's a legitimate edge case which should be handled at the framework level (at which point you'd have a test case), or it may lead to uncovering an app level bug.

2) As this is a public forum, we'd like to make everything as clear as possible, and provide the same warning to anyone else who reads the thread. As a general precedent we want to make it clear that if anyone encounters a crash in framework code which could be avoided by a simple null check or similar, this doesn't necessarily mean there is a hole in the framework code - it's equally likely to point to bad usage / other problems in application code, and the best way to proceed is always to debug app code. You'll either uncover a problem at the app level, or end up with a test case indicating a real framework bug which we can then address.

Regards
Isomorphic Software

[senordhuff]

Sounds good. Understood on all fronts. Alone the same lines as the first null check. We found another situation where the same thing is happening and a simple null check should resolve it as well.

11:59:14.968:XRP1:WARN:Log:TypeError: Cannot read property 'dataPath' of null
Stack from error.stack:
DataSource.getField()
DataSource.recordsMatchingFilter()
DataSource.applyFilter()
ResultSet.addCacheData()
ResultSet.updateCache()
ResultSet.handleUpdate()
ResultSet.dataSourceDataChanged()
eval()
DataSource.updateCaches()
[c]DataSource.handleUpdate()

    getField : function (fieldName, checkDataPath) {
        if (isc.isAn.Object(fieldName)) fieldName = fieldName.name;
        var fields = this.getFields();
        var field = fields ? fields[fieldName] : null;

        if (field == null && checkDataPath && fields != null) {
            for (var i in fields) {
                if (fields[i].dataPath == fieldName) {
                    field = fields[i];
                    break;
                }
            }
        }
        return field;
    },

[Isomorphic]

getFields() should never return an array containing any null entries so something is clearly in an invalid state.
If you're ever setting "fields" to an array containing a null entry that's invalid usage, and something worth looking for in your app which would cause this.

Regardless, we've added this check!

Isomorphic Software