PDA

View Full Version : [OPEN-1039] JsonReader bug in 3.2.1 that affects Ext.data.ArrayStore



extbio
10 Jun 2010, 12:15 PM
Ext version tested:


Ext 3.2.1 (buggy)
Ext 3.1.0 (ok)



Description:


Ext.data.ArrayStore fails to instantiate when field name begins with non-word char and also contains a '.' (or '[' for that matter)



Test Case:



Ext.onReady(function() {
var store = new Ext.data.ArrayStore({
fields: [{"name":"*Mr. Smith"}]
});
alert('got here!');
});
Steps to reproduce the problem:


Run the code, see if the alert box shows up



The result that was expected:


Alert box shows up



The result that occurs instead:


No show - script execution fails with an error



Possible fix:


Several options:




Revert Ext.data.JsonReader.createAccessor to 3.1.0 version.
Fix the code inside Ext.data.JsonReader.createAccessor.
Use Ext.data.JsonReader.simpleAccess to replace Ext.data.JsonReader.createAccessor (this clearly was echoed by the Ext team member who left comment "TODO This isn't used anywhere?? Don't we want to use this where possible instead of complex #createAccessor?" on simpleAccess.

But clearly the reason why #3 option wasn't already taken was the hack implemented in it was not available in simpleAccess, although there's almost certainly a better way to achieve that.
Another important comment is that we've noticed multiple instances where "new" code in Ext 3.2.1 created bugs instead of improving the old working code. Another such instance was documented here (http://www.extjs.com/forum/showthread.php?99859-Ext.data.Store). It was a bug (unfixed, though we thoroughly explained) in Ext.data.Store. I sincerely hope that more care be taken by the Ext team when rewriting existing working code.

Condor
11 Jun 2010, 5:20 AM
Quick fix:

Ext.override(Ext.data.ArrayReader, {
buildExtractors: Ext.emptyFn,
extractValues: Ext.emptyFn
});

There are much better ways to solve this, but that would require a rather big rewrite of all the readers (which is being done for Ext 4).

extbio
11 Jun 2010, 6:13 AM
I'm fine with removing those functions, but there seems to be confusion even among Ext team members as to the importance of those functions, are you sure changing them to emptyFn is not going to affect any existing apps? For my own usage, I simply overrode createAccessor using 3.1.0 code.

evant
28 Jun 2010, 6:06 AM
Obviously it's a fairly strange use case, but technically valid nonetheless.

I think the simplest change is to just modify the regex in createAccessor to only match "[" or "." at the start of the string.

extbio
28 Jun 2010, 7:06 AM
Obviously it's a fairly strange use case

Why is it strange? There could be many valid use cases that break 3.2.x. Field names like "2008.01", "年.月.日" would all break it. I don't see why "*Mr. Smith" would never be used as a column name either.

Regardless, it's rather ridiculous trying to sugarcoat your bugs by criticizing the use case. Don't forget, your customer just did your job for you by spending the time to debug and create an easy-to-follow use case for you so you can fix it in no time. And you're obviously quite a grateful developer.


I think the simplest change is to just modify the regex in createAccessor to only match "[" or "." at the start of the string.

Did you even read the source code? Clearly the source code went to the length to enable a hack like "myoptions.mykey" as expr (to create an accessor to 'obj'.myoptions.mykey), and your "solution" would break your own hack. If you think it doesn't matter if it's broken, then might as well replace createAccessor with simpleAccess.

Jamie Avins
28 Jun 2010, 8:33 AM
Please try to keep things calm. It's obviously just a case Evan didn't think of and wasn't taken into account. We're all human, we all make mistakes. We appreciate the report.

evant
28 Jun 2010, 6:03 PM
Perhaps I didn't make myself clear. When I was saying it's a strange use case, I was indicating that was why it probably wasn't considered. When was the last time you saw a database field called "*Mr. Smith". That generally tends to be data, not something that describes the data.

Nonetheless, what I was suggesting was that only ".[" at the start of the string should be accepted for the regex matching, anything else would need to happen via a passed function for the mapping. I see a few options:

1) Keep it as it is (obviously not desireable).
2) What I suggested above
3) Provide an option on the reader to allow/disallow anything other than the simple access method, once it has passed by the function checks.

3 is probably the most flexible, because we would like to keep that functionality as it is quite useful.

Condor
28 Jun 2010, 10:42 PM
Am I missing something here?

The OP is using an ArrayStore, which shouldn't even be using JsonReader.buildExtractors or JsonReader.extractValues, because ArrayReader.readRecords has it's own implementation.
The correct fix would be to implement buildExtractors and extractValues for ArrayReader, but since the whole thing is rewritten for Ext 4 anyway, temporarily setting buildExtractors and extractValues to Ext.emptyFn seems the correct fix.

If the OP would be using a JsonReader, then he should specify a mapping, e.g. mapping:'["*Mr. Smith"]'.
I'm not a fan of changing the createAccessor function as suggested, because it makes the method less flexible (I wrote it this way for a reason!).

evant
28 Jun 2010, 11:03 PM
@Condor: You're pretty much correct. As you and others have suggested, the ArrayReader can be pared down to a very minimal set of code, so we want to have something that will be "future compatible" (woah) going forward.

Animal
28 Jun 2010, 11:44 PM
In fact the correct fix for the OP, is to explicitly use an integer mapping value.

Paste this into Firebug:



var store = new Ext.data.ArrayStore({
fields: [{"name":"*Mr. Smith", mapping:0}],
data: [['I am Smith']]
});
alert(store.getAt(0).get("*Mr. Smith"));

extbio
29 Jun 2010, 9:56 AM
@Jamie: Yes, I guess I was overreacting a bit, because I found Ext forum tend to be less helpful than professional/premium quality, precisely because of a dismissive or "it's a feature not bug" or "you're not using it the right way" attitude when a bug was found. Many threads, including a bit in this one, demonstrated this issue.

@evant: Code in OP uses Ext.data.ArrayStore, there's no requirement that the columns be database fields. AFAIK, people use ArrayStore with all sorts of DBMS-columnname-incompatible characters.

@condor: I agree, when I tracked down the issue with the ArrayStore example to JsonReader, I felt it was strange too. I don't intend to use JsonReader at all in this case, so certainly I agree that it's best that ArrayStore does not use JsonReader's methods by default.

@animal: The fix you proposed is ONE of the many fixes, but it is far from the CORRECT fix. Firstly, it doesn't address the fact that the code in OP works fine for 3.1.x but not 3.2.x. Your proposal would demand all clients rewrite their code that uses ArrayStore. 2nd, there's no documentation indicating that when certain special characters exist in ArrayStore field names, one needs to specify mapping (to avoid calling a buggy function). 3rd, shouldn't the mapping done automatically by Ext code when it sees special characters (or by default)?

I'm fine with whichever fix you guys come up with, as long as it doesn't demand users to rewrite their 3.1.x-compatible code. As I said, so far I'm happy with just overriding the 3.2.x code (of createAccessor) with the 3.1.x counterpart.

Animal
29 Jun 2010, 11:14 AM
If truth be told, it should ot have worked in 3.1

JsonReader <-> ArrayStore

Mismatch.

yakovsh
31 Aug 2010, 6:01 AM
We are having a similar problem with JsonReader itself that it does not allow dots (.) in field names when pared with a grid. Our solution was based on this thread (http://www.sencha.com/forum/showthread.php?17246) with a try catch block. The fix appears below but is obviously temporary:

Ext.override(Ext.data.JsonReader, {
createAccessor : function(){
var re = /[\[\.]/;
return function(expr) {
if(Ext.isEmpty(expr)){
return Ext.emptyFn;
}
if(Ext.isFunction(expr)){
return expr;
}
// start modified code
try {
var i = String(expr).search(re);
if(i >= 0){
return new Function('obj', 'return obj' + (i > 0 ? '.' : '') + expr);
}
} catch(e) {
return function(obj){
return obj[expr];
};
}
// end of modified code
return function(obj){
return obj[expr];
};
};
}()
});

yakovsh
31 Aug 2010, 6:01 AM
We are having a similar problem with JsonReader itself that it does not allow dots (.) in field names when pared with a grid. Our solution was based on this thread (http://www.sencha.com/forum/showthread.php?17246) with a try catch block. The fix appears below but is obviously temporary:

Ext.override(Ext.data.JsonReader, {
createAccessor : function(){
var re = /[\[\.]/;
return function(expr) {
if(Ext.isEmpty(expr)){
return Ext.emptyFn;
}
if(Ext.isFunction(expr)){
return expr;
}
// start modified code
try {
var i = String(expr).search(re);
if(i >= 0){
return new Function('obj', 'return obj' + (i > 0 ? '.' : '') + expr);
}
} catch(e) {
return function(obj){
return obj[expr];
};
}
// end of modified code
return function(obj){
return obj[expr];
};
};
}()
});

yakovsh
31 Aug 2010, 6:02 AM
I would also add regarding the comment above that it is possible to have all kinds of characters in database column names, even periods. For example, in SQL Server, as long as everything is escaped properly, any character is fine.

Condor
31 Aug 2010, 6:41 AM
Why do you insist on trying to patch createAccessor to fix this problem? createAccessor isn't the problem!

The problem is that ArrayReader uses JsonReader.buildExtractors and JsonReader.extractValues, which you can simply fix with:

Ext.override(Ext.data.ArrayReader, {
buildExtractors: Ext.emptyFn,
extractValues: Ext.emptyFn
});

yakovsh
31 Aug 2010, 6:44 AM
Why do you insist on trying to patch createAccessor to fix this problem? createAccessor isn't the problem!

The problem is that ArrayReader uses JsonReader.buildExtractors and JsonReader.extractValues
...

Because my problem is not in ArrayReader. Rather, I am having issues with JsonReader itself reading Json data from the server with field names containing periods. Should I file this as a separate bug?

Condor
31 Aug 2010, 6:55 AM
You don't need to patch createAccessor for that either!

Simply specify a mapping, e.g.

{name: 'abc_def', mapping: '["abc.def"]'}
or

{name: 'abc_def', mapping: function(v){ return v['abc.def'];}}

yakovsh
31 Aug 2010, 7:02 AM
That still does not resolve the problem with the library itself that it does not handle periods properly

Condor
31 Aug 2010, 7:11 AM
What do you mean by 'proper'? The '.' in a mapping normally means 'nested object', e.g. {"abc": {"def": 123}}.

If you want to use JsonReader to read properties with '.' in the name then you will have to use a mapping, because it isn't covered by the default behaviour.

Using '.' in a name is not advisable anyway, because you can only access the property using ['name'] notation.

yakovsh
31 Aug 2010, 7:17 AM
When a grid is used with JsonReader, it is displaying data from a database. It does not make sense to impose "." restrictions originating from JSON on data in a database

Condor
31 Aug 2010, 11:52 PM
You're not getting my point!

mapping: 'a.b' is supposed to mean {a: {b: 1}} and not {'a.b': 1}.

Using '.' in property names is not recommended (because javascript also treats it as a child object property if you do not use [] notation).

yakovsh
1 Sep 2010, 6:57 AM
You're not getting my point!

mapping: 'a.b' is supposed to mean {a: {b: 1}} and not {'a.b': 1}.

Using '.' in property names is not recommended (because javascript also treats it as a child object property if you do not use [] notation).

I know why JavaScript does not like it, HOWEVER, JSON allows dots inside strings and so does JavaScript. ExtJS needs to handle this gracefully and not simply say that this is not allowed. At the least this should be documented somewhere.

Additionally, in versions prior to 3.1, this worked because there was a try/catch block in createAccessor.

Animal
1 Sep 2010, 11:45 AM
But dots already have a meaning in Javascript.

They mean access of a property. This is what the mapping scheme does.

If you want to use non-standard property names which contain dots, then inject an extractor function as a mapping.

Juanito
14 Mar 2012, 12:25 PM
The solution to this is to set JsonReader#useSimpleAccessors to true, that will allow brackets and dots in field names without trying to access sub-objects

By the way, for the people that are saying that we should not have fields with brackets or periods in names. In my current project, a database field can have many values (an array). That is denoted by havingh fieldName[0], fieldName[1] and so on. Also, when a field correlates to another, say a field is meant so express the unit to be used when entering data for another field, we use '.unit' at the end For example, weight is the field that stores the number and weight.unit is the field that stores what unit is being stored (Kg, Lbs, grams)

So please don't try to tell people that field names should not have periods or special characters. The feature to access sub-objects with dot or bracket notation is the special case, it should not be the default.