[Mono-devel-list] [PATCH] Fix null reference exceptions in System.Data.DataView
Hi folks,
I just noticed that a bug in DataView.cs is still/again present in the current
revision (r46988, svn://svn.myrealbox.com/source/trunk/mcs/)
The problem is that a variable, rowCache, is often set to null but no null
checks are done. That way a fresh instance of DataView throws a
NullReferenceException e.g. when reading the Count property as it simply does
"return rowCache.Length;".
The fix is to never set the variable to null but to an empty array. The
attached patch implements this fix (only three lines need that fix). I
already sent a patch to fix the very same bug on 11.11.2004, it seems like it
never got included...
C'ya,
Marc
--
Marc Haisenko
Systemspezialist
Webport IT-Services GmbH
mailto: [EMAIL PROTECTED]
Index: DataView.cs
===
--- DataView.cs (revision 46985)
+++ DataView.cs (working copy)
@@ -41,7 +41,7 @@
IExpression rowFilterExpr;
string sort = String.Empty;
protected DataViewRowState rowState;
- protected DataRowView[] rowCache = null;
+ protected DataRowView[] rowCache = new DataRowView[0];
// BeginInit() support
bool isInitPhase = false;
@@ -535,7 +535,7 @@
if (dataTable != null)
UnregisterEventHandlers ();
Index = null;
- rowCache = null;
+ rowCache = new DataRowView[0];
isOpen = false;
}
@@ -703,7 +703,7 @@
{
// TODO: what really happens?
Close ();
- rowCache = null;
+ rowCache = new DataRowView[0];
Open ();
OnListChanged (new ListChangedEventArgs (ListChangedType.Reset, -1 ));
}
___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-devel-list] [PATCH] Fix null reference exceptions in System.Data.DataView
On Wednesday 06 July 2005 14:03, Konstantin Triger wrote: > Hello Marc, > > My view that a problem is deeper: probably we run into a situation where > the DataView is not functional when it should be. Replacing the > assignment of rowCache to an empty array will prevent throwing an > exception, but will not solve the logical problem. > > Please open a bugzilla bug with a test case and a patch attached. > > Regards, > Konstantin Triger I don't think there's a logical problem in DataView.cs, the code seems quite fine to me. Of course a few more bound checks here and there wouldn't hurt, but they only miss in cases where an exception due to an index out bounds is expected anyways. And it works alright for us when rowCache isn't null, and the code is written assuming rowCache will never be null (we it was initialized with null, I don't know). But if you really think it's necessary and that the patch doesn't suffice I can of course file a bug report... C'ya, Marc -- Marc Haisenko Systemspezialist Webport IT-Services GmbH mailto: [EMAIL PROTECTED] ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
[Mono-devel-list] Repost 3: [PATCH] Fix NullReferenceException in DataView.cs
Hi folks,
this is the fourth time I'm sending this patch to this list: the first post
got ignored, the second time it got accepted (applied by Miguel in r36917 on
2004-12-01), then accidently reverted by Konstantin Triger in r44547 on
2005-05-16 (he managed to replace the whole file with one that was partially
out-of-date).
I reposted the adapted patch a second time about a month ago as I noted that
it isn't included in HEAD, which spawned concerns by Konstantin that the
patch doesn't address a deeper design flaw, which I disagreed with. The
complete code assumes rowCache is always an array instance and never null,
and my patch assures it really is never null. But nothing happened, no reply,
no applied patch.
So I'm sending this primitive patch again, since a project our company works
on relies on the correct behaviour of DataView. Please, could someone
re-apply it ? :-) Or at least this time tell me what's wrong with that patch
and what design issue to address to fix it once and for all.
Thnx a lot,
Marc Haisenko
--
Marc Haisenko
Systemspezialist
Webport IT-Services GmbH
mailto: [EMAIL PROTECTED]
Index: DataView.cs
===
--- DataView.cs (revision 47264)
+++ DataView.cs (working copy)
@@ -41,7 +41,7 @@
IExpression rowFilterExpr;
string sort = String.Empty;
protected DataViewRowState rowState;
- protected DataRowView[] rowCache = null;
+ protected DataRowView[] rowCache = new DataRowView[0];
// BeginInit() support
bool isInitPhase = false;
@@ -535,7 +535,7 @@
if (dataTable != null)
UnregisterEventHandlers ();
Index = null;
- rowCache = null;
+ rowCache = new DataRowView[0];
isOpen = false;
}
@@ -703,7 +703,7 @@
{
// TODO: what really happens?
Close ();
- rowCache = null;
+ rowCache = new DataRowView[0];
Open ();
OnListChanged (new ListChangedEventArgs (ListChangedType.Reset, -1 ));
}
___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
[Mono-devel-list] DataView, its very weird history and its future
Hi folks, while investigating DataView.cs and bugs related to it I found the history of this file to be a little weird. First a bit of background: I work for a company that works on an industry project (GUI for an industrial laser) which will run on Portable .NET (aka dotGNU). As you might know or not, dotGNU repackages and uses a part of Mono's class library as "ml-pnet". Among other classes, the complete System.Data namespace from Mono is repackaged. We then found a bug in Mono's DataView back in fall/winter 2004, which I fixed and posted a patch. A few weeks ago we hit another bug with ml-pnet 0.7.0 (more on that later), I checked out the current Mono SubVersion trunk, found the same source as back in fall/winter 2004 and fixed the very same bug. Now it turned out that the patch doesn't apply to the DataView in ml-pnet 0.7.0 as that is a completely different implementation. While investigating this I found this weird history of this file (ISO dates): 2004-11-11: Implementation A: I fixed a bug, posted the patch. 2004-11-30: I reposted the patch 2004-12-01: Miguel de Icaza commits the patch (r36917) 2005-02-01: Implementation B: Atsushi Enomoto "mostly reimplemented" DataView (r39945) 2005-05-11: dotGNU released 0.7.0, including Mono's DataView.cs (implementation B from Atsushi Enomoto, I don't know which SubVersion revision) 2005-05-16: Implementation A: Konstantin Triger overwrites DataView.cs while "merging the Mainsoft branch to the trunk" with a version that pre-dates 2004-12-01 (i.e. over half a year old), completely reverting Atsushis implementation (r44547) 2005-07-06: I fixed the very same bug back from 2004-11-11 and posted the patch. It wasn't a applied since Konstantin was concerned that there is a "deeper problem" with DataView. 2005-07-14: Since I noticed my patch didn't go to trunk and I reposted my patch So my question about the future of DataView: which implementation will be used ? Will implementation A be continued to be used in the future or will there be a revert to implementation B (the one from Atsushi Enomoto) ? E.g., do I need to bother fixing the bug in implementation B or not ;-) C'ya, Marc -- Marc Haisenko Systemspezialist Webport IT-Services GmbH mailto: [EMAIL PROTECTED] ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-devel-list] DataView, its very weird history and its future
On Thursday 14 July 2005 14:07, Atsushi Eno wrote: > Hi Marc, > > I don't think it is worthy of keeping my bunch of hacky fixes > in DataView (it was short term that I touched them), unless the > newer implementation is extremely buggy. Even if it was, Kosta, > Suresh and other Mainsoft/Novell guys would fix regressions. > > Lately there was a bunch of code contribution from Mainsoft and > it is on the integration way i.e. could be temporarily unstable. > > So let's hope that they handle things better than before ;-) > > Atsushi Eno So would it be desired to post a patch that would bring DataView.cs to the state before your changes, including all changes (if any) that happened after Konstantins accident in May ? C'ya, Marc -- Marc Haisenko Systemspezialist Webport IT-Services GmbH mailto: [EMAIL PROTECTED] ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
[Mono-devel-list] [PATCH] Fix parsing of sort strings in DataTable
Hi folks,
here's another patch for the System.Data namespace.
Explanation: you can give a DataTable a sort string (via the Sort property)
which can look like this: "columnName1 ASC" , "columnName1, columnName2 DESC"
or even "columnName1 ASC, columnName2 DESC".
If you want to specify column names that contain spaces you need to "escape"
them... by putting it inside square brackets like this: "[Column name with
spaces]".
The problem is that if someone uses a name starting with a "[" or ending with
a "]" as ColumnName for a DataColumn (e.g., "string2]"), and you give a
DataTable containing such a column a sort string a la "string2] ASC" then
Monos DataTable.ParseSortString throws an ArgumentException.
Unfortunately such a string is valid... and some developers really do name
their columns like that (let's not discuss about that ;-)
The attached patch fixes the behaviour of DataTable.ParseSortString. It also
allows (also valid) strings like "[string2]] ASC" (yes, I've tested against
MS .NET to make sure something like that is valid).
C'ya,
Marc
--
Marc Haisenko
Systemspezialist
Webport IT-Services GmbH
mailto: [EMAIL PROTECTED]
Index: class/System.Data/System.Data/DataTable.cs
===
--- class/System.Data/System.Data/DataTable.cs (revision 47306)
+++ class/System.Data/System.Data/DataTable.cs (working copy)
@@ -1878,12 +1878,42 @@
string[] columnExpression = sort.Trim ().Split (new char[1] {','});
for (int c = 0; c < columnExpression.Length; c++) {
- string[] columnSortInfo = columnExpression[c].Trim ().Split (new char[1] {' '});
+ string rawColumnName = columnExpression[c].Trim ();
+ string[] columnSortInfo;
+
+ if (rawColumnName.StartsWith ("[") && (rawColumnName.IndexOf ("]") > 0)) {
+ // Column name is "escaped" a la "[Name with spaces]", so we can't
+ // split at spaces. We just split it manually.
+ int i = rawColumnName.LastIndexOf ("]");
+
+ if (i + 1 < rawColumnName.Length) {
+ // The "]" is not the last character which means we also have
+ // an optional sort order... we extract that one and trim it.
+ columnSortInfo = new String[2];
+ columnSortInfo[1] = rawColumnName.Substring (i + 1,
+rawColumnName.Length - i - 1).Trim ();
+ } else {
+ // The "]" is the last character, we don't have a sort order
+ columnSortInfo = new String[1];
+ }
+
+ // Get everything between leading "[" and the LAST "]", no trimming !
+ columnSortInfo[0] = rawColumnName.Substring (1, i - 1);
+ } else {
+ // No column name "escaping", just split at spaces and trim just to
+ // be sure.
+ columnSortInfo = rawColumnName.Split (new char[1] {' '});
+
+ // Fix entries (trim strings)
+ for (int i = 0; i < columnSortInfo.Length; i++) {
+ columnSortInfo[i] = columnSortInfo[i].Trim ();
+ }
+ }
- string columnName = columnSortInfo[0].Trim ();
+ string columnName = columnSortInfo[0];
string sortOrder = "ASC";
if (columnSortInfo.Length > 1)
- sortOrder = columnSortInfo[1].Trim ().ToUpper (table.Locale);
+ sortOrder = columnSortInfo[1].ToUpper (table.Locale);
ListSortDirection sortDirection = ListSortDirection.Ascending;
switch (sortOrder) {
@@ -1894,29 +1924,22 @@
sortDirection = ListSortDirection.Descending;
break;
default:
- throw new IndexOutOfRangeException ("Could not find column: " + columnExpression[c]);
+ throw new IndexOutOfRangeException ("Could not find column: " + rawColumnName);
}
- if (columnName.StartsWith("[") || columnName.EndsWith("]")) {
- if (columnName.StartsWith("[") && columnName.EndsWith("]"))
- columnName = columnName.Substring(1, columnName.Length - 2);
- else
- throw new ArgumentException(String.Format("{0} isn't a valid Sort string entry.", columnName));
- }
-
DataColumn dc = table.Columns[columnName];
if (dc == null){
try {
dc = table.Columns[Int32.Parse (columnName)];
- }
- catch (FormatException) {
+ } catch (FormatException) {
throw new IndexOutOfRangeException("Cannot find column " + columnName);
+ }
}
- }
columns.Add (dc);
sorts.Add(sortDirection);
-}
+}
+
sortColumns = (DataColumn[]) columns.ToArray (typeof (DataColumn));
sortDirections = new ListSortDirection[sorts.Count];
for (int i = 0; i < sortDirections.Length; i++)
___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
[Mono-devel-list] [PATCH] Fix "neutral" sorting in DataView
Hi folks,
this simple patch fixes resetting the sorting to default sorting in a
DataView. If you set the Sort property to some string and later pass it null,
it should revert to "no sorting". But unfortunately the old sort string is
still saved and passed to the DataTable. The attached patch fixes this.
C'ya,
Marc
PS: Please don't forget my "Fix parsing of sort strings in DataTable" patch
from Monday ;-)
--
Marc Haisenko
Systemspezialist
Webport IT-Services GmbH
mailto: [EMAIL PROTECTED]
Index: class/System.Data/System.Data/DataView.cs
===
--- class/System.Data/System.Data/DataView.cs (revision 47468)
+++ class/System.Data/System.Data/DataView.cs (working copy)
@@ -270,6 +270,7 @@
if (value == null) {
/* if given value is null useDefaultSort */
useDefaultSort = true;
+ sort = "";
/* if ApplyDefault sort is true try appling it */
if (ApplyDefaultSort == true)
PopulateDefaultSort ();
___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-devel-list] [PATCH] Fix "neutral" sorting in DataView
On Wednesday 20 July 2005 17:57, Boris Kirzner wrote: > Hello Marc, > > I think your patch is fixing the symptom, not the problem. > If you want to fix the problem, you probably should change DataView code > so it always uses Sort property instead of sort private member (except > the places this can not be done), and change the Sort get : it should > return String.Empty if useDefaultSort is true and sort otherwise. Yes, you're right. The code already does use the Sort property in the place that matters (UpdateIndex), so it'd really be better to change get_Sort to return String.Empty if useDefaultSort is true. I'll change that and send in the new patch tomorrow. > And it would be really gentle if you'll supply a test case for your fix. Do you really think the work is worth the hassle for such an obvious error ? I agree that tests that demonstrate the bug are a Good Thing (tm), which is why I've included one with my patch for the ListChanged event in DataView, but, ya know... I'm lazy ;-) > Boris C'ya, -- Marc Haisenko Systemspezialist Webport IT-Services GmbH mailto: [EMAIL PROTECTED] ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-devel-list] [PATCH] Fix "neutral" sorting in DataView
On Wednesday 20 July 2005 18:57, Miguel de Icaza wrote: > Hello, > > > And it would be really gentle if you'll supply a test case for your fix. > > NUnit tests cases are best as we can integrate those into the regular > builds to avoid having regressions. > > Miguel Okay, will do... but I'm out of office until next Wednesday and have more urgent things to do today, so I can't resubmit my patch including a NUnit test before next Wednesday or Thursday (I never wrote a NUnit test but I guess I'll find some examples somewhere). C'ya, Marc -- Marc Haisenko Systemspezialist Webport IT-Services GmbH mailto: [EMAIL PROTECTED] ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-devel-list] [PATCH] Fix "neutral" sorting in DataView
On Thursday 21 July 2005 10:26, Marc Haisenko wrote:
> On Wednesday 20 July 2005 18:57, Miguel de Icaza wrote:
> > Hello,
> >
> > > And it would be really gentle if you'll supply a test case for your
> > > fix.
> >
> > NUnit tests cases are best as we can integrate those into the regular
> > builds to avoid having regressions.
> >
> > Miguel
>
> Okay, will do... but I'm out of office until next Wednesday and have more
> urgent things to do today, so I can't resubmit my patch including a NUnit
> test before next Wednesday or Thursday (I never wrote a NUnit test but I
> guess I'll find some examples somewhere).
Oh dear... luckily I now managed to write the stuff today. I've wasted two and
a half hours just for that silly test ! Would anyone care to explain to me
why the line
Assert.AreEqual (1, 2, "Foo");
causes mcs to complain "error CS0119: Expression denotes a `method group',
where a `variable', `value' or `type' was expected" ?
Anyway, attached are the fixed patch and an NUnit test. Hope it's what you
wanted from me, it fails on an unpatched Mono and passes when my patch is
applied.
C'ya,
Marc
--
Marc Haisenko
Systemspezialist
Webport IT-Services GmbH
mailto: [EMAIL PROTECTED]
Index: class/System.Data/System.Data/DataView.cs
===
--- class/System.Data/System.Data/DataView.cs (revision 47468)
+++ class/System.Data/System.Data/DataView.cs (working copy)
@@ -258,7 +258,13 @@
[DataSysDescription ("Indicates the order in which data is returned by this DataView.")]
[DefaultValue ("")]
public string Sort {
- get { return sort; }
+ get {
+if (useDefaultSort)
+ return String.Empty;
+else
+ return sort;
+ }
+
set {
if (isInitPhase) {
initSort = value;
@@ -267,7 +273,7 @@
if (value == sort)
return;
-if (value == null) {
+if ((value == null) || (value.Equals (String.Empty))) {
/* if given value is null useDefaultSort */
useDefaultSort = true;
/* if ApplyDefault sort is true try appling it */
// DataViewSortingTest.cs - Nunit Test Cases for testing the DataView
// sorting
// Authors:
// Marc Haisenko ([EMAIL PROTECTED])
//
// Permission is hereby granted, free of charge, to any person obtaining
// a copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to
// permit persons to whom the Software is furnished to do so, subject to
// the following conditions:
//
// The above copyright notice and this permission notice shall be
// included in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
// LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
// OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
// WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
//
using NUnit.Framework;
using System;
using System.Data;
using System.ComponentModel;
using System.IO;
using MonoTests.System.Data.Test.Utils;
namespace MonoTests.System.Data
{
[TestFixture]
public class DataViewSortingTest : Assertion
{
DataTable dataTable;
DataView dataView;
[SetUp]
public void GetReady ()
{
dataTable = new DataTable ("itemTable");
DataColumn dc1 = new DataColumn ("itemId", typeof(int));
DataColumn dc2 = new DataColumn ("itemName", typeof(string));
dataTable.Columns.Add (dc1);
dataTable.Columns.Add (dc2);
dataTable.Rows.Add (new object[2] { 1, "First entry" });
dataTable.Rows.Add (new object[2] { 0, "Second entry" });
dataTable.Rows.Add (new object[2] { 3, "Third entry" });
dataTable.Rows.Add (new object[2] { 2, "Fourth entry" });
dataView = new DataView (dataTable);
}
[TearDown]
public void Clean ()
{
dataTable = null;
dataView = null;
}
public void MyAssert (int a, Object b, string s)
{
AssertEquals (s, a, (int) b);
}
[Test]
public void Test ()
{
string s = "Default sorting: ";
MyAssert (1, dataView[0][0], s + "First entry has wrong item");
MyAssert (0, dataView[1][0], s + "Second entry has wrong item");
MyAssert (3, dataView[2][0], s + "Third entry has wrong item");
MyAssert (2, dataView[3][0], s + "Fourth entry has wrong item");
s
Re: [Mono-devel-list] [PATCH] Fix "neutral" sorting in DataView
On Thursday 21 July 2005 17:18, Atsushi Eno wrote: > Hi Marc, > > > Oh dear... luckily I now managed to write the stuff today. I've wasted > > two and a half hours just for that silly test ! Would anyone care to > > explain to me why the line > > Assert.AreEqual (1, 2, "Foo"); > > causes mcs to complain "error CS0119: Expression denotes a `method > > group', where a `variable', `value' or `type' was expected" ? > > Because there is both Assert class and Assert method in that scope. > Since the test class is derived from Assertion, it guesses "Assert" > is NUnit.Framework.Assertion.Assert method, not NUnit.Framework.Assert > class. Aah, thanks a lot :-) But I guess a better error message should be displayed here, it'd be better if mcs would complain about ambiguity. I didn't had (and still don't have) an idea what a "method group" is supposed to mean. > So if you want to use static Assert methods, you can use alias by > using statement, like: > > using AssertType = NUnit.Framework.Assert; > > and call AssertType.AreEqual(). > > Atsushi Eno Thanks for the explanations, Marc -- Marc Haisenko Systemspezialist Webport IT-Services GmbH mailto: [EMAIL PROTECTED] ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
